Bug 128526 - Use asynchronous spellchecking in spelling-hasspellingmarker.js
Summary: Use asynchronous spellchecking in spelling-hasspellingmarker.js
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-02-10 04:28 PST by Grzegorz Czajkowski
Modified: 2014-02-14 04:22 PST (History)
1 user (show)

See Also:


Attachments
Patch (6.32 KB, patch)
2014-02-10 04:31 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
took into account Ryosuke's comments (6.40 KB, patch)
2014-02-11 04:35 PST, Grzegorz Czajkowski
rniwa: review+
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-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>