Bug 127284 - Refactoring inline_spelling_markers.html to use asynchronous spellchecking
Summary: Refactoring inline_spelling_markers.html to use asynchronous spellchecking
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: Grzegorz Czajkowski
URL:
Keywords:
Depends on:
Blocks: 125689
  Show dependency treegraph
 
Reported: 2014-01-20 04:43 PST by Grzegorz Czajkowski
Modified: 2014-02-25 05:26 PST (History)
9 users (show)

See Also:


Attachments
Patch (541.41 KB, patch)
2014-01-20 05:25 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
Took Ryosuke's comments into account (542.65 KB, patch)
2014-02-17 07:10 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (487.18 KB, application/zip)
2014-02-17 08:31 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (487.84 KB, application/zip)
2014-02-17 09:33 PST, Build Bot
no flags Details
make Mac bot green (542.67 KB, patch)
2014-02-18 23:58 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
apply Ryosuke's comments (543.11 KB, patch)
2014-02-25 00:30 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Grzegorz Czajkowski 2014-01-20 04:43:46 PST
Use asynchronous text checking in:
 - inline_spelling_markers.html,
 - inline-spelling_markers-hidpi.html.
Comment 1 Grzegorz Czajkowski 2014-01-20 05:25:52 PST
Created attachment 221654 [details]
Patch
Comment 2 Grzegorz Czajkowski 2014-01-20 11:46:32 PST
This test and its hdipi brother were rewritten considerably. That's why I decided to submit them separately.

I'd appreciate your comments.
Comment 3 Ryosuke Niwa 2014-02-13 23:49:13 PST
Comment on attachment 221654 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        inline-spelling_markers-hidpi.html

Nit: should read inline-spelling-markers-hidpi.html

> LayoutTests/ChangeLog:19
> +        * editing/spelling/inline_spelling_markers-expected.txt: Added.
> +        Make cross platform text expectation by dumping spelling/grammar markers
> +        instead of whole tree which in terms of spellchecking says nothing.
> +
> +        * editing/spelling/inline_spelling_markers.html:
> +        Remove unnecessary new lines from div elements so position of markers can be given from 0.
> +        Activate text checking by adding a word separator so WebKit starts performing spell and grammar
> +        checking. Selection change does not invoke grammar checking unless we start editing the input
> +        element's content.

Please rename the test to inline-spelling-markers.html to match the naming convention.

> LayoutTests/editing/spelling/inline-spelling-markers-hidpi.html:63
> +function typeIncorrectPhrase(element)
> +{
> +    element.focus();
> +    document.execCommand("InsertText", false, incorrectPhrase);
> +}

This function seems to hide what we're really doing. It's better to put in runTextCheckingTestFor directly.

> LayoutTests/editing/spelling/inline-spelling-markers-hidpi.html:70
> +function performTextChecking()
> +{
> +    const wordSeparator = " ";
> +    // Add a word separator so both spelling and grammar markers will appear.
> +    document.execCommand("InsertText", false, wordSeparator);
> +}

Ditto.

> LayoutTests/editing/spelling/inline-spelling-markers-hidpi.html:76
> +function verifyMarkers()
> +{
> +    if (!window.internals)
> +        return done();
>  

We can't test this at all in browser!?
Comment 4 Grzegorz Czajkowski 2014-02-17 07:10:33 PST
Created attachment 224364 [details]
Took Ryosuke's comments into account
Comment 5 Grzegorz Czajkowski 2014-02-17 07:16:30 PST
Comment on attachment 221654 [details]
Patch

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

>> LayoutTests/editing/spelling/inline-spelling-markers-hidpi.html:76
>>  
> 
> We can't test this at all in browser!?

This function only verifies markers using shouldBecomeEqual*. All desired elements and their content with incorrect phrase have been already created, done() here just allows to move to the next test case. Opening it with any Browses gives a reasonable content.
Comment 6 Build Bot 2014-02-17 08:31:44 PST
Comment on attachment 224364 [details]
Took Ryosuke's comments into account

Attachment 224364 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6153928395718656

New failing tests:
editing/spelling/inline-spelling-markers.html
Comment 7 Build Bot 2014-02-17 08:31:46 PST
Created attachment 224373 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 8 Build Bot 2014-02-17 09:33:32 PST
Comment on attachment 224364 [details]
Took Ryosuke's comments into account

Attachment 224364 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6054065842683904

New failing tests:
editing/spelling/inline-spelling-markers.html
Comment 9 Build Bot 2014-02-17 09:33:35 PST
Created attachment 224388 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 10 Grzegorz Czajkowski 2014-02-17 10:30:58 PST
(In reply to comment #6)
> (From update of attachment 224364 [details])
> Attachment 224364 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/6153928395718656
> 
> New failing tests:
> editing/spelling/inline-spelling-markers.html

By chance, I removed 'debug(div.id)' from the test. That's why the result is different.
Comment 11 Grzegorz Czajkowski 2014-02-18 23:58:25 PST
Created attachment 224600 [details]
make Mac bot green
Comment 12 Ryosuke Niwa 2014-02-20 22:56:27 PST
Comment on attachment 224600 [details]
make Mac bot green

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

> LayoutTests/editing/spelling/inline-spelling-markers-hidpi.html:71
> +    shouldBecomeEqual('internals.hasSpellingMarker(8, 4)', 'true', function() { // Verifies 'adlj'.
> +        shouldBecomeEqual('internals.hasSpellingMarker(13, 6)', 'true', function() { // Verifies 'adaasj'.
> +            shouldBecomeEqual('internals.hasSpellingMarker(20, 5)', 'true', function() {  // Verifies 'sdklj'.
> +                verifyGrammarMarkers();

Instead of adding a comment in the code like this, include it in the eval itself so that the expected result doesn't look so cryptic.

> LayoutTests/editing/spelling/inline-spelling-markers-hidpi.html:79
> +        shouldBecomeEqual('internals.hasGrammarMarker(4, 3)', 'true', function() { // Verifies duplicated 'the'.
> +            shouldBecomeEqual('internals.hasGrammarMarker(33, 5)', 'true', function() { // Verifies duplicated 'there'.

Ditto.

> LayoutTests/editing/spelling/inline-spelling-markers.html:86
> +    // Take care of spelling markers first.
> +    shouldBecomeEqual('internals.hasSpellingMarker(8, 4)', 'true', function() { // Verifies 'adlj'.
> +        shouldBecomeEqual('internals.hasSpellingMarker(13, 6)', 'true', function() { // Verifies 'adaasj'.
> +            shouldBecomeEqual('internals.hasSpellingMarker(20, 5)', 'true', function() {  // Verifies 'sdklj'.
> +                verifyGrammarMarkers();
> +            });
> +         });
> +    });
> +
> +    function verifyGrammarMarkers()
> +    {
> +        shouldBecomeEqual('internals.hasGrammarMarker(4, 3)', 'true', function() { // Verifies duplicated 'the'.
> +            shouldBecomeEqual('internals.hasGrammarMarker(33, 5)', 'true', function() { // Verifies duplicated 'there'.
> +                debug("");
> +                done();
> +            });
> +        });
> +    }

Ditto.
Comment 13 Grzegorz Czajkowski 2014-02-25 00:30:48 PST
Created attachment 225125 [details]
apply Ryosuke's comments
Comment 14 Grzegorz Czajkowski 2014-02-25 00:45:50 PST
(In reply to comment #12)
> (From update of attachment 224600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224600&action=review
> 
> > LayoutTests/editing/spelling/inline-spelling-markers-hidpi.html:71
> > +    shouldBecomeEqual('internals.hasSpellingMarker(8, 4)', 'true', function() { // Verifies 'adlj'.
> > +        shouldBecomeEqual('internals.hasSpellingMarker(13, 6)', 'true', function() { // Verifies 'adaasj'.
> > +            shouldBecomeEqual('internals.hasSpellingMarker(20, 5)', 'true', function() {  // Verifies 'sdklj'.
> > +                verifyGrammarMarkers();
> 
> Instead of adding a comment in the code like this, include it in the eval itself so that the expected result doesn't look so cryptic.
> 

I moved those comments into debug() for inline-spelling-markers.html only, hidpi test still contains those comments in html file due to the output of messages produced by shouldBecome* covers whole visible view and as a result input elements are placed outside so that pixel tests do no catch them. 

Feel free to reject commit-queue if we should keep those messages in hidpi. One option that comes to mind is to scroll the page to desired position. I
Comment 15 WebKit Commit Bot 2014-02-25 05:26:21 PST
Comment on attachment 225125 [details]
apply Ryosuke's comments

Clearing flags on attachment: 225125

Committed r164643: <http://trac.webkit.org/changeset/164643>
Comment 16 WebKit Commit Bot 2014-02-25 05:26:25 PST
All reviewed patches have been landed.  Closing bug.