Bug 74393 – [Chromium] Enable grammar checking

• Component: HTML Editing
• Status: RESOLVED FIXED

[Kenichi Ishibashi]

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=editing%2Fspelling%2Fgrammar-paste.html

[Kenichi Ishibashi]

Committed suppression as r102668: <http://trac.webkit.org/changeset/102668>

[Hironori Bono]

Created attachment 142680 [details]
A patch v0

Greetings,

I have quickly implemented a mock grammar checker not only to fix this issue but also to enable grammar checking on Chromium. I would like to set r? to test this change on EWS bots.

Regards,

Hironori Bono

[Ryosuke Niwa]

The change in the editing code looks sane.

james, enne: could you look at the chance in GraphicsContextSkia.cpp and see if that makes sense?

[James Robinson]

Comment on attachment 142680 [details]
A patch v0

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

> Source/WebCore/ChangeLog:14
> +        (WebCore::GraphicsContext::drawLineForDocumentMarker): render grammar marker with gray on Windows and Linux or green on Mac.

is there any test coverage for this part?

[Hajime Morrita]

Comment on attachment 142680 [details]
A patch v0

We could have another paste test just for checking grammar rendering.
Or we could turn existing one to pixel+test test.

[Ryosuke Niwa]

(In reply to comment #5)
> (From update of attachment 142680 [details])
> We could have another paste test just for checking grammar rendering.
> Or we could turn existing one to pixel+test test.

Note we can use dumpAsText(true) to force generating png results (no render tree dump).

[Hironori Bono]

Created attachment 142983 [details]
Patch v1 (added a pixel test)

Greetings,

Thanks for your review and comments. Even though I have added a pixel test, I'm not sure if it succeeds on bots.

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 142985 [details]
Patch v2 (fixed an image)

Greetings,

Oops, I have attached a wrong image for Chromium Linux.

Regards,

Hironori Bono

[WebKit Review Bot]

Comment on attachment 142985 [details]
Patch v2 (fixed an image)

Attachment 142985 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12734578

New failing tests:
editing/spelling/grammar-markers.html

[WebKit Review Bot]

Created attachment 142992 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hajime Morrita]

Comment on attachment 142985 [details]
Patch v2 (fixed an image)

Looks good. As Ryosuke mentioned, dumpAsText(true) will suppress render tree dump an simplify the expectation.

[Hironori Bono]

Created attachment 143180 [details]
Patch v3 (updated a test and its expected image)

Greetings Morita-san,

Thanks for your review and advice.
I have updated the test and its expected image. (The image created by my Linux does not seem to match with the one created by the bot.)

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 143256 [details]
Patch v4 (removed unnecessary files)

Greetings,

Sorry, I forgot removing unnecessary files from my previous change, which added layoutTestController.dumpAsText(true) to its layout test.

Regards,

Hironori Bono

[Ryosuke Niwa]

Comment on attachment 143256 [details]
Patch v4 (removed unnecessary files)

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

> Tools/DumpRenderTree/chromium/MockGrammarCheck.h:50
> +    ~MockGrammarCheck();

Why do we need the constructor & destructor here?

> LayoutTests/editing/spelling/grammar-markers.html:20
> +var dstNode = document.getElementById('dst');

Please don't use abbreviations like dst.

> LayoutTests/editing/spelling/grammar-markers.html:24
> +var nretry = 10;

Please spell out the variable name instead of using obnoxious prefix like n.

[Hironori Bono]

Created attachment 143449 [details]
Patch v5 (applied comments and updated to top-of-trunk)

Greetings Niwa-san,

Thanks for your review and comments. I have updated my change to apply your comments. (Also, it solves a conflict in GraphicsContextSkia.cpp.)

Regards,

Hironori Bono

[WebKit Review Bot]

Comment on attachment 143449 [details]
Patch v5 (applied comments and updated to top-of-trunk)

Attachment 143449 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12780054

[Ryosuke Niwa]

Comment on attachment 143449 [details]
Patch v5 (applied comments and updated to top-of-trunk)

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

> Source/WebKit/chromium/src/WebTextCheckingResult.cpp:49
> +        detail.location = 0;

Why is the offset always zero? WebEditorClient::checkGrammarOfString sets this to location.

> Source/WebKit/chromium/src/WebTextCheckingResult.cpp:51
> +        detail.userDescription = replacement;

I don't think this is the correct use of user description.

> Source/WebKit/chromium/src/WebTextCheckingResult.cpp:52
> +        result.details.append(detail);

Is it really okay not to fill up the guesses list?

> Tools/DumpRenderTree/chromium/MockGrammarCheck.h:40
> +namespace WebKit {
> +class WebString;
> +struct WebTextCheckingResult;
> +}

Please place a blank line after { and before }.

> LayoutTests/editing/spelling/grammar-markers.html:5
> +<head>
> +<title></title>
> +</head>

No need for head/title.

[Hironori Bono]

Comment on attachment 143449 [details]
Patch v5 (applied comments and updated to top-of-trunk)

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

>> Source/WebKit/chromium/src/WebTextCheckingResult.cpp:49
>> +        detail.location = 0;
> 
> Why is the offset always zero? WebEditorClient::checkGrammarOfString sets this to location.

Technically, Editor::markAndReplaceFor uses (result.location + detail.location) as the beginning of a grammar marker. So, if we set result.location to 0, this value should be location. On the other hand, if we set result.location to location, this value should be 0. (I'm not sure which is the expected usage.)

>> Source/WebKit/chromium/src/WebTextCheckingResult.cpp:51
>> +        detail.userDescription = replacement;
> 
> I don't think this is the correct use of user description.

Editor::markAndReplaceFor somehow uses this value as the description of a grammar marker: <http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/editing/Editor.cpp&type=cs&l=2054>, i.e. this function discards replacement and the guesses list when it adds a grammar marker.

>> Source/WebKit/chromium/src/WebTextCheckingResult.cpp:52
>> +        result.details.append(detail);
> 
> Is it really okay not to fill up the guesses list?

In brief, the above function discards the guesses list, unfortunately.

[Hironori Bono]

Comment on attachment 143449 [details]
Patch v5 (applied comments and updated to top-of-trunk)

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

>> LayoutTests/editing/spelling/grammar-markers.html:5
>> +</head>
> 
> No need for head/title.

If I recall correctly, HTML5 needs them. (In my personal opinion, I would like to make this file compliant with HTML5 because it has "<!DOCTYPE html>".)

[Ryosuke Niwa]

Comment on attachment 143449 [details]
Patch v5 (applied comments and updated to top-of-trunk)

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

>>> Source/WebKit/chromium/src/WebTextCheckingResult.cpp:51
>>> +        detail.userDescription = replacement;
>> 
>> I don't think this is the correct use of user description.
> 
> Editor::markAndReplaceFor somehow uses this value as the description of a grammar marker: <http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/editing/Editor.cpp&type=cs&l=2054>, i.e. this function discards replacement and the guesses list when it adds a grammar marker.

That is very misleading. Perhaps we should rename this member variable.

>>> LayoutTests/editing/spelling/grammar-markers.html:5
>>> +</head>
>> 
>> No need for head/title.
> 
> If I recall correctly, HTML5 needs them. (In my personal opinion, I would like to make this file compliant with HTML5 because it has "<!DOCTYPE html>".)

I don't think so.

[Hironori Bono]

Comment on attachment 143449 [details]
Patch v5 (applied comments and updated to top-of-trunk)

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

>>>> LayoutTests/editing/spelling/grammar-markers.html:5
>>>> +</head>
>>> 
>>> No need for head/title.
>> 
>> If I recall correctly, HTML5 needs them. (In my personal opinion, I would like to make this file compliant with HTML5 because it has "<!DOCTYPE html>".)
> 
> I don't think so.

Do I misunderstand the spec? Section 4.1.1 (*1) seems to write the content model of an <html> element includes a <head> element and Section 4.2.1 (*2) seems to write the content mode of an <head> element includes a <title> element, respectively.
(*1) <http://www.w3.org/TR/html5/the-html-element.html#the-html-element>
(*2) <http://www.w3.org/TR/html5/the-head-element.html>
Anyway, I do not have any objections to remove them itself and I'm going to upload another change that removes them and added blank lines to MockGrammarCheck.h.

[Hironori Bono]

Created attachment 143520 [details]
Patch v6 (fixed a build break and a couple of style issues)

Greetings Niwa-san,

Sorry, my previous change does not only have style issues but also it causes a build break on Chromium. (This is just a bonehead mistake in creating my previous change.) Even though I think I have applied your comments, I may miss some as usual. It would be definitely helpful to blame me if I miss your comments.

Regards,

Hironori Bono

[WebKit Review Bot]

Comment on attachment 143520 [details]
Patch v6 (fixed a build break and a couple of style issues)

Attachment 143520 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12780168

New failing tests:
editing/spelling/grammar-markers.html

[WebKit Review Bot]

Created attachment 143545 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Ryosuke Niwa]

Comment on attachment 143520 [details]
Patch v6 (fixed a build break and a couple of style issues)

r=me provided you fix the test :)

[Hironori Bono]

Created attachment 143766 [details]
Patch v7 (updated a test result, for landing)

Greetings Niwa-san,

Many thanks for your review and comments.
I have updated a pixel-test result to work with the latest WebKit. I will land it if it does not cause any problems.

Regards,

Hironori Bono

[WebKit Review Bot]

Comment on attachment 143766 [details]
Patch v7 (updated a test result, for landing)

Clearing flags on attachment: 143766

Committed r118352: <http://trac.webkit.org/changeset/118352>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 87390

[Hironori Bono]

Created attachment 143979 [details]
Patch v8 (fixed assertion errors)

Greetings,

Unfortunately, my previous change unveiled assertion errors as written in Bug 87390. (I did not notice this "ASSERT(badGrammarLocation == -1)" because I have not tested my previous change with Debug builds.) I have fixed EditorClientImpl::checkGrammarOfString() to set badGrammarLocation to -1 instead of 0. (When I tested the latest change with a Debug build on my Win7 box, it finishes without crashes.)

Regards,

Hironori Bono

[Ryosuke Niwa]

(In reply to comment #30)
> Unfortunately, my previous change unveiled assertion errors as written in Bug 87390. (I did not notice this "ASSERT(badGrammarLocation == -1)" because I have not tested my previous change with Debug builds.) I have fixed EditorClientImpl::checkGrammarOfString() to set badGrammarLocation to -1 instead of 0. (When I tested the latest change with a Debug build on my Win7 box, it finishes without crashes.)

You're talking about the assertion in findFirstBadGrammar, right?

findBadGrammars in TextCheckingHelper.cpp has:
        int badGrammarLocation = -1;
        int badGrammarLength = 0;
        Vector<GrammarDetail> badGrammarDetails;
        client->checkGrammarOfString(text + checkLocation, checkLength, badGrammarDetails, &badGrammarLocation, &badGrammarLength);
        if (!badGrammarLength)
            break;
        ASSERT(0 <= badGrammarLocation && badGrammarLocation <= checkLength);
        ASSERT(0 < badGrammarLength && badGrammarLocation + badGrammarLength <= checkLength);

We should change these assertions to match each other (probably in a separate patch).

[Hironori Bono]

Greetings Niwa-san,

Thanks for your quick response.
Yes, it is an assert in that function: <http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/editing/TextCheckingHelper.cpp&exact_package=chromium&q=findFirstBadGrammar&type=cs&l=495>.

Regards,

Hironori Bono

[WebKit Review Bot]

Comment on attachment 143979 [details]
Patch v8 (fixed assertion errors)

Clearing flags on attachment: 143979

Committed r118479: <http://trac.webkit.org/changeset/118479>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.