Componentize spellcheck [3]: move renderer/ files to component.

Componentize spellcheck [3]: move renderer/ files to component.

Move all chrome/renderer spellcheck files and tests to component,
update dependencies and build files. Also address some compile
and unittest issues, e.g. make sure to properly initialize blink
in tests to avoid crashes.

The main motivation behind the componentization of the spellcheck
feature is that we want to make it available in Android WebView.

Preceding 'componentize' patches:
[1] https://codereview.chromium.org/2166683003/
[2] https://codereview.chromium.org/2177343002/

BUG=583616, 629609
Committed: https://crrev.com/7ef691f4cdae52d0171ee1abafe33b5df8c09dd3
Cr-Commit-Position: refs/heads/master@{#411469}

[timvolodine, 2016-08-01 17:05:15]

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

[commit-bot: I haz the power, 2016-08-01 17:05:33]

Dry run: 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-01 17:09:10]

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

[commit-bot: I haz the power, 2016-08-01 17:09:11]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[timvolodine, 2016-08-01 17:25:51]

Description was changed from

==========
Componentize spellcheck [3]: move renderer/ files to component.

Move chrome/renderer spellcheck files to component, update dependencies
and build files.

The main motivation behind the componentization of the
spellcheck feature is that we want to make it available
in Android WebView.

Preceding 'componentize' patches:
[1] https://codereview.chromium.org/2166683003/
[2] https://codereview.chromium.org/2177343002/

Note:
Skipping presubmit due to warnings about UMA_HISTOGRAM because
the relevant histogram names are not public and live in google3.

BUG=583616, 629609
==========

to

==========
Componentize spellcheck [3]: move renderer/ files to component.

Move chrome/renderer spellcheck files to component, update dependencies
and build files.

The main motivation behind the componentization of the
spellcheck feature is that we want to make it available
in Android WebView.

Preceding 'componentize' patches:
[1] https://codereview.chromium.org/2166683003/
[2] https://codereview.chromium.org/2177343002/

BUG=583616, 629609
==========

[timvolodine, 2016-08-01 17:28:08]

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

[commit-bot: I haz the power, 2016-08-01 17:28:27]

Dry run: 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-01 17:32:59]

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

[commit-bot: I haz the power, 2016-08-01 17:33:00]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[timvolodine, 2016-08-01 18:14:36]

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

[commit-bot: I haz the power, 2016-08-01 18:15:14]

Dry run: 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-01 18:26:21]

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

[commit-bot: I haz the power, 2016-08-01 18:26:22]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[timvolodine, 2016-08-01 19:24:09]

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

[commit-bot: I haz the power, 2016-08-01 19:24:40]

Dry run: 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-01 19:34:43]

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

[commit-bot: I haz the power, 2016-08-01 19:34:43]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[timvolodine, 2016-08-02 13:54:19]

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

[commit-bot: I haz the power, 2016-08-02 13:54:34]

Dry run: 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-02 14:26:12]

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

[commit-bot: I haz the power, 2016-08-02 14:26:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[timvolodine, 2016-08-02 15:32:49]

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

[commit-bot: I haz the power, 2016-08-02 15:33:11]

Dry run: 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-02 16:48:21]

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

[commit-bot: I haz the power, 2016-08-02 16:48:22]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2016-08-02 16:49:39]

Description was changed from

==========
Componentize spellcheck [3]: move renderer/ files to component.

Move chrome/renderer spellcheck files to component, update dependencies
and build files.

The main motivation behind the componentization of the
spellcheck feature is that we want to make it available
in Android WebView.

Preceding 'componentize' patches:
[1] https://codereview.chromium.org/2166683003/
[2] https://codereview.chromium.org/2177343002/

BUG=583616, 629609
==========

to

==========
Componentize spellcheck [3]: move renderer/ files to component.

Move chrome/renderer spellcheck files to component, update
dependencies and build files. Also addresses some compile
and unittest issues, e.g. makes sure to properly initialize
blink in tests to avoid crashes.

The main motivation behind the componentization of the
spellcheck feature is that we want to make it available
in Android WebView.

Preceding 'componentize' patches:
[1] https://codereview.chromium.org/2166683003/
[2] https://codereview.chromium.org/2177343002/

BUG=583616, 629609
==========

[timvolodine, 2016-08-02 16:53:21]

Note: presubmit complains about UMA_HISTOGRAM names not matching with
histograms.xml (like SpellCheck.api.check.suggestions). I assume this is because
these histogram names are not public and live in google3. Maybe we should
consider moving them to chromium at some stage..

[timvolodine, 2016-08-02 16:55:36]

timvolodine@chromium.org changed reviewers:
+ groby@chromium.org, rouslan@chromium.org

[timvolodine, 2016-08-02 16:55:37]

please take a look

[timvolodine, 2016-08-02 16:57:27]

Description was changed from

==========
Componentize spellcheck [3]: move renderer/ files to component.

Move chrome/renderer spellcheck files to component, update
dependencies and build files. Also addresses some compile
and unittest issues, e.g. makes sure to properly initialize
blink in tests to avoid crashes.

The main motivation behind the componentization of the
spellcheck feature is that we want to make it available
in Android WebView.

Preceding 'componentize' patches:
[1] https://codereview.chromium.org/2166683003/
[2] https://codereview.chromium.org/2177343002/

BUG=583616, 629609
==========

to

==========
Componentize spellcheck [3]: move renderer/ files to component.

Move all chrome/renderer spellcheck files and tests to component,
update dependencies and build files. Also address some compile
and unittest issues, e.g. make sure to properly initialize blink
in tests to avoid crashes.

The main motivation behind the componentization of the spellcheck
feature is that we want to make it available in Android WebView.

Preceding 'componentize' patches:
[1] https://codereview.chromium.org/2166683003/
[2] https://codereview.chromium.org/2177343002/

BUG=583616, 629609
==========

[timvolodine, 2016-08-03 12:46:18]

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

[commit-bot: I haz the power, 2016-08-03 12:46:32]

Dry run: 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-03 13:38:39]

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

[commit-bot: I haz the power, 2016-08-03 13:38:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[groby-ooo-7-16, 2016-08-03 17:31:54]

Mostly LG, except for the Blink question

https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
File components/spellcheck/renderer/spellcheck_multilingual_unittest.cc (right):

https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
components/spellcheck/renderer/spellcheck_multilingual_unittest.cc:50:
MultilingualSpellCheckTest() { test_runner::EnsureBlinkInitialized(); }
Why does this need blink initialized?

https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
File components/spellcheck/renderer/spellcheck_provider_test.cc (right):

https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
components/spellcheck/renderer/spellcheck_provider_test.cc:100:
test_runner::EnsureBlinkInitialized();
Same question - technically, all the tests should run independent of blink.
(They use some blink structures, but shouldn't use any functionality)

https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
File components/spellcheck/renderer/spellcheck_unittest.cc (right):

https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
components/spellcheck/renderer/spellcheck_unittest.cc:53:
test_runner::EnsureBlinkInitialized();
See above :)

[timvolodine, 2016-08-03 19:12:30]

thanks groby@, answers below

https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
File components/spellcheck/renderer/spellcheck_multilingual_unittest.cc (right):

https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
components/spellcheck/renderer/spellcheck_multilingual_unittest.cc:50:
MultilingualSpellCheckTest() { test_runner::EnsureBlinkInitialized(); }
On 2016/08/03 17:31:54, groby wrote:
> Why does this need blink initialized?

if I don't do that I get a crash stack with something like:
[ RUN      ] SpellCheckTest.CreateTextCheckingResultsAddsGrammarMarkers
ASSERTION FAILED: s_initialized
../../third_party/WebKit/Source/wtf/allocator/Partitions.h(53) : static
WTF::PartitionRootGeneric *WTF::Partitions::bufferPartition()

(this is why many bots in patch #5 were red.)

it looks like the explicit init is typically needed for tests in components, we
have a couple of component tests doing a similar thing:
https://cs.chromium.org/search/?q=%22test_runner::EnsureBlinkInitialized();%2...

https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
File components/spellcheck/renderer/spellcheck_provider_test.cc (right):

https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
components/spellcheck/renderer/spellcheck_provider_test.cc:100:
test_runner::EnsureBlinkInitialized();
On 2016/08/03 17:31:54, groby wrote:
> Same question - technically, all the tests should run independent of blink.
> (They use some blink structures, but shouldn't use any functionality)

see previous answer

https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
File components/spellcheck/renderer/spellcheck_unittest.cc (right):

https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
components/spellcheck/renderer/spellcheck_unittest.cc:53:
test_runner::EnsureBlinkInitialized();
On 2016/08/03 17:31:54, groby wrote:
> See above :)

same answer ;)

[groby-ooo-7-16, 2016-08-03 20:37:40]

On 2016/08/03 19:12:30, timvolodine wrote:
> thanks groby@, answers below
> 
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> File components/spellcheck/renderer/spellcheck_multilingual_unittest.cc
(right):
> 
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> components/spellcheck/renderer/spellcheck_multilingual_unittest.cc:50:
> MultilingualSpellCheckTest() { test_runner::EnsureBlinkInitialized(); }
> On 2016/08/03 17:31:54, groby wrote:
> > Why does this need blink initialized?
> 
> if I don't do that I get a crash stack with something like:
> [ RUN      ] SpellCheckTest.CreateTextCheckingResultsAddsGrammarMarkers
> ASSERTION FAILED: s_initialized
> ../../third_party/WebKit/Source/wtf/allocator/Partitions.h(53) : static
> WTF::PartitionRootGeneric *WTF::Partitions::bufferPartition()
> 
> (this is why many bots in patch #5 were red.)
> 
> it looks like the explicit init is typically needed for tests in components,
we
> have a couple of component tests doing a similar thing:
>
https://cs.chromium.org/search/?q=%22test_runner::EnsureBlinkInitialized();%2...
> 
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> File components/spellcheck/renderer/spellcheck_provider_test.cc (right):
> 
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> components/spellcheck/renderer/spellcheck_provider_test.cc:100:
> test_runner::EnsureBlinkInitialized();
> On 2016/08/03 17:31:54, groby wrote:
> > Same question - technically, all the tests should run independent of blink.
> > (They use some blink structures, but shouldn't use any functionality)
> 
> see previous answer
> 
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> File components/spellcheck/renderer/spellcheck_unittest.cc (right):
> 
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> components/spellcheck/renderer/spellcheck_unittest.cc:53:
> test_runner::EnsureBlinkInitialized();
> On 2016/08/03 17:31:54, groby wrote:
> > See above :)
> 
> same answer ;)

In that case, LGTM - but please

a) file a bug around this, because it shouldn't be necessary, and
b) refer to that bug in the files and/or mention the files in the bug. T

This should be cleaned up at some point.

[groby-ooo-7-16, 2016-08-03 20:38:21]

On 2016/08/03 20:37:40, groby wrote:
> On 2016/08/03 19:12:30, timvolodine wrote:
> > thanks groby@, answers below
> > 
> >
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> > File components/spellcheck/renderer/spellcheck_multilingual_unittest.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> > components/spellcheck/renderer/spellcheck_multilingual_unittest.cc:50:
> > MultilingualSpellCheckTest() { test_runner::EnsureBlinkInitialized(); }
> > On 2016/08/03 17:31:54, groby wrote:
> > > Why does this need blink initialized?
> > 
> > if I don't do that I get a crash stack with something like:
> > [ RUN      ] SpellCheckTest.CreateTextCheckingResultsAddsGrammarMarkers
> > ASSERTION FAILED: s_initialized
> > ../../third_party/WebKit/Source/wtf/allocator/Partitions.h(53) : static
> > WTF::PartitionRootGeneric *WTF::Partitions::bufferPartition()
> > 
> > (this is why many bots in patch #5 were red.)
> > 
> > it looks like the explicit init is typically needed for tests in components,
> we
> > have a couple of component tests doing a similar thing:
> >
>
https://cs.chromium.org/search/?q=%22test_runner::EnsureBlinkInitialized();%2...
> > 
> >
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> > File components/spellcheck/renderer/spellcheck_provider_test.cc (right):
> > 
> >
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> > components/spellcheck/renderer/spellcheck_provider_test.cc:100:
> > test_runner::EnsureBlinkInitialized();
> > On 2016/08/03 17:31:54, groby wrote:
> > > Same question - technically, all the tests should run independent of
blink.
> > > (They use some blink structures, but shouldn't use any functionality)
> > 
> > see previous answer
> > 
> >
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> > File components/spellcheck/renderer/spellcheck_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> > components/spellcheck/renderer/spellcheck_unittest.cc:53:
> > test_runner::EnsureBlinkInitialized();
> > On 2016/08/03 17:31:54, groby wrote:
> > > See above :)
> > 
> > same answer ;)
> 
> In that case, LGTM - but please
> 
> a) file a bug around this, because it shouldn't be necessary, and
> b) refer to that bug in the files and/or mention the files in the bug. T
> 
> This should be cleaned up at some point.

And because I forgot to say it - thank you for making this happen. There've been
several aborted attempts to componentize spellcheck, and it's great to see you
making such progress on it!

[timvolodine, 2016-08-04 14:22:30]

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

[commit-bot: I haz the power, 2016-08-04 14:22:55]

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

[timvolodine, 2016-08-04 14:30:14]

On 2016/08/03 20:38:21, groby wrote:
> On 2016/08/03 20:37:40, groby wrote:
> > On 2016/08/03 19:12:30, timvolodine wrote:
> > > thanks groby@, answers below
> > > 
> > >
> >
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> > > File components/spellcheck/renderer/spellcheck_multilingual_unittest.cc
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> > > components/spellcheck/renderer/spellcheck_multilingual_unittest.cc:50:
> > > MultilingualSpellCheckTest() { test_runner::EnsureBlinkInitialized(); }
> > > On 2016/08/03 17:31:54, groby wrote:
> > > > Why does this need blink initialized?
> > > 
> > > if I don't do that I get a crash stack with something like:
> > > [ RUN      ] SpellCheckTest.CreateTextCheckingResultsAddsGrammarMarkers
> > > ASSERTION FAILED: s_initialized
> > > ../../third_party/WebKit/Source/wtf/allocator/Partitions.h(53) : static
> > > WTF::PartitionRootGeneric *WTF::Partitions::bufferPartition()
> > > 
> > > (this is why many bots in patch #5 were red.)
> > > 
> > > it looks like the explicit init is typically needed for tests in
components,
> > we
> > > have a couple of component tests doing a similar thing:
> > >
> >
>
https://cs.chromium.org/search/?q=%22test_runner::EnsureBlinkInitialized();%2...
> > > 
> > >
> >
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> > > File components/spellcheck/renderer/spellcheck_provider_test.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> > > components/spellcheck/renderer/spellcheck_provider_test.cc:100:
> > > test_runner::EnsureBlinkInitialized();
> > > On 2016/08/03 17:31:54, groby wrote:
> > > > Same question - technically, all the tests should run independent of
> blink.
> > > > (They use some blink structures, but shouldn't use any functionality)
> > > 
> > > see previous answer
> > > 
> > >
> >
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> > > File components/spellcheck/renderer/spellcheck_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2198143002/diff/140001/components/spellcheck/...
> > > components/spellcheck/renderer/spellcheck_unittest.cc:53:
> > > test_runner::EnsureBlinkInitialized();
> > > On 2016/08/03 17:31:54, groby wrote:
> > > > See above :)
> > > 
> > > same answer ;)
> > 
> > In that case, LGTM - but please
> > 
> > a) file a bug around this, because it shouldn't be necessary, and
> > b) refer to that bug in the files and/or mention the files in the bug. T

filed crbug.com/634326 regarding this and also added TODO references in the
files.

> > 
> > This should be cleaned up at some point.
> 
> And because I forgot to say it - thank you for making this happen. There've
been
> several aborted attempts to componentize spellcheck, and it's great to see you
> making such progress on it!

Thanks, no problem. It does tend to be a bit more work than I originally
anticipated though ;)
And thank *you* for reviewing this effort!

[timvolodine, 2016-08-04 14:34:27]

timvolodine@chromium.org changed reviewers:
+ jam@chromium.org

[timvolodine, 2016-08-04 14:34:28]

+jam@: for RS of the remaining few bits, in particular

chrome/renderer/BUILD.gn
chrome/renderer/DEPS
chrome/renderer/chrome_content_renderer_client.cc
chrome/test/base/chrome_render_view_test.cc
components/BUILD.gn

[commit-bot: I haz the power, 2016-08-04 15:29:45]

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

[commit-bot: I haz the power, 2016-08-04 15:29:52]

Dry run: This issue passed the CQ dry run.

[jam, 2016-08-04 17:49:54]

On 2016/08/04 14:34:28, timvolodine wrote:
> +jam@: for RS of the remaining few bits, in particular
> 
> chrome/renderer/BUILD.gn
> chrome/renderer/DEPS
> chrome/renderer/chrome_content_renderer_client.cc
> chrome/test/base/chrome_render_view_test.cc
> components/BUILD.gn

these look fine but I see this change adds a dependency from the spellchecker
component to test_runner.

Why is this now needed? Is this because a test that ran under chrome's unit
tests now doesn't get some initialization there?

I don't think we should this extra dependency now, since test_runner was
factored out for mandoline but that's not needed anymore. adding more
dependencies to it will make it harder to move back into content (non-public).

Can you investigate why this wasn't needed in chrome's unit test target, and
follow what it does?

[timvolodine, 2016-08-05 12:26:14]

On 2016/08/04 17:49:54, jam wrote:
> On 2016/08/04 14:34:28, timvolodine wrote:
> > +jam@: for RS of the remaining few bits, in particular
> > 
> > chrome/renderer/BUILD.gn
> > chrome/renderer/DEPS
> > chrome/renderer/chrome_content_renderer_client.cc
> > chrome/test/base/chrome_render_view_test.cc
> > components/BUILD.gn
> 
> these look fine but I see this change adds a dependency from the spellchecker
> component to test_runner.
> 
> Why is this now needed? Is this because a test that ran under chrome's unit
> tests now doesn't get some initialization there?
> 
> I don't think we should this extra dependency now, since test_runner was
> factored out for mandoline but that's not needed anymore. adding more
> dependencies to it will make it harder to move back into content (non-public).
> 
> Can you investigate why this wasn't needed in chrome's unit test target, and
> follow what it does?

Yes this is needed for the test_runner::EnsureBlinkInitialized. It appears other
component tests using blink also do this. I've filed a follow-up
crbug.com/634326 with more details and added todos for this in the relevant
files.

[timvolodine, 2016-08-05 13:09:31]

On 2016/08/05 12:26:14, timvolodine wrote:
> On 2016/08/04 17:49:54, jam wrote:
> > On 2016/08/04 14:34:28, timvolodine wrote:
> > > +jam@: for RS of the remaining few bits, in particular
> > > 
> > > chrome/renderer/BUILD.gn
> > > chrome/renderer/DEPS
> > > chrome/renderer/chrome_content_renderer_client.cc
> > > chrome/test/base/chrome_render_view_test.cc
> > > components/BUILD.gn
> > 
> > these look fine but I see this change adds a dependency from the
spellchecker
> > component to test_runner.
> > 
> > Why is this now needed? Is this because a test that ran under chrome's unit
> > tests now doesn't get some initialization there?
> > 
> > I don't think we should this extra dependency now, since test_runner was
> > factored out for mandoline but that's not needed anymore. adding more
> > dependencies to it will make it harder to move back into content
(non-public).
> > 
> > Can you investigate why this wasn't needed in chrome's unit test target, and
> > follow what it does?
> 
> Yes this is needed for the test_runner::EnsureBlinkInitialized. It appears
other
> component tests using blink also do this. I've filed a follow-up
> crbug.com/634326 with more details and added todos for this in the relevant
> files.

Digging a bit deeper, I think the case with chrome/ is that
chrome/test/base/run_all_unittests.cc uses a UnitTestTestSuite which in turn
initializes blink via TestBlinkWebUnitTestSupport

[timvolodine, 2016-08-08 14:33:40]

jam@: ptal?

[jam, 2016-08-09 15:41:33]

On 2016/08/05 13:09:31, timvolodine wrote:
> On 2016/08/05 12:26:14, timvolodine wrote:
> > On 2016/08/04 17:49:54, jam wrote:
> > > On 2016/08/04 14:34:28, timvolodine wrote:
> > > > +jam@: for RS of the remaining few bits, in particular
> > > > 
> > > > chrome/renderer/BUILD.gn
> > > > chrome/renderer/DEPS
> > > > chrome/renderer/chrome_content_renderer_client.cc
> > > > chrome/test/base/chrome_render_view_test.cc
> > > > components/BUILD.gn
> > > 
> > > these look fine but I see this change adds a dependency from the
> spellchecker
> > > component to test_runner.
> > > 
> > > Why is this now needed? Is this because a test that ran under chrome's
unit
> > > tests now doesn't get some initialization there?
> > > 
> > > I don't think we should this extra dependency now, since test_runner was
> > > factored out for mandoline but that's not needed anymore. adding more
> > > dependencies to it will make it harder to move back into content
> (non-public).
> > > 
> > > Can you investigate why this wasn't needed in chrome's unit test target,
and
> > > follow what it does?
> > 
> > Yes this is needed for the test_runner::EnsureBlinkInitialized. It appears
> other
> > component tests using blink also do this. I've filed a follow-up
> > crbug.com/634326 with more details and added todos for this in the relevant
> > files.
> 
> Digging a bit deeper, I think the case with chrome/ is that
> chrome/test/base/run_all_unittests.cc uses a UnitTestTestSuite which in turn
> initializes blink via TestBlinkWebUnitTestSupport

Ok, let's just make components_unittests do the same thing as chrome to
instantiate this object:

i.e.
https://cs.chromium.org/chromium/src/components/test/run_all_unittests.cc?rcl...
should copy the line from
https://cs.chromium.org/chromium/src/chrome/test/base/run_all_unittests.cc?rc...

then we can remove the other places in components that use test_runner as well.
You can do it in this change or in another cl if you want.

[please use gerrit instead, 2016-08-09 16:49:58]

rouslan@chromium.org changed reviewers:
- rouslan@chromium.org

[timvolodine, 2016-08-09 17:06:44]

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

[commit-bot: I haz the power, 2016-08-09 17:07:34]

Dry run: 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-09 17:18:18]

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

[commit-bot: I haz the power, 2016-08-09 17:18:20]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[timvolodine, 2016-08-09 18:12:35]

On 2016/08/09 15:41:33, jam wrote:
> On 2016/08/05 13:09:31, timvolodine wrote:
> > On 2016/08/05 12:26:14, timvolodine wrote:
> > > On 2016/08/04 17:49:54, jam wrote:
> > > > On 2016/08/04 14:34:28, timvolodine wrote:
> > > > > +jam@: for RS of the remaining few bits, in particular
> > > > > 
> > > > > chrome/renderer/BUILD.gn
> > > > > chrome/renderer/DEPS
> > > > > chrome/renderer/chrome_content_renderer_client.cc
> > > > > chrome/test/base/chrome_render_view_test.cc
> > > > > components/BUILD.gn
> > > > 
> > > > these look fine but I see this change adds a dependency from the
> > spellchecker
> > > > component to test_runner.
> > > > 
> > > > Why is this now needed? Is this because a test that ran under chrome's
> unit
> > > > tests now doesn't get some initialization there?
> > > > 
> > > > I don't think we should this extra dependency now, since test_runner was
> > > > factored out for mandoline but that's not needed anymore. adding more
> > > > dependencies to it will make it harder to move back into content
> > (non-public).
> > > > 
> > > > Can you investigate why this wasn't needed in chrome's unit test target,
> and
> > > > follow what it does?
> > > 
> > > Yes this is needed for the test_runner::EnsureBlinkInitialized. It appears
> > other
> > > component tests using blink also do this. I've filed a follow-up
> > > crbug.com/634326 with more details and added todos for this in the
relevant
> > > files.
> > 
> > Digging a bit deeper, I think the case with chrome/ is that
> > chrome/test/base/run_all_unittests.cc uses a UnitTestTestSuite which in turn
> > initializes blink via TestBlinkWebUnitTestSupport
> 
> Ok, let's just make components_unittests do the same thing as chrome to
> instantiate this object:
> 
> i.e.
>
https://cs.chromium.org/chromium/src/components/test/run_all_unittests.cc?rcl...
> should copy the line from
>
https://cs.chromium.org/chromium/src/chrome/test/base/run_all_unittests.cc?rc...
> 
> then we can remove the other places in components that use test_runner as
well.
> You can do it in this change or in another cl if you want.

Ok I've tried to do the change in run_all_unittests (patch set 10) and it seems
a few bots still not happy, notably the ios bots, also looks like the clean-up
has to happen in one go to prevent triggering assert in blink
Platform::initialize. I would prefer to do this + cleanup in a separate patch
since it's not really spellcheck related anyway. Does this sound ok? It would be
great to land this patch since it is blocking further refactoring..

[jam, 2016-08-09 19:52:41]

On 2016/08/09 18:12:35, timvolodine wrote:
> On 2016/08/09 15:41:33, jam wrote:
> > On 2016/08/05 13:09:31, timvolodine wrote:
> > > On 2016/08/05 12:26:14, timvolodine wrote:
> > > > On 2016/08/04 17:49:54, jam wrote:
> > > > > On 2016/08/04 14:34:28, timvolodine wrote:
> > > > > > +jam@: for RS of the remaining few bits, in particular
> > > > > > 
> > > > > > chrome/renderer/BUILD.gn
> > > > > > chrome/renderer/DEPS
> > > > > > chrome/renderer/chrome_content_renderer_client.cc
> > > > > > chrome/test/base/chrome_render_view_test.cc
> > > > > > components/BUILD.gn
> > > > > 
> > > > > these look fine but I see this change adds a dependency from the
> > > spellchecker
> > > > > component to test_runner.
> > > > > 
> > > > > Why is this now needed? Is this because a test that ran under chrome's
> > unit
> > > > > tests now doesn't get some initialization there?
> > > > > 
> > > > > I don't think we should this extra dependency now, since test_runner
was
> > > > > factored out for mandoline but that's not needed anymore. adding more
> > > > > dependencies to it will make it harder to move back into content
> > > (non-public).
> > > > > 
> > > > > Can you investigate why this wasn't needed in chrome's unit test
target,
> > and
> > > > > follow what it does?
> > > > 
> > > > Yes this is needed for the test_runner::EnsureBlinkInitialized. It
appears
> > > other
> > > > component tests using blink also do this. I've filed a follow-up
> > > > crbug.com/634326 with more details and added todos for this in the
> relevant
> > > > files.
> > > 
> > > Digging a bit deeper, I think the case with chrome/ is that
> > > chrome/test/base/run_all_unittests.cc uses a UnitTestTestSuite which in
turn
> > > initializes blink via TestBlinkWebUnitTestSupport
> > 
> > Ok, let's just make components_unittests do the same thing as chrome to
> > instantiate this object:
> > 
> > i.e.
> >
>
https://cs.chromium.org/chromium/src/components/test/run_all_unittests.cc?rcl...
> > should copy the line from
> >
>
https://cs.chromium.org/chromium/src/chrome/test/base/run_all_unittests.cc?rc...
> > 
> > then we can remove the other places in components that use test_runner as
> well.
> > You can do it in this change or in another cl if you want.
> 
> Ok I've tried to do the change in run_all_unittests (patch set 10) and it
seems
> a few bots still not happy, notably the ios bots, also looks like the clean-up
> has to happen in one go to prevent triggering assert in blink
> Platform::initialize. I would prefer to do this + cleanup in a separate patch
> since it's not really spellcheck related anyway. Does this sound ok? It would
be
> great to land this patch since it is blocking further refactoring..

re ios: right blink isn't used there so this has to be surrounded by an ios
ifdef.
removing the other calls seems trivial.

can you do this in another cl first, and then land this one?

I understand that this slows you down a little. but if each refactoring ends up
extra dependencies to avoid being slowed down, that leads to our ever growing
dependency tree which adds a maintenance burden (i.e. when we want to move out
test_runner).

[timvolodine, 2016-08-09 23:15:47]

On 2016/08/09 19:52:41, jam wrote:
> On 2016/08/09 18:12:35, timvolodine wrote:
> > On 2016/08/09 15:41:33, jam wrote:
> > > On 2016/08/05 13:09:31, timvolodine wrote:
> > > > On 2016/08/05 12:26:14, timvolodine wrote:
> > > > > On 2016/08/04 17:49:54, jam wrote:
> > > > > > On 2016/08/04 14:34:28, timvolodine wrote:
> > > > > > > +jam@: for RS of the remaining few bits, in particular
> > > > > > > 
> > > > > > > chrome/renderer/BUILD.gn
> > > > > > > chrome/renderer/DEPS
> > > > > > > chrome/renderer/chrome_content_renderer_client.cc
> > > > > > > chrome/test/base/chrome_render_view_test.cc
> > > > > > > components/BUILD.gn
> > > > > > 
> > > > > > these look fine but I see this change adds a dependency from the
> > > > spellchecker
> > > > > > component to test_runner.
> > > > > > 
> > > > > > Why is this now needed? Is this because a test that ran under
chrome's
> > > unit
> > > > > > tests now doesn't get some initialization there?
> > > > > > 
> > > > > > I don't think we should this extra dependency now, since test_runner
> was
> > > > > > factored out for mandoline but that's not needed anymore. adding
more
> > > > > > dependencies to it will make it harder to move back into content
> > > > (non-public).
> > > > > > 
> > > > > > Can you investigate why this wasn't needed in chrome's unit test
> target,
> > > and
> > > > > > follow what it does?
> > > > > 
> > > > > Yes this is needed for the test_runner::EnsureBlinkInitialized. It
> appears
> > > > other
> > > > > component tests using blink also do this. I've filed a follow-up
> > > > > crbug.com/634326 with more details and added todos for this in the
> > relevant
> > > > > files.
> > > > 
> > > > Digging a bit deeper, I think the case with chrome/ is that
> > > > chrome/test/base/run_all_unittests.cc uses a UnitTestTestSuite which in
> turn
> > > > initializes blink via TestBlinkWebUnitTestSupport
> > > 
> > > Ok, let's just make components_unittests do the same thing as chrome to
> > > instantiate this object:
> > > 
> > > i.e.
> > >
> >
>
https://cs.chromium.org/chromium/src/components/test/run_all_unittests.cc?rcl...
> > > should copy the line from
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/test/base/run_all_unittests.cc?rc...
> > > 
> > > then we can remove the other places in components that use test_runner as
> > well.
> > > You can do it in this change or in another cl if you want.
> > 
> > Ok I've tried to do the change in run_all_unittests (patch set 10) and it
> seems
> > a few bots still not happy, notably the ios bots, also looks like the
clean-up
> > has to happen in one go to prevent triggering assert in blink
> > Platform::initialize. I would prefer to do this + cleanup in a separate
patch
> > since it's not really spellcheck related anyway. Does this sound ok? It
would
> be
> > great to land this patch since it is blocking further refactoring..
> 
> re ios: right blink isn't used there so this has to be surrounded by an ios
> ifdef.
> removing the other calls seems trivial.
> 
> can you do this in another cl first, and then land this one?

ok I've created a cl with the fixes here :
https://codereview.chromium.org/2229943002/
however there still seem to be some issues with Bookmark unittests on linux, due
to "Only one platform event source can be created." not sure what's happening
there, any ideas?

> 
> I understand that this slows you down a little. but if each refactoring ends
up
> extra dependencies to avoid being slowed down, that leads to our ever growing
> dependency tree which adds a maintenance burden (i.e. when we want to move out
> test_runner).

Yes I agree, but at the same time don't see why fixing the components unit
testing framework should be a blocker for this particular spellchecker
refactoring. It clearly wasn't for the other components. Especially given it
turns out to be not so trivial.

[jam, 2016-08-10 00:04:43]

On 2016/08/09 23:15:47, timvolodine wrote:
> On 2016/08/09 19:52:41, jam wrote:
> > On 2016/08/09 18:12:35, timvolodine wrote:
> > > On 2016/08/09 15:41:33, jam wrote:
> > > > On 2016/08/05 13:09:31, timvolodine wrote:
> > > > > On 2016/08/05 12:26:14, timvolodine wrote:
> > > > > > On 2016/08/04 17:49:54, jam wrote:
> > > > > > > On 2016/08/04 14:34:28, timvolodine wrote:
> > > > > > > > +jam@: for RS of the remaining few bits, in particular
> > > > > > > > 
> > > > > > > > chrome/renderer/BUILD.gn
> > > > > > > > chrome/renderer/DEPS
> > > > > > > > chrome/renderer/chrome_content_renderer_client.cc
> > > > > > > > chrome/test/base/chrome_render_view_test.cc
> > > > > > > > components/BUILD.gn
> > > > > > > 
> > > > > > > these look fine but I see this change adds a dependency from the
> > > > > spellchecker
> > > > > > > component to test_runner.
> > > > > > > 
> > > > > > > Why is this now needed? Is this because a test that ran under
> chrome's
> > > > unit
> > > > > > > tests now doesn't get some initialization there?
> > > > > > > 
> > > > > > > I don't think we should this extra dependency now, since
test_runner
> > was
> > > > > > > factored out for mandoline but that's not needed anymore. adding
> more
> > > > > > > dependencies to it will make it harder to move back into content
> > > > > (non-public).
> > > > > > > 
> > > > > > > Can you investigate why this wasn't needed in chrome's unit test
> > target,
> > > > and
> > > > > > > follow what it does?
> > > > > > 
> > > > > > Yes this is needed for the test_runner::EnsureBlinkInitialized. It
> > appears
> > > > > other
> > > > > > component tests using blink also do this. I've filed a follow-up
> > > > > > crbug.com/634326 with more details and added todos for this in the
> > > relevant
> > > > > > files.
> > > > > 
> > > > > Digging a bit deeper, I think the case with chrome/ is that
> > > > > chrome/test/base/run_all_unittests.cc uses a UnitTestTestSuite which
in
> > turn
> > > > > initializes blink via TestBlinkWebUnitTestSupport
> > > > 
> > > > Ok, let's just make components_unittests do the same thing as chrome to
> > > > instantiate this object:
> > > > 
> > > > i.e.
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/components/test/run_all_unittests.cc?rcl...
> > > > should copy the line from
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/test/base/run_all_unittests.cc?rc...
> > > > 
> > > > then we can remove the other places in components that use test_runner
as
> > > well.
> > > > You can do it in this change or in another cl if you want.
> > > 
> > > Ok I've tried to do the change in run_all_unittests (patch set 10) and it
> > seems
> > > a few bots still not happy, notably the ios bots, also looks like the
> clean-up
> > > has to happen in one go to prevent triggering assert in blink
> > > Platform::initialize. I would prefer to do this + cleanup in a separate
> patch
> > > since it's not really spellcheck related anyway. Does this sound ok? It
> would
> > be
> > > great to land this patch since it is blocking further refactoring..
> > 
> > re ios: right blink isn't used there so this has to be surrounded by an ios
> > ifdef.
> > removing the other calls seems trivial.
> > 
> > can you do this in another cl first, and then land this one?
> 
> ok I've created a cl with the fixes here :
> https://codereview.chromium.org/2229943002/
> however there still seem to be some issues with Bookmark unittests on linux,
due
> to "Only one platform event source can be created." not sure what's happening
> there, any ideas?

I've replied on that cl with the the lines that need to be removed (verified
locally).

> 
> > 
> > I understand that this slows you down a little. but if each refactoring ends
> up
> > extra dependencies to avoid being slowed down, that leads to our ever
growing
> > dependency tree which adds a maintenance burden (i.e. when we want to move
out
> > test_runner).
> 
> Yes I agree, but at the same time don't see why fixing the components unit
> testing framework should be a blocker for this particular spellchecker
> refactoring. It clearly wasn't for the other components. Especially given it
> turns out to be not so trivial.

The reason is that probably other reviewers might not care about adding extra
dependencies. I've seen the impact of the overuse and dependence on components
more than the average person (i.e. see
https://bugs.chromium.org/p/chromium/issues/detail?id=624590), so that's why I
asked to not add more depenencies. I also worked on mandoline which caused
test_runner to be factored out, so not all other reviewers may know that this is
something that should move back
(https://bugs.chromium.org/p/chromium/issues/detail?id=585292).

lgtm for this cl after https://codereview.chromium.org/2229943002/ lands

[timvolodine, 2016-08-11 22:32:48]

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

[commit-bot: I haz the power, 2016-08-11 22:33:35]

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

[timvolodine, 2016-08-11 22:37:10]

> > ok I've created a cl with the fixes here :
> > https://codereview.chromium.org/2229943002/
> > however there still seem to be some issues with Bookmark unittests on linux,
> due
> > to "Only one platform event source can be created." not sure what's
happening
> > there, any ideas?
> 
> I've replied on that cl with the the lines that need to be removed (verified
> locally).
>

yes, thanks for your help on that patch :)
 
> > 
> > > 
> > > I understand that this slows you down a little. but if each refactoring
ends
> > up
> > > extra dependencies to avoid being slowed down, that leads to our ever
> growing
> > > dependency tree which adds a maintenance burden (i.e. when we want to move
> out
> > > test_runner).
> > 
> > Yes I agree, but at the same time don't see why fixing the components unit
> > testing framework should be a blocker for this particular spellchecker
> > refactoring. It clearly wasn't for the other components. Especially given it
> > turns out to be not so trivial.
> 
> The reason is that probably other reviewers might not care about adding extra
> dependencies. I've seen the impact of the overuse and dependence on components
> more than the average person (i.e. see
> https://bugs.chromium.org/p/chromium/issues/detail?id=624590), so that's why I
> asked to not add more depenencies. I also worked on mandoline which caused
> test_runner to be factored out, so not all other reviewers may know that this
is
> something that should move back
> (https://bugs.chromium.org/p/chromium/issues/detail?id=585292).
> 
> lgtm for this cl after https://codereview.chromium.org/2229943002/ lands

Makes sense, it's just that I was slightly worried that clean-up would spiral
into a 'spaghetti' chain of fixes. The testrunner patch has landed, will land
this cl as well after rebase.

Thanks for the reviews!

[commit-bot: I haz the power, 2016-08-12 00:10:14]

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

[commit-bot: I haz the power, 2016-08-12 00:10:15]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2016-08-12 00:10:27]

The CQ bit was checked by timvolodine@chromium.org

[timvolodine, 2016-08-12 00:10:27]

The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2198143002/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-12 00:11:24]

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-12 00:18:38]

Description was changed from

==========
Componentize spellcheck [3]: move renderer/ files to component.

Move all chrome/renderer spellcheck files and tests to component,
update dependencies and build files. Also address some compile
and unittest issues, e.g. make sure to properly initialize blink
in tests to avoid crashes.

The main motivation behind the componentization of the spellcheck
feature is that we want to make it available in Android WebView.

Preceding 'componentize' patches:
[1] https://codereview.chromium.org/2166683003/
[2] https://codereview.chromium.org/2177343002/

BUG=583616, 629609
==========

to

==========
Componentize spellcheck [3]: move renderer/ files to component.

Move all chrome/renderer spellcheck files and tests to component,
update dependencies and build files. Also address some compile
and unittest issues, e.g. make sure to properly initialize blink
in tests to avoid crashes.

The main motivation behind the componentization of the spellcheck
feature is that we want to make it available in Android WebView.

Preceding 'componentize' patches:
[1] https://codereview.chromium.org/2166683003/
[2] https://codereview.chromium.org/2177343002/

BUG=583616, 629609
==========

[commit-bot: I haz the power, 2016-08-12 00:18:39]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-08-12 00:21:55]

Description was changed from

==========
Componentize spellcheck [3]: move renderer/ files to component.

Move all chrome/renderer spellcheck files and tests to component,
update dependencies and build files. Also address some compile
and unittest issues, e.g. make sure to properly initialize blink
in tests to avoid crashes.

The main motivation behind the componentization of the spellcheck
feature is that we want to make it available in Android WebView.

Preceding 'componentize' patches:
[1] https://codereview.chromium.org/2166683003/
[2] https://codereview.chromium.org/2177343002/

BUG=583616, 629609
==========

to

==========
Componentize spellcheck [3]: move renderer/ files to component.

Move all chrome/renderer spellcheck files and tests to component,
update dependencies and build files. Also address some compile
and unittest issues, e.g. make sure to properly initialize blink
in tests to avoid crashes.

The main motivation behind the componentization of the spellcheck
feature is that we want to make it available in Android WebView.

Preceding 'componentize' patches:
[1] https://codereview.chromium.org/2166683003/
[2] https://codereview.chromium.org/2177343002/

BUG=583616, 629609

Committed: https://crrev.com/7ef691f4cdae52d0171ee1abafe33b5df8c09dd3
Cr-Commit-Position: refs/heads/master@{#411469}
==========

[commit-bot: I haz the power, 2016-08-12 00:21:56]

Patchset 12 (id:??) landed as
https://crrev.com/7ef691f4cdae52d0171ee1abafe33b5df8c09dd3
Cr-Commit-Position: refs/heads/master@{#411469}