Componentize spellcheck [1]: create component and move switches there.

Componentize spellcheck [1]: create component and move switches there.

Current spellcheck code lives in the chrome/ layer, but we want to make it
available in webview as well. Therefore this effort of making a dedicated
spellcheck component. As a first step we create a //components/spellcheck
and move the chrome spellcheck related switches there.

BUG=583616, 629609
Committed: https://crrev.com/677f301fb8de766a62d065408821722bb89f85f3
Cr-Commit-Position: refs/heads/master@{#407469}

[timvolodine, 2016-07-20 12:33:41]

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

[commit-bot: I haz the power, 2016-07-20 12:34:09]

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-07-20 12:59:14]

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

[commit-bot: I haz the power, 2016-07-20 12:59:15]

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

[timvolodine, 2016-07-20 13:12:28]

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

[timvolodine, 2016-07-20 13:13:16]

On 2016/07/20 13:12:29, timvolodine wrote:

peter@: if you have the time to take a look..

[Peter Beverloo, 2016-07-20 13:35:25]

This seems good to me with a few comments and questions:

  - Is the plan to move all of //chrome/{browser,renderer}/spellchecker/
    and the common files to the new (layered) component? A cursory look
    tells me that the it'll largely be a mechanical move, modulo
    dependencies on grit, Chrome's user data directory and some testing
    infrastructure. Will the consumer have to pass a delegate of sorts?

  - Similar CLs tend to have thousands of lines and move everything in one
    go, and while it's fine to move everything in iterations instead, you
    may find that the presence of IPC makes this difficult. (It's not
    great for one layer to include a messages file from another layer...)

  - Having two consumers (WebView and Chrome) qualifies this for being a
    component, but since the rules recently were tightened it would be
    good to clarify in your CL description *why* it has to be
    componentized.

https://codereview.chromium.org/2166683003/diff/1/components/spellcheck/OWNERS
File components/spellcheck/OWNERS (right):

https://codereview.chromium.org/2166683003/diff/1/components/spellcheck/OWNER...
components/spellcheck/OWNERS:1: timvolodine@chromium.org
+groby@chromium.org
+rouslan@chromium.org

(They are owners of the existing code and both still work on Chromium. They can
review the move as well :-).)

[timvolodine, 2016-07-20 18:53:42]

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

[commit-bot: I haz the power, 2016-07-20 18:54:43]

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

[timvolodine, 2016-07-20 19:06:33]

Description was changed from

==========
Componentize spellcheck [1]: create component and move switches there.

As a first step create a //components/spellcheck and move the
chrome spellcheck related switches there.

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

to

==========
Componentize spellcheck [1]: create component and move switches there.

Current spellcheck code lives in the chrome/ layer, but we want to make it
available in webview as well. Therefore this effort of making a dedicated
spellcheck component. As a first step we create a //components/spellcheck
and move the chrome spellcheck related switches there.

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

[timvolodine, 2016-07-20 19:26:00]

Thanks for the comments! replies below:

On 2016/07/20 13:35:25, Peter Beverloo wrote:
> This seems good to me with a few comments and questions:
> 
>   - Is the plan to move all of //chrome/{browser,renderer}/spellchecker/
>     and the common files to the new (layered) component? A cursory look
>     tells me that the it'll largely be a mechanical move, modulo
>     dependencies on grit, Chrome's user data directory and some testing
>     infrastructure. Will the consumer have to pass a delegate of sorts?

Yes, plan is to have //components/spellcheck/{browser,renderer,common,android}
containing the common code. As you already mentioned there are some bits that
are less mechanical than others, especially the //chrome/browser/ seems to be
the most 'difficult' one. We'll need some kind of 'delegate' for a few classes
using grit and chrome paths, maybe set by the consumer at some 'early' stage or
even make it statically linked.
> 
>   - Similar CLs tend to have thousands of lines and move everything in one
>     go, and while it's fine to move everything in iterations instead, you
>     may find that the presence of IPC makes this difficult. (It's not
>     great for one layer to include a messages file from another layer...)
> 
I went for 'incremental' approach to keep the patches smaller and easier to
review (and hopefully faster). Also this avoids painful rebases in case of one
large patch.
There is actually a work-in-progress master patch here:
https://codereview.chromium.org/2159283003/, which seemed cumbersome to review
in one go though..

>   - Having two consumers (WebView and Chrome) qualifies this for being a
>     component, but since the rules recently were tightened it would be
>     good to clarify in your CL description *why* it has to be
>     componentized.
> 
Yes the main idea is to make it available in webview. Another option is to move
it to content/ layer, however making a component is probably cleaner. I've added
more explanation in description. Should I worry about the new rules? is there
any pointer to the (tightened) rules?

[timvolodine, 2016-07-20 19:26:23]

https://codereview.chromium.org/2166683003/diff/1/components/spellcheck/OWNERS
File components/spellcheck/OWNERS (right):

https://codereview.chromium.org/2166683003/diff/1/components/spellcheck/OWNER...
components/spellcheck/OWNERS:1: timvolodine@chromium.org
On 2016/07/20 13:35:25, Peter Beverloo wrote:
> mailto:+groby@chromium.org
> mailto:+rouslan@chromium.org
> 
> (They are owners of the existing code and both still work on Chromium. They
can
> review the move as well :-).)

yes of course. DONE.

[timvolodine, 2016-07-20 19:27:45]

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

[timvolodine, 2016-07-20 19:27:45]

+rouslan@ and groby@ : FYI, move refactoring will be coming soon..

[commit-bot: I haz the power, 2016-07-20 19:35:25]

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

[commit-bot: I haz the power, 2016-07-20 19:35:28]

Dry run: Try jobs failed on following builders:
  win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)

[groby-ooo-7-16, 2016-07-20 21:04:38]

On 2016/07/20 19:27:45, timvolodine wrote:
> +rouslan@ and groby@ : FYI, move refactoring will be coming soon..

Thanks for the heads up - any details on what "some kind of delegate" will
actually be?

Also, be aware there have been earlier efforts to componentize - see 
https://codereview.chromium.org/1884583003/ (might be worth reaching out to the
author?)

[groby-ooo-7-16, 2016-07-20 21:07:34]

On 2016/07/20 21:04:38, groby wrote:
> On 2016/07/20 19:27:45, timvolodine wrote:
> > +rouslan@ and groby@ : FYI, move refactoring will be coming soon..
> 
> Thanks for the heads up - any details on what "some kind of delegate" will
> actually be?
> 
> Also, be aware there have been earlier efforts to componentize - see 
> https://codereview.chromium.org/1884583003/ (might be worth reaching out to
the
> author?)

Skimming some more - Android spellcheck is currently not enabled because it
relies on functionality provided by the keyboard. (And only the Google Keyboard
offers it, others don't). Which means embedding this in WebView will not be
helpful, per se :)

Are there plans to revitalize Android spellchecking in general?

[Peter Beverloo, 2016-07-20 22:15:59]

On 2016/07/20 19:26:00, timvolodine wrote:
> Thanks for the comments! replies below:
> 
> On 2016/07/20 13:35:25, Peter Beverloo wrote:
> > This seems good to me with a few comments and questions:
> > 
> >   - Is the plan to move all of //chrome/{browser,renderer}/spellchecker/
> >     and the common files to the new (layered) component? A cursory look
> >     tells me that the it'll largely be a mechanical move, modulo
> >     dependencies on grit, Chrome's user data directory and some testing
> >     infrastructure. Will the consumer have to pass a delegate of sorts?
> 
> Yes, plan is to have //components/spellcheck/{browser,renderer,common,android}
> containing the common code. As you already mentioned there are some bits that
> are less mechanical than others, especially the //chrome/browser/ seems to be
> the most 'difficult' one. We'll need some kind of 'delegate' for a few classes
> using grit and chrome paths, maybe set by the consumer at some 'early' stage
or
> even make it statically linked.
> > 
> >   - Similar CLs tend to have thousands of lines and move everything in one
> >     go, and while it's fine to move everything in iterations instead, you
> >     may find that the presence of IPC makes this difficult. (It's not
> >     great for one layer to include a messages file from another layer...)
> > 
> I went for 'incremental' approach to keep the patches smaller and easier to
> review (and hopefully faster). Also this avoids painful rebases in case of one
> large patch.
> There is actually a work-in-progress master patch here:
> https://codereview.chromium.org/2159283003/, which seemed cumbersome to review
> in one go though..
> 
> >   - Having two consumers (WebView and Chrome) qualifies this for being a
> >     component, but since the rules recently were tightened it would be
> >     good to clarify in your CL description *why* it has to be
> >     componentized.
> > 
> Yes the main idea is to make it available in webview. Another option is to
move
> it to content/ layer, however making a component is probably cleaner. I've
added
> more explanation in description. Should I worry about the new rules? is there
> any pointer to the (tightened) rules?

No, you don't have to worry as this qualifies. John's updating the README here:

https://codereview.chromium.org/2166093002/#

[Peter Beverloo, 2016-07-20 22:16:43]

On 2016/07/20 21:07:34, groby wrote:
> On 2016/07/20 21:04:38, groby wrote:
> > On 2016/07/20 19:27:45, timvolodine wrote:
> > > +rouslan@ and groby@ : FYI, move refactoring will be coming soon..
> > 
> > Thanks for the heads up - any details on what "some kind of delegate" will
> > actually be?
> > 
> > Also, be aware there have been earlier efforts to componentize - see 
> > https://codereview.chromium.org/1884583003/ (might be worth reaching out to
> the
> > author?)
> 
> Skimming some more - Android spellcheck is currently not enabled because it
> relies on functionality provided by the keyboard. (And only the Google
Keyboard
> offers it, others don't). Which means embedding this in WebView will not be
> helpful, per se :)
> 
> Are there plans to revitalize Android spellchecking in general?

That's exactly what Tim's doing :-)

[groby-ooo-7-16, 2016-07-21 14:44:22]

On 2016/07/20 22:16:43, Peter Beverloo wrote:
> On 2016/07/20 21:07:34, groby wrote:
> > On 2016/07/20 21:04:38, groby wrote:
> > > On 2016/07/20 19:27:45, timvolodine wrote:
> > > > +rouslan@ and groby@ : FYI, move refactoring will be coming soon..
> > > 
> > > Thanks for the heads up - any details on what "some kind of delegate" will
> > > actually be?
> > > 
> > > Also, be aware there have been earlier efforts to componentize - see 
> > > https://codereview.chromium.org/1884583003/ (might be worth reaching out
to
> > the
> > > author?)
> > 
> > Skimming some more - Android spellcheck is currently not enabled because it
> > relies on functionality provided by the keyboard. (And only the Google
> Keyboard
> > offers it, others don't). Which means embedding this in WebView will not be
> > helpful, per se :)
> > 
> > Are there plans to revitalize Android spellchecking in general?
> 
> That's exactly what Tim's doing :-)

Ah - in that case, streamers and confetti are due! Thank you for doing this.

[timvolodine, 2016-07-21 15:27:40]

On 2016/07/21 14:44:22, groby wrote:
> On 2016/07/20 22:16:43, Peter Beverloo wrote:
> > On 2016/07/20 21:07:34, groby wrote:
> > > On 2016/07/20 21:04:38, groby wrote:
> > > > On 2016/07/20 19:27:45, timvolodine wrote:
> > > > > +rouslan@ and groby@ : FYI, move refactoring will be coming soon..
> > > > 
> > > > Thanks for the heads up - any details on what "some kind of delegate"
will
> > > > actually be?
> > > > 
> > > > Also, be aware there have been earlier efforts to componentize - see 
> > > > https://codereview.chromium.org/1884583003/ (might be worth reaching out
> to
> > > the
> > > > author?)
> > > 
> > > Skimming some more - Android spellcheck is currently not enabled because
it
> > > relies on functionality provided by the keyboard. (And only the Google
> > Keyboard
> > > offers it, others don't). Which means embedding this in WebView will not
be
> > > helpful, per se :)
> > > 
> > > Are there plans to revitalize Android spellchecking in general?
> > 
> > That's exactly what Tim's doing :-)
> 
> Ah - in that case, streamers and confetti are due! Thank you for doing this.

groby@:
yes the idea is to make it 'shippable' on android, I've shared a design doc with
you with more details https://goo.gl/tcDCGz.

I am aware of the existing patches but they are somewhat stale and not exactly
what we want hence this renewed effort.

[timvolodine, 2016-07-21 19:31:51]

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

[commit-bot: I haz the power, 2016-07-21 19:33:10]

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-07-21 19:46:30]

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

[commit-bot: I haz the power, 2016-07-21 19:46:31]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[timvolodine, 2016-07-22 11:52:50]

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

[commit-bot: I haz the power, 2016-07-22 11:52:59]

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

[Peter Beverloo, 2016-07-22 12:45:25]

Anyhow, I think we'd all like to see this happen so lgtm to start the move :)

https://codereview.chromium.org/2166683003/diff/60001/components/spellcheck.gypi
File components/spellcheck.gypi (right):

https://codereview.chromium.org/2166683003/diff/60001/components/spellcheck.g...
components/spellcheck.gypi:1: # Copyright (c) 2016 The Chromium Authors. All
rights reserved.
nit: no (c)

https://codereview.chromium.org/2166683003/diff/60001/components/spellcheck.g...
components/spellcheck.gypi:5: # GYP version:
components:spellcheck.gypi:spellcheck_common
I don't understand this line- the GN version is referred to on line 9, so I
think you can just remove it

https://codereview.chromium.org/2166683003/diff/60001/components/spellcheck/O...
File components/spellcheck/OWNERS (right):

https://codereview.chromium.org/2166683003/diff/60001/components/spellcheck/O...
components/spellcheck/OWNERS:3: timvolodine@chromium.org
Up to groby@ and rouslan@, but you could consider adding a comment that your
ownership exists for the purposes of refactoring and Android until you gain more
experience with the code.

[commit-bot: I haz the power, 2016-07-22 13:02:47]

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

[commit-bot: I haz the power, 2016-07-22 13:02:48]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2016-07-22 17:23:19]

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

[commit-bot: I haz the power, 2016-07-22 17:23:41]

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

[timvolodine, 2016-07-22 17:25:35]

thanks for the review!

https://codereview.chromium.org/2166683003/diff/60001/components/spellcheck.gypi
File components/spellcheck.gypi (right):

https://codereview.chromium.org/2166683003/diff/60001/components/spellcheck.g...
components/spellcheck.gypi:1: # Copyright (c) 2016 The Chromium Authors. All
rights reserved.
On 2016/07/22 12:45:24, Peter Beverloo wrote:
> nit: no (c)

Done.

https://codereview.chromium.org/2166683003/diff/60001/components/spellcheck.g...
components/spellcheck.gypi:5: # GYP version:
components:spellcheck.gypi:spellcheck_common
On 2016/07/22 12:45:24, Peter Beverloo wrote:
> I don't understand this line- the GN version is referred to on line 9, so I
> think you can just remove it

removed, done.

https://codereview.chromium.org/2166683003/diff/60001/components/spellcheck/O...
File components/spellcheck/OWNERS (right):

https://codereview.chromium.org/2166683003/diff/60001/components/spellcheck/O...
components/spellcheck/OWNERS:3: timvolodine@chromium.org
On 2016/07/22 12:45:25, Peter Beverloo wrote:
> Up to groby@ and rouslan@, but you could consider adding a comment that your
> ownership exists for the purposes of refactoring and Android until you gain
more
> experience with the code.

Done.

[timvolodine, 2016-07-22 17:28:12]

groby@: are you fine with this patch? just making sure :)

[commit-bot: I haz the power, 2016-07-22 18:26:11]

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

[commit-bot: I haz the power, 2016-07-22 18:26:12]

Dry run: This issue passed the CQ dry run.

[groby-ooo-7-16, 2016-07-22 18:44:48]

LGTM

[timvolodine, 2016-07-22 19:35:19]

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

[timvolodine, 2016-07-22 19:35:20]

+jam@: for the general rubber-stamp

[jam, 2016-07-22 20:51:45]

lgtm

[timvolodine, 2016-07-25 13:12:32]

The CQ bit was checked by timvolodine@chromium.org

[timvolodine, 2016-07-25 13:12:32]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
groby@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2166683003/#ps100001
(title: "rebase + --no-find-copies")

[commit-bot: I haz the power, 2016-07-25 13:12:40]

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-07-25 14:01:13]

Description was changed from

==========
Componentize spellcheck [1]: create component and move switches there.

Current spellcheck code lives in the chrome/ layer, but we want to make it
available in webview as well. Therefore this effort of making a dedicated
spellcheck component. As a first step we create a //components/spellcheck
and move the chrome spellcheck related switches there.

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

to

==========
Componentize spellcheck [1]: create component and move switches there.

Current spellcheck code lives in the chrome/ layer, but we want to make it
available in webview as well. Therefore this effort of making a dedicated
spellcheck component. As a first step we create a //components/spellcheck
and move the chrome spellcheck related switches there.

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

[commit-bot: I haz the power, 2016-07-25 14:01:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-25 14:03:39]

Description was changed from

==========
Componentize spellcheck [1]: create component and move switches there.

Current spellcheck code lives in the chrome/ layer, but we want to make it
available in webview as well. Therefore this effort of making a dedicated
spellcheck component. As a first step we create a //components/spellcheck
and move the chrome spellcheck related switches there.

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

to

==========
Componentize spellcheck [1]: create component and move switches there.

Current spellcheck code lives in the chrome/ layer, but we want to make it
available in webview as well. Therefore this effort of making a dedicated
spellcheck component. As a first step we create a //components/spellcheck
and move the chrome spellcheck related switches there.

BUG=583616, 629609

Committed: https://crrev.com/677f301fb8de766a62d065408821722bb89f85f3
Cr-Commit-Position: refs/heads/master@{#407469}
==========

[commit-bot: I haz the power, 2016-07-25 14:03:41]

Patchset 6 (id:??) landed as
https://crrev.com/677f301fb8de766a62d065408821722bb89f85f3
Cr-Commit-Position: refs/heads/master@{#407469}