Verify misspellings after lines merge/split asynchronously

Verify misspellings after lines merge/split asynchronously

Adapt the following spelling tests to use asynchronous spellchecking:
    - editing/spelling/spelling-backspace-between-lines.html
    - editing/spelling/spelling-linebreak.html

Group test cases into separated methods. Use shouldBeEqualToString to
compare strings. Add newly created div elements instead of overriding
the previous one in order that non DumpRenderTree users can see what
has been tested.

It's based on WebKit changeset: http://trac.webkit.org/changeset/163615

BUG=295722
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169222

[groby-ooo-7-16, 2014-03-12 16:37:35]

LGTM - and if I haven't said that yet, thank you for all the cleanup work!

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
File
LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js
(right):

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js:15:
var div = document.createElement("div");
Just out of curiosity: Why split this into "proper" DOM building + innerHTML?

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js:33:
window.sel = setup("target1"); // ^OK
Nit:"Why call this "target1"? Does it make sense to use something a bit more
descriptive? Maybe "two-lines", "merged-lines",  and "merged-and-forward-delete"

It makes the expectations just a tiny bit more readable.

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js:57:
if (window.internals)
I'd prefer moving that to the top of the file, as in

if (!window.internals) {
  debug("This test only runs on DumpRenderTree, since it requires
window.internals");
  finishJSTest();
}

That cleans up all the acces to window.internals.* throughout the test - no if,
no else done();

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js:92:
if (window.internals)
Why would that be gated on window.internals?

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
File LayoutTests/editing/spelling/spelling-linebreak.html (right):

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/spelling-linebreak.html:21: }

again, probably just abort on missing internals. (Or am I missing a case where
this needs to load and execute outside of DRT?)

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/spelling-linebreak.html:60: <!-- wee need leading
"Hello," to preserve the marker even after setPosition() -->
Nit: "we". I know it's not yours, but while you're here... :)

[tony, 2014-03-12 16:42:01]

LGTM2

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
File
LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js
(right):

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js:92:
if (window.internals)
On 2014/03/12 16:37:36, groby wrote:
> Why would that be gated on window.internals?

It's to avoid dumping the output in the -expected.txt file.

[groby-ooo-7-16, 2014-03-12 16:44:30]

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
File
LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js
(right):

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js:92:
if (window.internals)
On 2014/03/12 16:42:01, tony wrote:
> On 2014/03/12 16:37:36, groby wrote:
> > Why would that be gated on window.internals?
> 
> It's to avoid dumping the output in the -expected.txt file.

So I take it we run these tests outside of DRT to visually inspect, too?
Otherwise, we could always set to "none", without check, right?

[tony, 2014-03-12 16:49:41]

On 2014/03/12 16:44:30, groby wrote:
>
https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
> File
> LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js
> (right):
> 
>
https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
>
LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js:92:
> if (window.internals)
> On 2014/03/12 16:42:01, tony wrote:
> > On 2014/03/12 16:37:36, groby wrote:
> > > Why would that be gated on window.internals?
> > 
> > It's to avoid dumping the output in the -expected.txt file.
> 
> So I take it we run these tests outside of DRT to visually inspect, too?
> Otherwise, we could always set to "none", without check, right?

Yes, that's right.  It's to make it easier to debug in content_shell.

[grzegorz, 2014-03-12 21:33:08]

Thanks for review!

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
File
LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js
(right):

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js:15:
var div = document.createElement("div");
On 2014/03/12 16:37:36, groby wrote:
> Just out of curiosity: Why split this into "proper" DOM building + innerHTML?

What I wanted to achieve here by creating separated 'test element' was to add
some bits for non DRT. IMHO it's fine if we can run those tests in browser. As a
result, we will see the same behavior in browser as the test does. Otherwise, we
will end up seeing the last element only (the previous were overwritten).

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js:33:
window.sel = setup("target1"); // ^OK
On 2014/03/12 16:37:36, groby wrote:
> Nit:"Why call this "target1"? Does it make sense to use something a bit more
> descriptive? Maybe "two-lines", "merged-lines",  and
"merged-and-forward-delete"
> 
> It makes the expectations just a tiny bit more readable.

Thanks. Will do that.

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js:57:
if (window.internals)
On 2014/03/12 16:37:36, groby wrote:
> I'd prefer moving that to the top of the file, as in
> 
> if (!window.internals) {
>   debug("This test only runs on DumpRenderTree, since it requires
> window.internals");
>   finishJSTest();
> }
> 
> That cleans up all the acces to window.internals.* throughout the test - no
if,
> no else done();

Actually, shouldBecome* helpers produces awful output when it calls any internal
method - assuming run outside DRT.

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
File LayoutTests/editing/spelling/spelling-linebreak.html (right):

https://codereview.chromium.org/197403002/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/spelling-linebreak.html:60: <!-- wee need leading
"Hello," to preserve the marker even after setPosition() -->
On 2014/03/12 16:37:36, groby wrote:
> Nit: "we". I know it's not yours, but while you're here... :)
Sure, I'll fix that as well.

[grzegorz, 2014-03-14 07:08:10]

On 2014/03/12 13:46:05, grzegorz wrote:

I am not sure whether I can commit the new patch set since the previous one has
been reviewed. The new patch contains minor changes so let me try.

[grzegorz, 2014-03-14 07:08:21]

The CQ bit was checked by g.czajkowski@samsung.com

[commit-bot: I haz the power, 2014-03-14 07:08:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/g.czajkowski@samsung.com/197403002/30001

[commit-bot: I haz the power, 2014-03-14 08:10:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-14 08:10:48]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[grzegorz, 2014-03-14 08:30:21]

The CQ bit was checked by g.czajkowski@samsung.com

[commit-bot: I haz the power, 2014-03-14 08:30:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/g.czajkowski@samsung.com/197403002/30001

[commit-bot: I haz the power, 2014-03-14 08:48:56]

Change committed as 169222