1030451 - Update spellchecker context menu suggestions for multiprocess

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P1)

Description

[Allison Naaktgeboren :ally]

breaking out bug 693555 into multiple parts. With e10s, we will need to refactor the contextmenu code path to not call into the child once the menu is triggered. So the child must send all the information the parent could need for hte spellchecking context menu with its 'contextmenu' message. The parent can then control the dictionaries from there.

Comment 2

[Allison Naaktgeboren :ally]

callstack from user click -> mozHunspell:Suggest()
xul.dll!mozHunspell::Suggest(const wchar_t * aWord, wchar_t * * * aSuggestions, unsigned int * aSuggestionCount) Line 536	C++
xul.dll!mozSpellChecker::CheckWord(const nsAString_internal & aWord, bool * aIsMisspelled, nsTArray<nsString> * aSuggestions) Line 146	C++
xul.dll!nsEditorSpellCheck::CheckCurrentWord(const wchar_t * aSuggestedWord, bool * aIsMisspelled) Line 453	C++
xul.dll!NS_InvokeByIndex(nsISupports * that, unsigned int methodIndex, unsigned int paramCount, nsXPTCVariant * params)
xul.dll!CallMethodHelper::Invoke()
xul.dll!CallMethodHelper::Call()
xul.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx, XPCWrappedNative::CallMode mode)
js -> cpp
InlineSpellChecker.prototype.AddSuggestionsToMenu calls -> CheckCurrentWord()
nsContextMenu.prototype.initSpellingItems
CM_initItems
CM_initMenu
onpopupshowing
right click

Comment 3

[Allison Naaktgeboren :ally]

Attachment: RpcToContextMenu - functions

At Bill's suggestion, in this patch the PRemoteSpellCheckEngine interface has been made into rpc. Not cleaned up.

The list of suggestions appears if available and replace works as well.

Flagging Bill for feedback on the rpc bits before I proceed further.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8467906 [details] [diff] [review]
RpcToContextMenu - functions

Looks good!

Comment 6

[Allison Naaktgeboren :ally]

Attachment: RpcContextMenuFinalDraft

A more respectable version.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8469709 [details] [diff] [review]
RpcContextMenuFinalDraft

Review of attachment 8469709 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a peer here, but I don't think anyone will mind. I just have some nits.

::: extensions/spellcheck/hunspell/src/PRemoteSpellcheckEngine.ipdl
@@ +16,2 @@
>  
> +  rpc CheckForMisspellingAndSuggestions(nsString aWord) returns (bool isMisspelled, nsString[] suggestions, int count);

Can we just name these Check() and CheckAndSuggest()? CheckForMisspellingAndSuggestions is a mouthful, and it's not really grammatical. It might also make sense to return |isCorrect| rather than |isMisspelled| since that's what the other code seems to do.

::: extensions/spellcheck/hunspell/src/RemoteSpellCheckEngineParent.cpp
@@ +34,2 @@
>    const nsString& aWord,
>    bool* isMisspelled)

This should be aIsMisspelled.

@@ +44,5 @@
> +RemoteSpellcheckEngineParent::AnswerCheckForMisspellingAndSuggestions(
> +  const nsString& aWord,
> +  bool* isMisspelled,
> +  InfallibleTArray<nsString>* suggestions,
> +  int* count)

Need to use aFoo for all these params. Also, |count| appears to be unused.

@@ +51,5 @@
> +  mEngine->Check(aWord.get(), &isCorrect);
> +  *isMisspelled = !isCorrect;
> +  if (!isCorrect) {
> +    char16_t **fauxSuggestions;
> +    uint32_t i, fauxCount = 0;

If you use aFoo above, you can drop the faux prefixes here. Also, |i| should be declared in the loop header.

@@ +54,5 @@
> +    char16_t **fauxSuggestions;
> +    uint32_t i, fauxCount = 0;
> +    mEngine->Suggest(aWord.get(), &fauxSuggestions, &fauxCount);
> +
> +    for (i=0;i<fauxCount;i++) {

This should be |for (int i = 0; i < count; i++) {| (note the spacing).

Comment 8

[Allison Naaktgeboren :ally]

(In reply to Bill McCloskey (:billm) from comment #7)
> Comment on attachment 8469709 [details] [diff] [review]
> RpcContextMenuFinalDraft
> 
> Review of attachment 8469709 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not a peer here, but I don't think anyone will mind. I just have some
> nits.

True, however You're the grand master of the new ipc design, which is what this patch hinges on and makes other uncomfortable reviewing it.

> ::: extensions/spellcheck/hunspell/src/PRemoteSpellcheckEngine.ipdl
> It might also make sense to return |isCorrect| rather than |isMisspelled| since that's what the other code seems to do.

My original patch on Bug 693555 did return isCorrect. However, :mrbkap had me move that to its current location https://bugzilla.mozilla.org/show_bug.cgi?id=693555#c27.  Ehsan was ok with blake's way, so that's what we did. https://bugzilla.mozilla.org/show_bug.cgi?id=693555#c30

If you would like me to change it back, I can do so. You'll need to get mrbkap on board though.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I see. I mistakenly thought that we always use isCorrect, but I guess there's a mixture already. Seems fine to leave it as isMisspelled.

Comment 10

[Allison Naaktgeboren :ally]

 https://hg.mozilla.org/integration/fx-team/rev/9ea4953de5b3

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9ea4953de5b3