Bug 74393 - [Chromium] Enable grammar checking
Summary: [Chromium] Enable grammar checking
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shinya Kawanaka
URL:
Keywords:
Depends on: 87390
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-13 02:34 PST by Kenichi Ishibashi
Modified: 2012-05-24 23:22 PDT (History)
10 users (show)

See Also:


Attachments
A patch v0 (18.90 KB, patch)
2012-05-18 03:54 PDT, Hironori Bono
morrita: review+
morrita: commit-queue-
Details | Formatted Diff | Diff
Patch v1 (added a pixel test) (86.73 KB, patch)
2012-05-21 03:30 PDT, Hironori Bono
no flags Details | Formatted Diff | Diff
Patch v2 (fixed an image) (76.65 KB, patch)
2012-05-21 03:34 PDT, Hironori Bono
morrita: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (482.72 KB, application/zip)
2012-05-21 04:31 PDT, WebKit Review Bot
no flags Details
Patch v3 (updated a test and its expected image) (74.45 KB, patch)
2012-05-21 20:28 PDT, Hironori Bono
no flags Details | Formatted Diff | Diff
Patch v4 (removed unnecessary files) (73.95 KB, patch)
2012-05-22 03:46 PDT, Hironori Bono
no flags Details | Formatted Diff | Diff
Patch v5 (applied comments and updated to top-of-trunk) (73.67 KB, patch)
2012-05-22 21:17 PDT, Hironori Bono
rniwa: review+
rniwa: commit-queue-
Details | Formatted Diff | Diff
Patch v6 (fixed a build break and a couple of style issues) (73.64 KB, patch)
2012-05-23 03:20 PDT, Hironori Bono
rniwa: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (651.80 KB, application/zip)
2012-05-23 05:46 PDT, WebKit Review Bot
no flags Details
Patch v7 (updated a test result, for landing) (73.49 KB, patch)
2012-05-24 02:09 PDT, Hironori Bono
no flags Details | Formatted Diff | Diff
Patch v8 (fixed assertion errors) (73.95 KB, patch)
2012-05-24 22:37 PDT, Hironori Bono
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Kenichi Ishibashi 2011-12-13 02:39:59 PST
Committed suppression as r102668: <http://trac.webkit.org/changeset/102668>
Comment 2 Hironori Bono 2012-05-18 03:54:14 PDT
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
Comment 3 Ryosuke Niwa 2012-05-18 12:37:17 PDT
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?
Comment 4 James Robinson 2012-05-18 12:42:02 PDT
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?
Comment 5 Hajime Morrita 2012-05-21 00:45:16 PDT
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.
Comment 6 Ryosuke Niwa 2012-05-21 01:39:35 PDT
(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).
Comment 7 Hironori Bono 2012-05-21 03:30:24 PDT
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
Comment 8 Hironori Bono 2012-05-21 03:34:30 PDT
Created attachment 142985 [details]
Patch v2 (fixed an image)

Greetings,

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

Regards,

Hironori Bono
Comment 9 WebKit Review Bot 2012-05-21 04:31:16 PDT
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
Comment 10 WebKit Review Bot 2012-05-21 04:31:21 PDT
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
Comment 11 Hajime Morrita 2012-05-21 17:43:09 PDT
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.
Comment 12 Hironori Bono 2012-05-21 20:28:13 PDT
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
Comment 13 Hironori Bono 2012-05-22 03:46:23 PDT
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
Comment 14 Ryosuke Niwa 2012-05-22 09:30:59 PDT
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.
Comment 15 Hironori Bono 2012-05-22 21:17:45 PDT
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
Comment 16 WebKit Review Bot 2012-05-22 22:21:39 PDT
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
Comment 17 Ryosuke Niwa 2012-05-22 22:22:52 PDT
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.
Comment 18 Hironori Bono 2012-05-22 23:50:28 PDT
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.
Comment 19 Hironori Bono 2012-05-22 23:57:53 PDT
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>".)
Comment 20 Ryosuke Niwa 2012-05-23 00:34:02 PDT
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.
Comment 21 Hironori Bono 2012-05-23 01:03:26 PDT
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.
Comment 22 Hironori Bono 2012-05-23 03:20:56 PDT
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
Comment 23 WebKit Review Bot 2012-05-23 05:46:20 PDT
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
Comment 24 WebKit Review Bot 2012-05-23 05:46:39 PDT
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
Comment 25 Ryosuke Niwa 2012-05-23 11:49:18 PDT
Comment on attachment 143520 [details]
Patch v6 (fixed a build break and a couple of style issues)

r=me provided you fix the test :)
Comment 26 Hironori Bono 2012-05-24 02:09:16 PDT
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
Comment 27 WebKit Review Bot 2012-05-24 05:03:19 PDT
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>
Comment 28 WebKit Review Bot 2012-05-24 05:03:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 WebKit Review Bot 2012-05-24 07:51:59 PDT
Re-opened since this is blocked by 87390
Comment 30 Hironori Bono 2012-05-24 22:37:01 PDT
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
Comment 31 Ryosuke Niwa 2012-05-24 22:43:21 PDT
(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).
Comment 32 Hironori Bono 2012-05-24 22:51:16 PDT
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
Comment 33 WebKit Review Bot 2012-05-24 23:22:17 PDT
Comment on attachment 143979 [details]
Patch v8 (fixed assertion errors)

Clearing flags on attachment: 143979

Committed r118479: <http://trac.webkit.org/changeset/118479>
Comment 34 WebKit Review Bot 2012-05-24 23:22:24 PDT
All reviewed patches have been landed.  Closing bug.