Bug 128526

Summary: Use asynchronous spellchecking in spelling-hasspellingmarker.js
Product: WebKit Reporter: Grzegorz Czajkowski <g.czajkowski>
Component: HTML EditingAssignee: Grzegorz Czajkowski <g.czajkowski>
Status: RESOLVED FIXED    
Severity: Normal CC: rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 125689    
Attachments:
Description Flags
Patch
none
took into account Ryosuke's comments rniwa: review+

Description Grzegorz Czajkowski 2014-02-10 04:28:53 PST
Refactoring spelling-hasspellingmarker.js to use asynchronous spellchecking.
Comment 1 Grzegorz Czajkowski 2014-02-10 04:31:10 PST
Created attachment 223695 [details]
Patch
Comment 2 Ryosuke Niwa 2014-02-10 17:39:26 PST
Comment on attachment 223695 [details]
Patch

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

> LayoutTests/editing/spelling/script-tests/spelling-hasspellingmarker.js:40
> +    if (window.internals)
> +        shouldBecomeEqual("internals.hasSpellingMarker(0, 2)", isMisspelled ? "true" : "false", done);
> +    else
> +        done();

Why are we calling done in the case window.internals is not defined?

> LayoutTests/editing/spelling/script-tests/spelling-hasspellingmarker.js:47
> +var tests = [ function() { verifySpellingMarkers("<textarea></textarea>", true); },
> +              function() { verifySpellingMarkers("<input type='text'></input>", true); },
> +              function() { verifySpellingMarkers("<div contenteditable='true'></div>", true); },
> +              function() { verifySpellingMarkers("<div contentEditable='true' spellcheck='false'></div>", false); }
> +            ];

Wrong indentations. The subsequent line should be indented by exactly 4 spaces.
It also seems that these could be simply markup & boolean objects as in:
{containerMarkup: "<textarea></textarea>", isMisspelled: true}

> LayoutTests/editing/spelling/script-tests/spelling-hasspellingmarker.js:53
> +        return window.setTimeout(next, 0);

We don't need "window."
Comment 3 Grzegorz Czajkowski 2014-02-10 23:29:50 PST
(In reply to comment #2)
> (From update of attachment 223695 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223695&action=review
> 
> > LayoutTests/editing/spelling/script-tests/spelling-hasspellingmarker.js:40
> > +    if (window.internals)
> > +        shouldBecomeEqual("internals.hasSpellingMarker(0, 2)", isMisspelled ? "true" : "false", done);
> > +    else
> > +        done();
> 
> Why are we calling done in the case window.internals is not defined?

I added it here to just satisfy non-DumpRenderTree users as done() invokes next test case that is defined in 'var tests = [ function(), function() ... ]'. Without it, Browser wouldn't create desired element for given markup. Maybe it could be better to first create such elements and then call verifySpellingMarkers() with early return when internals are not available?

> 
> > LayoutTests/editing/spelling/script-tests/spelling-hasspellingmarker.js:47
> > +var tests = [ function() { verifySpellingMarkers("<textarea></textarea>", true); },
> > +              function() { verifySpellingMarkers("<input type='text'></input>", true); },
> > +              function() { verifySpellingMarkers("<div contenteditable='true'></div>", true); },
> > +              function() { verifySpellingMarkers("<div contentEditable='true' spellcheck='false'></div>", false); }
> > +            ];
> 
> Wrong indentations. The subsequent line should be indented by exactly 4 spaces.

Right. I'll fix them.

> It also seems that these could be simply markup & boolean objects as in:
> {containerMarkup: "<textarea></textarea>", isMisspelled: true}
> 
> > LayoutTests/editing/spelling/script-tests/spelling-hasspellingmarker.js:53
> > +        return window.setTimeout(next, 0);
> 
> We don't need "window."

Ok.
Comment 4 Grzegorz Czajkowski 2014-02-11 04:35:28 PST
Created attachment 223842 [details]
took into account Ryosuke's comments
Comment 5 Grzegorz Czajkowski 2014-02-14 04:22:36 PST
Committed r164100: <http://trac.webkit.org/changeset/164100>