Bug 49366 – [Chromium][DRT] EventSender.contextClick() should aware spellchecking

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Hajime Morrita]

Return value of EventSender.contextClick() doesn't care about spellchecking, that makes
editing/spelling/context-menu-suggestions.html fail.
See also Bug 39105.

[Hajime Morrita]

Created attachment 74087 [details]
Patch

[WebKit Review Bot]

Attachment 74087 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6218010

[Kent Tamura]

Comment on attachment 74087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74087&action=review

> LayoutTests/ChangeLog:9
> +        - Added a test for improve the coverage.
> +        - Unskipped one passed test.

Do not do two things in one patch.  Adding a test (not Chromium-specific) should be separated from the DRT change (Chromium-specific).

> LayoutTests/editing/spelling/script-tests/context-menu-suggestions-for-selection.js:51
>  \ No newline at end of file

Add a line-break.

> LayoutTests/platform/chromium/test_expectations.txt:696
> -// Need to dump context menu items on eventSender.contextClick(true).
> -BUGWK39105 : editing/spelling/context-menu-suggestions.html = TEXT
> +// Need to enable mock spellchecker. see Bug 45633
> +BUGWK45633 WIN LINUX : editing/spelling/context-menu-suggestions.html = TEXT

The DRT change is DRT-only.  So test_shell still fail with it, right?

[Hajime Morrita]

Created attachment 74089 [details]
Patch

[WebKit Review Bot]

Attachment 74089 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6177014

[Kent Tamura]

Comment on attachment 74089 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74089&action=review

> WebKitTools/DumpRenderTree/chromium/EventSender.cpp:756
> +        for (int i = 0; i < suggestions.size(); ++i) 

"i" should be unsigned.

> WebKitTools/DumpRenderTree/chromium/MockSpellCheck.cpp:95
> +void MockSpellCheck::fillSuggestionList(const WebString& word,
> +                                        Vector<WebString>* suggestions)

nit: no need to break the line

> WebKitTools/DumpRenderTree/chromium/MockSpellCheck.h:66
> +    void fillSuggestionList(const WebKit::WebString& word,
> +                            Vector<WebKit::WebString>* suggestions);

nit: no need to break the line

[Hajime Morrita]

Created attachment 74090 [details]
fixing cr-linux build failure

[Hajime Morrita]

Hi Kent-san, thank you for reviewing!
I updated the patch.

(In reply to comment #3)
> (From update of attachment 74087 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74087&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        - Added a test for improve the coverage.
> > +        - Unskipped one passed test.
> Do not do two things in one patch.  Adding a test (not Chromium-specific) should be separated from the DRT change (Chromium-specific).

Sure. I removed the test from this patch. It will be filed as another bug
and fix it after this change is ported to test_shell.

> > LayoutTests/platform/chromium/test_expectations.txt:696
> > -// Need to dump context menu items on eventSender.contextClick(true).
> > -BUGWK39105 : editing/spelling/context-menu-suggestions.html = TEXT
> > +// Need to enable mock spellchecker. see Bug 45633
> > +BUGWK45633 WIN LINUX : editing/spelling/context-menu-suggestions.html = TEXT
> 
> The DRT change is DRT-only.  So test_shell still fail with it, right?
Yes. I discarded this change. I'll do this after test_shell porting.

[Hajime Morrita]

Created attachment 74091 [details]
Patch

[Hajime Morrita]

(In reply to comment #6)
> (From update of attachment 74089 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74089&action=review
> 
> > WebKitTools/DumpRenderTree/chromium/EventSender.cpp:756
> > +        for (int i = 0; i < suggestions.size(); ++i) 
> 
> "i" should be unsigned.
Fixed. It broke cr-linux build...

> 
> > WebKitTools/DumpRenderTree/chromium/MockSpellCheck.cpp:95
> > +void MockSpellCheck::fillSuggestionList(const WebString& word,
> > +                                        Vector<WebString>* suggestions)
> 
> nit: no need to break the line
Fixed.

> 
> > WebKitTools/DumpRenderTree/chromium/MockSpellCheck.h:66
> > +    void fillSuggestionList(const WebKit::WebString& word,
> > +                            Vector<WebKit::WebString>* suggestions);
> 
> nit: no need to break the line
Fixed.

[Hajime Morrita]

Committed r72183: <http://trac.webkit.org/changeset/72183>