Create a spellcheck component that will allow any embedder to reuse common functionality.

Create a spellcheck component that will allow any embedder to reuse common functionality.

Patch by Ed Baker <edbaker@blackberry.com>;.

BUG=583616

[cosmin.truta, 2016-04-12 15:14:25]

The CQ bit was checked by ctruta@blackberry.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-12 15:14:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884583003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884583003/1

[commit-bot: I haz the power, 2016-04-12 15:14:45]

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

[commit-bot: I haz the power, 2016-04-12 15:14:46]

Dry run: The author ctruta@blackberry.com has not signed Google Contributor
License Agreement. Please visit https://cla.developers.google.com to sign and
manage CLA.

[cosmin.truta, 2016-04-12 15:19:15]

Description was changed from

==========
Create a spellcheck component that will allow any embedder to reuse common
functionality.

Patch by Ed Baker <edbaker@blackberry.com>

BUG=583616
==========

to

==========
Create a spellcheck component that will allow any embedder to reuse common
functionality.

Patch by Ed Baker <edbaker@blackberry.com>

BUG=583616
==========

[cosmin.truta, 2016-04-12 15:19:16]

ctruta@blackberry.com changed reviewers:
+ groby@chromium.org, miguelg@chromium.org, morrita@chromium.org,
torne@chromium.org

[cosmin.truta, 2016-04-12 15:24:10]

On 2016/04/12 15:14:46, commit-bot: I haz the power wrote:
> Dry run: The author mailto:ctruta@blackberry.com has not signed Google
Contributor
> License Agreement. Please visit https://cla.developers.google.com to sign and
> manage CLA.

Hi,
I became an authorized Chromium contributor on behalf of BlackBerry Ltd. in
November 2013, although I may be mentioned under the previous company name
(RIM) in the CLA papers. I hope everything is all right on the legal front.

[edbaker, 2016-04-12 15:46:17]

Hi,

I'd like to ask for a review of this patch whenever it's convenient. The
intention is to create a spellcheck component and begin the process of
refactoring from /chrome. Once enough code has been refactored it will allow
interested embedders like android webview to make use of the common spellcheck
functionality.

[groby-ooo-7-16, 2016-04-12 17:37:58]

On 2016/04/12 15:46:17, edbaker wrote:
> Hi,
> 
> I'd like to ask for a review of this patch whenever it's convenient. The
> intention is to create a spellcheck component and begin the process of
> refactoring from /chrome. Once enough code has been refactored it will allow
> interested embedders like android webview to make use of the common spellcheck
> functionality.

Is there a short design doc for this?

[groby-ooo-7-16, 2016-04-12 17:39:00]

On 2016/04/12 15:24:10, cosmin.truta wrote:
> On 2016/04/12 15:14:46, commit-bot: I haz the power wrote:
> > Dry run: The author mailto:ctruta@blackberry.com has not signed Google
> Contributor
> > License Agreement. Please visit https://cla.developers.google.com to sign
and
> > manage CLA.
> 
> Hi,
> I became an authorized Chromium contributor on behalf of BlackBerry Ltd. in
> November 2013, although I may be mentioned under the previous company name
> (RIM) in the CLA papers. I hope everything is all right on the legal front.

Hm. I can't find either Blackberry or RIM in the CLA. Do you remember if it was
a company CLA or an individual one?

[Liam Quinn, 2016-04-12 17:41:16]

On 2016/04/12 17:39:00, groby wrote:
> On 2016/04/12 15:24:10, cosmin.truta wrote:
> > On 2016/04/12 15:14:46, commit-bot: I haz the power wrote:
> > > Dry run: The author mailto:ctruta@blackberry.com has not signed Google
> > Contributor
> > > License Agreement. Please visit https://cla.developers.google.com to sign
> and
> > > manage CLA.
> > 
> > Hi,
> > I became an authorized Chromium contributor on behalf of BlackBerry Ltd. in
> > November 2013, although I may be mentioned under the previous company name
> > (RIM) in the CLA papers. I hope everything is all right on the legal front.
> 
> Hm. I can't find either Blackberry or RIM in the CLA. Do you remember if it
was
> a company CLA or an individual one?

It was a company CLA. *@blackberry.com is listed in
https://chromium.googlesource.com/chromium/src/+/master/AUTHORS, and
ctruta@blackberry.com has contributed previously.

[Liam Quinn, 2016-04-12 17:49:26]

On 2016/04/12 17:41:16, Liam Quinn wrote:
> On 2016/04/12 17:39:00, groby wrote:
> > On 2016/04/12 15:24:10, cosmin.truta wrote:
> > > On 2016/04/12 15:14:46, commit-bot: I haz the power wrote:
> > > > Dry run: The author mailto:ctruta@blackberry.com has not signed Google
> > > Contributor
> > > > License Agreement. Please visit https://cla.developers.google.com to
sign
> > and
> > > > manage CLA.
> > > 
> > > Hi,
> > > I became an authorized Chromium contributor on behalf of BlackBerry Ltd.
in
> > > November 2013, although I may be mentioned under the previous company name
> > > (RIM) in the CLA papers. I hope everything is all right on the legal
front.
> > 
> > Hm. I can't find either Blackberry or RIM in the CLA. Do you remember if it
> was
> > a company CLA or an individual one?
> 
> It was a company CLA. mailto:*@blackberry.com is listed in
> https://chromium.googlesource.com/chromium/src/+/master/AUTHORS, and
> mailto:ctruta@blackberry.com has contributed previously.

When we signed the agreement, the company name was officially "Research In
Motion Limited".

[groby-ooo-7-16, 2016-04-12 17:54:08]

On 2016/04/12 17:49:26, Liam Quinn wrote:
> On 2016/04/12 17:41:16, Liam Quinn wrote:
> > On 2016/04/12 17:39:00, groby wrote:
> > > On 2016/04/12 15:24:10, cosmin.truta wrote:
> > > > On 2016/04/12 15:14:46, commit-bot: I haz the power wrote:
> > > > > Dry run: The author mailto:ctruta@blackberry.com has not signed Google
> > > > Contributor
> > > > > License Agreement. Please visit https://cla.developers.google.com to
> sign
> > > and
> > > > > manage CLA.
> > > > 
> > > > Hi,
> > > > I became an authorized Chromium contributor on behalf of BlackBerry Ltd.
> in
> > > > November 2013, although I may be mentioned under the previous company
name
> > > > (RIM) in the CLA papers. I hope everything is all right on the legal
> front.
> > > 
> > > Hm. I can't find either Blackberry or RIM in the CLA. Do you remember if
it
> > was
> > > a company CLA or an individual one?
> > 
> > It was a company CLA. mailto:*@blackberry.com is listed in
> > https://chromium.googlesource.com/chromium/src/+/master/AUTHORS, and
> > mailto:ctruta@blackberry.com has contributed previously.
> 
> When we signed the agreement, the company name was officially "Research In
> Motion Limited".

I'll follow up with the Powers That Be to get the CLA issue sorted. (It's not in
the list we're supposed to check against, so I want to make sure you don't face
this question for every single CL :)

[groby-ooo-7-16, 2016-04-12 17:56:51]

Ed: Thank you for taking this on - much appreciated!

The reason I asked for a design doc is that I assume there will be future
patches, and I was wondering if you had given the embedding dependencies (i.e.
when/if spellcheck needs to call out to Chrome proper) any thought.

Overall, looks good to me. I'll sort the CLA issue, and then it should be a
speedy progress from there. 

Oh, and if it's trivial for you - and really, only then - it'd be great if you
could break this into a namespacing CL and a move CL. (Assuming we need to keep
the namespace)

https://codereview.chromium.org/1884583003/diff/1/chrome/browser/renderer_con...
File chrome/browser/renderer_context_menu/spelling_menu_observer.h (right):

https://codereview.chromium.org/1884583003/diff/1/chrome/browser/renderer_con...
chrome/browser/renderer_context_menu/spelling_menu_observer.h:24: namespace
spellcheck {
Why the namespace? Chromium usually tries to avoid namespacing unless necessary.

https://codereview.chromium.org/1884583003/diff/1/chrome/browser/spellchecker...
File
chrome/browser/spellchecker/spellcheck_message_filter_platform_mac_unittest.cc
(right):

https://codereview.chromium.org/1884583003/diff/1/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellcheck_message_filter_platform_mac_unittest.cc:17:

If you stick with the namespace, mind doing a "using
spellcheck::SpellCheckResult" to minimize the reformatting?

https://codereview.chromium.org/1884583003/diff/1/chrome/browser/spellchecker...
File chrome/browser/spellchecker/spellchecker_session_bridge_android.cc (right):

https://codereview.chromium.org/1884583003/diff/1/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellchecker_session_bridge_android.cc:16: 
See above re: using.

[blundell, 2016-04-12 18:07:02]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2016-04-12 18:07:03]

https://codereview.chromium.org/1884583003/diff/1/chrome/browser/renderer_con...
File chrome/browser/renderer_context_menu/spelling_menu_observer.h (right):

https://codereview.chromium.org/1884583003/diff/1/chrome/browser/renderer_con...
chrome/browser/renderer_context_menu/spelling_menu_observer.h:24: namespace
spellcheck {
On 2016/04/12 17:56:51, groby wrote:
> Why the namespace? Chromium usually tries to avoid namespacing unless
necessary.

In //components, each individual component is supposed to be namespaced
(although not all components do that in practice), so this actually looks good
from that POV.

Chromium's policy as I understand it is that modules intended to be embedded
should be namespaced, but top-level embedders (such as //chrome) should not be
namespaced.

[groby-ooo-7-16, 2016-04-12 18:45:19]

On 2016/04/12 18:07:03, blundell wrote:
>
https://codereview.chromium.org/1884583003/diff/1/chrome/browser/renderer_con...
> File chrome/browser/renderer_context_menu/spelling_menu_observer.h (right):
> 
>
https://codereview.chromium.org/1884583003/diff/1/chrome/browser/renderer_con...
> chrome/browser/renderer_context_menu/spelling_menu_observer.h:24: namespace
> spellcheck {
> On 2016/04/12 17:56:51, groby wrote:
> > Why the namespace? Chromium usually tries to avoid namespacing unless
> necessary.
> 
> In //components, each individual component is supposed to be namespaced
> (although not all components do that in practice), so this actually looks good
> from that POV.
> 
> Chromium's policy as I understand it is that modules intended to be embedded
> should be namespaced, but top-level embedders (such as //chrome) should not be
> namespaced.

I stand corrected. (And wish we'd spell that out somewhere :)

Ed, please feel free to ignore the namespace comment, then. Sorry if I have
caused extra work.

[edbaker, 2016-04-12 19:12:20]

On 2016/04/12 18:45:19, groby wrote:
> On 2016/04/12 18:07:03, blundell wrote:
> >
>
https://codereview.chromium.org/1884583003/diff/1/chrome/browser/renderer_con...
> > File chrome/browser/renderer_context_menu/spelling_menu_observer.h (right):
> > 
> >
>
https://codereview.chromium.org/1884583003/diff/1/chrome/browser/renderer_con...
> > chrome/browser/renderer_context_menu/spelling_menu_observer.h:24: namespace
> > spellcheck {
> > On 2016/04/12 17:56:51, groby wrote:
> > > Why the namespace? Chromium usually tries to avoid namespacing unless
> > necessary.
> > 
> > In //components, each individual component is supposed to be namespaced
> > (although not all components do that in practice), so this actually looks
good
> > from that POV.
> > 
> > Chromium's policy as I understand it is that modules intended to be embedded
> > should be namespaced, but top-level embedders (such as //chrome) should not
be
> > namespaced.
> 
> I stand corrected. (And wish we'd spell that out somewhere :)
> 
> Ed, please feel free to ignore the namespace comment, then. Sorry if I have
> caused extra work.

That's no problem. I don't have a design document for the embedder dependencies
but I can look into producing one if you like, and I'm working on follow up
patches for more refactoring.

I can make more use of "using spellcheck::..." to reduce the amount of
reformatting if that's desirable in revised version of this patch

[groby-ooo-7-16, 2016-04-13 01:38:35]

On 2016/04/12 19:12:20, edbaker wrote:
> On 2016/04/12 18:45:19, groby wrote:
> > On 2016/04/12 18:07:03, blundell wrote:
> > >
> >
>
https://codereview.chromium.org/1884583003/diff/1/chrome/browser/renderer_con...
> > > File chrome/browser/renderer_context_menu/spelling_menu_observer.h
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1884583003/diff/1/chrome/browser/renderer_con...
> > > chrome/browser/renderer_context_menu/spelling_menu_observer.h:24:
namespace
> > > spellcheck {
> > > On 2016/04/12 17:56:51, groby wrote:
> > > > Why the namespace? Chromium usually tries to avoid namespacing unless
> > > necessary.
> > > 
> > > In //components, each individual component is supposed to be namespaced
> > > (although not all components do that in practice), so this actually looks
> good
> > > from that POV.
> > > 
> > > Chromium's policy as I understand it is that modules intended to be
embedded
> > > should be namespaced, but top-level embedders (such as //chrome) should
not
> be
> > > namespaced.
> > 
> > I stand corrected. (And wish we'd spell that out somewhere :)
> > 
> > Ed, please feel free to ignore the namespace comment, then. Sorry if I have
> > caused extra work.
> 
> That's no problem. I don't have a design document for the embedder
dependencies
> but I can look into producing one if you like, and I'm working on follow up
> patches for more refactoring.
> 
> I can make more use of "using spellcheck::..." to reduce the amount of
> reformatting if that's desirable in revised version of this patch

using... would be great, thank you. Meanwhile, I'm waiting on CLA feedback -
hopefully more tomorrow.

[cosmin.truta, 2016-04-13 19:25:07]

Description was changed from

==========
Create a spellcheck component that will allow any embedder to reuse common
functionality.

Patch by Ed Baker <edbaker@blackberry.com>

BUG=583616
==========

to

==========
Create a spellcheck component that will allow any embedder to reuse common
functionality.

Patch by Ed Baker <edbaker@blackberry.com>.

BUG=583616
==========

[groby-ooo-7-16, 2016-04-15 01:41:12]

On 2016/04/13 01:38:35, groby wrote:
> On 2016/04/12 19:12:20, edbaker wrote:
> > On 2016/04/12 18:45:19, groby wrote:
> > > On 2016/04/12 18:07:03, blundell wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1884583003/diff/1/chrome/browser/renderer_con...
> > > > File chrome/browser/renderer_context_menu/spelling_menu_observer.h
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1884583003/diff/1/chrome/browser/renderer_con...
> > > > chrome/browser/renderer_context_menu/spelling_menu_observer.h:24:
> namespace
> > > > spellcheck {
> > > > On 2016/04/12 17:56:51, groby wrote:
> > > > > Why the namespace? Chromium usually tries to avoid namespacing unless
> > > > necessary.
> > > > 
> > > > In //components, each individual component is supposed to be namespaced
> > > > (although not all components do that in practice), so this actually
looks
> > good
> > > > from that POV.
> > > > 
> > > > Chromium's policy as I understand it is that modules intended to be
> embedded
> > > > should be namespaced, but top-level embedders (such as //chrome) should
> not
> > be
> > > > namespaced.
> > > 
> > > I stand corrected. (And wish we'd spell that out somewhere :)
> > > 
> > > Ed, please feel free to ignore the namespace comment, then. Sorry if I
have
> > > caused extra work.
> > 
> > That's no problem. I don't have a design document for the embedder
> dependencies
> > but I can look into producing one if you like, and I'm working on follow up
> > patches for more refactoring.
> > 
> > I can make more use of "using spellcheck::..." to reduce the amount of
> > reformatting if that's desirable in revised version of this patch
> 
> using... would be great, thank you. Meanwhile, I'm waiting on CLA feedback -
> hopefully more tomorrow.

CLA feedback is here - it seems we changed our system, and that means you need
to sign a new CLA. (I'm terribly sorry about that extra work!)

Please go to  cla.developers.google.com and follow the instructions there, and
this should hopefully soon be done.

[Liam Quinn, 2016-04-15 21:09:51]

On 2016/04/15 01:41:12, groby wrote:
> CLA feedback is here - it seems we changed our system, and that means you need
> to sign a new CLA. (I'm terribly sorry about that extra work!)
> 
> Please go to  http://cla.developers.google.com and follow the instructions
there, and
> this should hopefully soon be done.

We'll need to get approval from our Legal department. Unfortunately, that may
take a while.

If anyone else wants to run with the bug in the meantime, please do feel free.

[blundell, 2016-08-16 08:03:45]

Is this CL obsolete now? If so, can you close it?

[Torne, 2016-08-16 11:51:48]

On 2016/08/16 08:03:45, blundell wrote:
> Is this CL obsolete now? If so, can you close it?

I'm going to close this as there was no response for a few months and
timvolodine@ is currently working on implementing spellcheck in webview (without
depending on this cl).

[groby-ooo-7-16, 2016-08-16 12:45:33]

On 2016/08/16 11:51:48, Torne wrote:
> On 2016/08/16 08:03:45, blundell wrote:
> > Is this CL obsolete now? If so, can you close it?
> 
> I'm going to close this as there was no response for a few months and
> timvolodine@ is currently working on implementing spellcheck in webview
(without
> depending on this cl).

+1