Add a switch to TestRunner to enable/disable mock spell checker

Add a switch to TestRunner to enable/disable mock spell checker

Most layout tests are not related to spell checker, and we do not want
to see failures due to appearance/disappearance of spelling markers in
their pixel results.

This patch adds a switch to TestRunner (more specifically, its
SpellCheckClient) so that the mock spell checker returns non-empty result
only when the switch is on. It is off by default, and hence, spelling
markers do not appear in unrelated tests. Spell checking tests are modified
to explicitly set this switch on so their results remain the same as before.

The following tests are rebaselined to remove the spelling markers from their
pixel results:
- compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- editing/execCommand/5569741.html
- editing/inserting/5418891.html
- editing/inserting/insert-div-027.html
- editing/inserting/paragraph-separator-01.html
- editing/inserting/paragraph-separator-02.html
- editing/inserting/paragraph-separator-03.html
- editing/inserting/return-key-with-selection-003.html
- editing/pasteboard/cut-text-001.html
- editing/pasteboard/undoable-fragment-removes.html
- editing/selection/move-backwords-by-word-001.html
- editing/selection/move-by-word-001.html
- editing/style/block-styles-007.html
- fast/dom/blur-contenteditable.html
- fast/dom/focus-contenteditable.html
- fast/forms/text/input-double-click-selection-gap-bug.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
- virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=639213
Committed: https://crrev.com/2fa3e76400d5f0fcc39eea19f9818599a3ea5016
Cr-Commit-Position: refs/heads/master@{#414042}

[Xiaocheng, 2016-08-24 06:00:53]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 06:01:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2016-08-24 06:16:13]

Description was changed from

==========
Add a switch to TestRunner controlling it's mock spell checker

BUG=
==========

to

==========
WIP Add a switch to TestRunner to enable/disable mock spell checker

Most layout tests are not related to spell checker, and we do not want
to see failures due to appearance/disappearance of spelling markers in
their pixel results.

This patch adds a switch to TestRunner (more specifically, its
SpellCheckClient) so that the mock spell checker returns non-empty result
only when the switch is on.

Spell checking tests are modified to explicitly set this switch on so
their results remain the same as before. Some other tests are rebaselined
to remove the spelling markers from their pixel results.

BUG=639213
==========

[Xiaocheng, 2016-08-24 06:16:13]

xiaochengh@chromium.org changed reviewers:
+ yosin@chromium.org

[Xiaocheng, 2016-08-24 06:17:13]

What do you think about this idea? It reuses existing code and still keeps spell
checking code paths tested in non-spell-checking tests.

[yosin_UTC9, 2016-08-24 06:32:23]

On 2016/08/24 at 06:17:13, xiaochengh wrote:
> What do you think about this idea? It reuses existing code and still keeps
spell checking code paths tested in non-spell-checking tests.
Do we abandon to use misspell word list?

It can be:

if (window.testRunner)
  testRunner.addMisspellWords(['foo', 'bar']);

I think having fixed string in .cc isn't beautiful and it takes time to find
misspell word from git repository.

[Xiaocheng, 2016-08-24 06:45:50]

On 2016/08/24 at 06:32:23, yosin wrote:
> On 2016/08/24 at 06:17:13, xiaochengh wrote:
> > What do you think about this idea? It reuses existing code and still keeps
spell checking code paths tested in non-spell-checking tests.
> Do we abandon to use misspell word list?
> 
> It can be:
> 
> if (window.testRunner)
>   testRunner.addMisspellWords(['foo', 'bar']);
> 
> I think having fixed string in .cc isn't beautiful and it takes time to find
misspell word from git repository.

I don't see the necessity of doing a big refactoring to mock spell checker,
given that this approach already works well.

Could you explain what "it takes time to find misspell word from git repository"
means?

[Xiaocheng, 2016-08-24 07:06:25]

Description was changed from

==========
WIP Add a switch to TestRunner to enable/disable mock spell checker

Most layout tests are not related to spell checker, and we do not want
to see failures due to appearance/disappearance of spelling markers in
their pixel results.

This patch adds a switch to TestRunner (more specifically, its
SpellCheckClient) so that the mock spell checker returns non-empty result
only when the switch is on.

Spell checking tests are modified to explicitly set this switch on so
their results remain the same as before. Some other tests are rebaselined
to remove the spelling markers from their pixel results.

BUG=639213
==========

to

==========
WIP Add a switch to TestRunner to enable/disable mock spell checker

Most layout tests are not related to spell checker, and we do not want
to see failures due to appearance/disappearance of spelling markers in
their pixel results.

This patch adds a switch to TestRunner (more specifically, its
SpellCheckClient) so that the mock spell checker returns non-empty result
only when the switch is on.

Spell checking tests are modified to explicitly set this switch on so
their results remain the same as before. Some other tests are rebaselined
to remove the spelling markers from their pixel results:
-
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- editing/execCommand/5569741.html
- editing/inserting/5418891.html
- editing/inserting/insert-div-027.html
- editing/inserting/paragraph-separator-01.html
- editing/inserting/paragraph-separator-02.html
- editing/inserting/paragraph-separator-03.html
- editing/inserting/return-key-with-selection-003.html
- editing/pasteboard/cut-text-001.html
- editing/pasteboard/undoable-fragment-removes.html
- editing/selection/move-backwords-by-word-001.html
- editing/selection/move-by-word-001.html
- editing/style/block-styles-007.html
- fast/dom/blur-contenteditable.html
- fast/dom/focus-contenteditable.html
- fast/forms/text/input-double-click-selection-gap-bug.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
-
virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=639213
==========

[yosin_UTC9, 2016-08-24 07:10:02]

On 2016/08/24 at 06:45:50, xiaochengh wrote:
> On 2016/08/24 at 06:32:23, yosin wrote:
> > On 2016/08/24 at 06:17:13, xiaochengh wrote:
> > > What do you think about this idea? It reuses existing code and still keeps
spell checking code paths tested in non-spell-checking tests.
> > Do we abandon to use misspell word list?
> > 
> > It can be:
> > 
> > if (window.testRunner)
> >   testRunner.addMisspellWords(['foo', 'bar']);
> > 
> > I think having fixed string in .cc isn't beautiful and it takes time to find
misspell word from git repository.
> 
> I don't see the necessity of doing a big refactoring to mock spell checker,
given that this approach already works well.
> 
> Could you explain what "it takes time to find misspell word from git
repository" means?

When I write a test for spell checker and using misspell word "xyz", and I see
there is no read wave under "xyz".
Then I should check whether my change is valid or not. After I confirm my test
is valid. I think why "xyz" doesn't
have red wave line. I start looking spell check engine then find
MockSpellChecker with misspell word list.

[Xiaocheng, 2016-08-24 07:18:56]

On 2016/08/24 at 07:10:02, yosin wrote:
> On 2016/08/24 at 06:45:50, xiaochengh wrote:
> > On 2016/08/24 at 06:32:23, yosin wrote:
> > > On 2016/08/24 at 06:17:13, xiaochengh wrote:
> > > > What do you think about this idea? It reuses existing code and still
keeps spell checking code paths tested in non-spell-checking tests.
> > > Do we abandon to use misspell word list?
> > > 
> > > It can be:
> > > 
> > > if (window.testRunner)
> > >   testRunner.addMisspellWords(['foo', 'bar']);
> > > 
> > > I think having fixed string in .cc isn't beautiful and it takes time to
find misspell word from git repository.
> > 
> > I don't see the necessity of doing a big refactoring to mock spell checker,
given that this approach already works well.
> > 
> > Could you explain what "it takes time to find misspell word from git
repository" means?
> 
> When I write a test for spell checker and using misspell word "xyz", and I see
there is no read wave under "xyz".
> Then I should check whether my change is valid or not. After I confirm my test
is valid. I think why "xyz" doesn't
> have red wave line. I start looking spell check engine then find
MockSpellChecker with misspell word list.

I see. Then there are kind of two separate issues:

(1) The current design is not friendly to new developers writing spell checking
tests.

(2) Pixel results of unrelated tests are affected by spelling markers.

This patch only aims at (2), while your approach solves both of them but
requires a much larger CL.

I don't want to spend the rest of the week (or perhaps month) on tinkering the
test runner. How about just file a new bug about (1) and let this CL simply
fixes (2)?

[commit-bot: I haz the power, 2016-08-24 07:26:24]

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

[commit-bot: I haz the power, 2016-08-24 07:26:25]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xiaocheng, 2016-08-24 07:29:42]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 07:30:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2016-08-24 07:52:36]

Description was changed from

==========
WIP Add a switch to TestRunner to enable/disable mock spell checker

Most layout tests are not related to spell checker, and we do not want
to see failures due to appearance/disappearance of spelling markers in
their pixel results.

This patch adds a switch to TestRunner (more specifically, its
SpellCheckClient) so that the mock spell checker returns non-empty result
only when the switch is on.

Spell checking tests are modified to explicitly set this switch on so
their results remain the same as before. Some other tests are rebaselined
to remove the spelling markers from their pixel results:
-
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- editing/execCommand/5569741.html
- editing/inserting/5418891.html
- editing/inserting/insert-div-027.html
- editing/inserting/paragraph-separator-01.html
- editing/inserting/paragraph-separator-02.html
- editing/inserting/paragraph-separator-03.html
- editing/inserting/return-key-with-selection-003.html
- editing/pasteboard/cut-text-001.html
- editing/pasteboard/undoable-fragment-removes.html
- editing/selection/move-backwords-by-word-001.html
- editing/selection/move-by-word-001.html
- editing/style/block-styles-007.html
- fast/dom/blur-contenteditable.html
- fast/dom/focus-contenteditable.html
- fast/forms/text/input-double-click-selection-gap-bug.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
-
virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=639213
==========

to

==========
Add a switch to TestRunner to enable/disable mock spell checker

Most layout tests are not related to spell checker, and we do not want
to see failures due to appearance/disappearance of spelling markers in
their pixel results.

This patch adds a switch to TestRunner (more specifically, its
SpellCheckClient) so that the mock spell checker returns non-empty result
only when the switch is on. It is off by default, and hence, spelling
markers do not appear in unrelated tests. Spell checking tests are modified
to explicitly set this switch on so their results remain the same as before.

The following tests are rebaselined to remove the spelling markers from their
pixel results:
-
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- editing/execCommand/5569741.html
- editing/inserting/5418891.html
- editing/inserting/insert-div-027.html
- editing/inserting/paragraph-separator-01.html
- editing/inserting/paragraph-separator-02.html
- editing/inserting/paragraph-separator-03.html
- editing/inserting/return-key-with-selection-003.html
- editing/pasteboard/cut-text-001.html
- editing/pasteboard/undoable-fragment-removes.html
- editing/selection/move-backwords-by-word-001.html
- editing/selection/move-by-word-001.html
- editing/style/block-styles-007.html
- fast/dom/blur-contenteditable.html
- fast/dom/focus-contenteditable.html
- fast/forms/text/input-double-click-selection-gap-bug.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
-
virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=639213
==========

[yosin_UTC9, 2016-08-24 08:00:38]

lgtm

But, I'm not fan of switch. :-<

https://codereview.chromium.org/2270293003/diff/20001/components/test_runner/...
File components/test_runner/spell_check_client.cc (right):

https://codereview.chromium.org/2270293003/diff/20001/components/test_runner/...
components/test_runner/spell_check_client.cc:23: : enabled_(false),
nit: let's initialize |enabled_| in class declaration.

https://codereview.chromium.org/2270293003/diff/20001/components/test_runner/...
File components/test_runner/spell_check_client.h (right):

https://codereview.chromium.org/2270293003/diff/20001/components/test_runner/...
components/test_runner/spell_check_client.h:53: bool enabled_;
s/enabled_;/enabled_ = false;/

[Xiaocheng, 2016-08-24 08:23:34]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 08:23:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2016-08-24 08:28:22]

xiaochengh@chromium.org changed reviewers:
+ tkent@chromium.org

[Xiaocheng, 2016-08-24 08:28:23]

+tkent for owner's review

PTAL.

https://codereview.chromium.org/2270293003/diff/20001/components/test_runner/...
File components/test_runner/spell_check_client.cc (right):

https://codereview.chromium.org/2270293003/diff/20001/components/test_runner/...
components/test_runner/spell_check_client.cc:23: : enabled_(false),
On 2016/08/24 at 08:00:38, Yosi_UTC9 wrote:
> nit: let's initialize |enabled_| in class declaration.

Done.

https://codereview.chromium.org/2270293003/diff/20001/components/test_runner/...
File components/test_runner/spell_check_client.h (right):

https://codereview.chromium.org/2270293003/diff/20001/components/test_runner/...
components/test_runner/spell_check_client.h:53: bool enabled_;
On 2016/08/24 at 08:00:38, Yosi_UTC9 wrote:
> s/enabled_;/enabled_ = false;/

Done.

[tkent, 2016-08-24 08:45:54]

lgtm

[commit-bot: I haz the power, 2016-08-24 11:15:50]

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

[commit-bot: I haz the power, 2016-08-24 11:15:51]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2016-08-24 11:20:13]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2016-08-24 11:20:13]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2270293003/#ps40001
(title: "Use in-class initialization")

[commit-bot: I haz the power, 2016-08-24 11:20:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 11:27:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-24 11:29:19]

Description was changed from

==========
Add a switch to TestRunner to enable/disable mock spell checker

Most layout tests are not related to spell checker, and we do not want
to see failures due to appearance/disappearance of spelling markers in
their pixel results.

This patch adds a switch to TestRunner (more specifically, its
SpellCheckClient) so that the mock spell checker returns non-empty result
only when the switch is on. It is off by default, and hence, spelling
markers do not appear in unrelated tests. Spell checking tests are modified
to explicitly set this switch on so their results remain the same as before.

The following tests are rebaselined to remove the spelling markers from their
pixel results:
-
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- editing/execCommand/5569741.html
- editing/inserting/5418891.html
- editing/inserting/insert-div-027.html
- editing/inserting/paragraph-separator-01.html
- editing/inserting/paragraph-separator-02.html
- editing/inserting/paragraph-separator-03.html
- editing/inserting/return-key-with-selection-003.html
- editing/pasteboard/cut-text-001.html
- editing/pasteboard/undoable-fragment-removes.html
- editing/selection/move-backwords-by-word-001.html
- editing/selection/move-by-word-001.html
- editing/style/block-styles-007.html
- fast/dom/blur-contenteditable.html
- fast/dom/focus-contenteditable.html
- fast/forms/text/input-double-click-selection-gap-bug.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
-
virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=639213
==========

to

==========
Add a switch to TestRunner to enable/disable mock spell checker

Most layout tests are not related to spell checker, and we do not want
to see failures due to appearance/disappearance of spelling markers in
their pixel results.

This patch adds a switch to TestRunner (more specifically, its
SpellCheckClient) so that the mock spell checker returns non-empty result
only when the switch is on. It is off by default, and hence, spelling
markers do not appear in unrelated tests. Spell checking tests are modified
to explicitly set this switch on so their results remain the same as before.

The following tests are rebaselined to remove the spelling markers from their
pixel results:
-
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- editing/execCommand/5569741.html
- editing/inserting/5418891.html
- editing/inserting/insert-div-027.html
- editing/inserting/paragraph-separator-01.html
- editing/inserting/paragraph-separator-02.html
- editing/inserting/paragraph-separator-03.html
- editing/inserting/return-key-with-selection-003.html
- editing/pasteboard/cut-text-001.html
- editing/pasteboard/undoable-fragment-removes.html
- editing/selection/move-backwords-by-word-001.html
- editing/selection/move-by-word-001.html
- editing/style/block-styles-007.html
- fast/dom/blur-contenteditable.html
- fast/dom/focus-contenteditable.html
- fast/forms/text/input-double-click-selection-gap-bug.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
-
virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=639213

Committed: https://crrev.com/2fa3e76400d5f0fcc39eea19f9818599a3ea5016
Cr-Commit-Position: refs/heads/master@{#414042}
==========

[commit-bot: I haz the power, 2016-08-24 11:29:20]

Patchset 3 (id:??) landed as
https://crrev.com/2fa3e76400d5f0fcc39eea19f9818599a3ea5016
Cr-Commit-Position: refs/heads/master@{#414042}