Update spellchecker context menu suggestions for multiprocess

RESOLVED FIXED in mozilla34

Status

()

Toolkit
XUL Widgets
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ally, Assigned: ally)

Tracking

unspecified
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(e10s+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
tracking-e10s: --- → ?
Depends on: 1030449, 693555
(Assignee)

Updated

4 years ago
Assignee: nobody → ally
Assignee: ally → nobody
Blocks: 997456
tracking-e10s: ? → +
Assignee: nobody → ally
Status: NEW → ASSIGNED
OS: Windows 8 → All
Hardware: x86_64 → All
Blocks: 997462
No longer blocks: 997456
Priority: -- → P1

Updated

4 years ago
Duplicate of this bug: 1040909
(Assignee)

Updated

4 years ago
Summary: Update spellchecker context menu for multiprocess → Update spellchecker context menu suggestions for multiprocess
(Assignee)

Comment 2

4 years ago
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
(Assignee)

Comment 3

4 years ago
Created attachment 8467906 [details] [diff] [review]
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.
Attachment #8467906 - Flags: feedback?(wmccloskey)
Comment on attachment 8467906 [details] [diff] [review]
RpcToContextMenu - functions

Looks good!
Attachment #8467906 - Flags: feedback?(wmccloskey) → feedback+
(Assignee)

Comment 5

4 years ago
(Assignee)

Comment 6

4 years ago
Created attachment 8469709 [details] [diff] [review]
RpcContextMenuFinalDraft

A more respectable version.
Attachment #8467906 - Attachment is obsolete: true
Attachment #8469709 - Flags: review?(wmccloskey)
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).
Attachment #8469709 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 8

4 years ago
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/9ea4953de5b3
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.