Refactor dependency on Profile to BrowserContext in spell check.

Refactor dependency on Profile to BrowserContext in spell check. Continuation of issue 23639002.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223699

[msmith.v3, 2013-09-10 18:52:40]

New issue under a venture3systems.com email account. Continuation of issue
23639002.

[Lei Zhang, 2013-09-10 18:57:27]

lgtm

I removed the "contributed by ..." part of the CL description. The commit should
be under your name.

[commit-bot: I haz the power, 2013-09-10 19:01:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/megan.smith@venture3systems.com/238680...

[groby-ooo-7-16, 2013-09-10 19:09:51]

A few more nits - maybe with the next CL :)

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
File chrome/browser/spellchecker/spellcheck_message_filter.cc (right):

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
chrome/browser/spellchecker/spellcheck_message_filter.cc:20: #include
"content/public/browser/browser_context.h"
Nit: Don't need this (in most places). We're almost never dereferencing
BrowserContext, so no need to include the header

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right):

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
chrome/browser/spellchecker/spellcheck_service_browsertest.cc:65:
GetInstance()->GetServiceForBrowserContext(context, false));
Does this get the context somewhere? Can't see that, might need to add it

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
chrome/browser/spellchecker/spellcheck_service_browsertest.cc:72:
SpellcheckServiceFactory::GetForContext(GetProfile());
Use context.

[msmith.v3, 2013-09-10 19:27:49]

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right):

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
chrome/browser/spellchecker/spellcheck_service_browsertest.cc:65:
GetInstance()->GetServiceForBrowserContext(context, false));
On 2013/09/10 19:09:51, groby wrote:
> Does this get the context somewhere? Can't see that, might need to add it

Yep - copy-paste error. Sure that's going to break something. Will fix that
momentarily.

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
chrome/browser/spellchecker/spellcheck_service_browsertest.cc:72:
SpellcheckServiceFactory::GetForContext(GetProfile());
On 2013/09/10 19:09:51, groby wrote:
> Use context.

What's the proper way to grab the context inside one of these types of tests? I
see a way to grab the profile; should I just rename the GetProfile() method in
this case?

[commit-bot: I haz the power, 2013-09-10 19:33:15]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[groby-ooo-7-16, 2013-09-10 20:48:06]

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right):

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
chrome/browser/spellchecker/spellcheck_service_browsertest.cc:72:
SpellcheckServiceFactory::GetForContext(GetProfile());
GetProfile() is fine - I was just assuming you'd assign it to context and reuse
that here :)

On 2013/09/10 19:27:50, msmith.v3 wrote:
> On 2013/09/10 19:09:51, groby wrote:
> > Use context.
> 
> What's the proper way to grab the context inside one of these types of tests?
I
> see a way to grab the profile; should I just rename the GetProfile() method in
> this case?

[msmith.v3, 2013-09-10 21:02:34]

On 2013/09/10 20:48:06, groby wrote:
>
https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
> File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right):
> 
>
https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
> chrome/browser/spellchecker/spellcheck_service_browsertest.cc:72:
> SpellcheckServiceFactory::GetForContext(GetProfile());
> GetProfile() is fine - I was just assuming you'd assign it to context and
reuse
> that here :)
> 
> On 2013/09/10 19:27:50, msmith.v3 wrote:
> > On 2013/09/10 19:09:51, groby wrote:
> > > Use context.
> > 
> > What's the proper way to grab the context inside one of these types of
tests?
> I
> > see a way to grab the profile; should I just rename the GetProfile() method
in
> > this case?

Ok. Is chromium_builder_tests the proper target I should be building to make
sure I don't break things again or is there a smaller target?

[groby-ooo-7-16, 2013-09-10 21:10:09]

anything ending in browsertests.cc is in browser_tests, unit tests are in
unit_tests. Alternatively, just use "git cl try" and let the trybots do the
compiling for you.

I'm not sure if chrome_builder_tests covers everything else, but it's worth a
try. (I usually rely on the CQ after running the obvious candidates like
unit/browser tests)

[msmith.v3, 2013-09-11 19:25:07]

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right):

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
chrome/browser/spellchecker/spellcheck_service_browsertest.cc:65:
GetInstance()->GetServiceForBrowserContext(context, false));
On 2013/09/10 19:27:50, msmith.v3 wrote:
> On 2013/09/10 19:09:51, groby wrote:
> > Does this get the context somewhere? Can't see that, might need to add it
> 
> Yep - copy-paste error. Sure that's going to break something. Will fix that
> momentarily.

This turned out to be problematic. The reason for the original
SpellcheckServiceFactory::GetForProfileWithoutCreating method was
GetServiceForBrowserContext() is protected - I can't call it here. This line
seems to only be here to make debugging the test a little easier - the real test
is the actual creation of the service and the event trigger. I see two options -
remove this line and the ASSERT_EQ, or put back the
SpellcheckServiceFactory::GetForContextWithoutCreating method.just for this
test. Thoughts?

[groby-ooo-7-16, 2013-09-12 21:35:52]

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right):

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
chrome/browser/spellchecker/spellcheck_service_browsertest.cc:65:
GetInstance()->GetServiceForBrowserContext(context, false));
Expose it in spellcheck_factory.h via
FRIEND_TEST_ALL_PREFIXED(SpellcheckServiceBrowserTest, DeleteCorruptedBDICT); 

(goes into the private: section)

On 2013/09/11 19:25:07, msmith.v3 wrote:
> On 2013/09/10 19:27:50, msmith.v3 wrote:
> > On 2013/09/10 19:09:51, groby wrote:
> > > Does this get the context somewhere? Can't see that, might need to add it
> > 
> > Yep - copy-paste error. Sure that's going to break something. Will fix that
> > momentarily.
> 
> This turned out to be problematic. The reason for the original
> SpellcheckServiceFactory::GetForProfileWithoutCreating method was
> GetServiceForBrowserContext() is protected - I can't call it here. This line
> seems to only be here to make debugging the test a little easier - the real
test
> is the actual creation of the service and the event trigger. I see two options
-
> remove this line and the ASSERT_EQ, or put back the
> SpellcheckServiceFactory::GetForContextWithoutCreating method.just for this
> test. Thoughts?

[msmith.v3, 2013-09-13 17:02:14]

Think I covered all the issues. Thanks for your patience and feedback. Hopefully
my next attempt will go more smoothly as I learn more about working with this
code base.

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
File chrome/browser/spellchecker/spellcheck_message_filter.cc (right):

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
chrome/browser/spellchecker/spellcheck_message_filter.cc:20: #include
"content/public/browser/browser_context.h"
On 2013/09/10 19:09:51, groby wrote:
> Nit: Don't need this (in most places). We're almost never dereferencing
> BrowserContext, so no need to include the header

Done.

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right):

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
chrome/browser/spellchecker/spellcheck_service_browsertest.cc:65:
GetInstance()->GetServiceForBrowserContext(context, false));
On 2013/09/12 21:35:53, groby wrote:
> Expose it in spellcheck_factory.h via
> FRIEND_TEST_ALL_PREFIXED(SpellcheckServiceBrowserTest, DeleteCorruptedBDICT); 
> 
> (goes into the private: section)
> 
> On 2013/09/11 19:25:07, msmith.v3 wrote:
> > On 2013/09/10 19:27:50, msmith.v3 wrote:
> > > On 2013/09/10 19:09:51, groby wrote:
> > > > Does this get the context somewhere? Can't see that, might need to add
it
> > > 
> > > Yep - copy-paste error. Sure that's going to break something. Will fix
that
> > > momentarily.
> > 
> > This turned out to be problematic. The reason for the original
> > SpellcheckServiceFactory::GetForProfileWithoutCreating method was
> > GetServiceForBrowserContext() is protected - I can't call it here. This line
> > seems to only be here to make debugging the test a little easier - the real
> test
> > is the actual creation of the service and the event trigger. I see two
options
> -
> > remove this line and the ASSERT_EQ, or put back the
> > SpellcheckServiceFactory::GetForContextWithoutCreating method.just for this
> > test. Thoughts?
> 

Done.

https://codereview.chromium.org/23868013/diff/1/chrome/browser/spellchecker/s...
chrome/browser/spellchecker/spellcheck_service_browsertest.cc:72:
SpellcheckServiceFactory::GetForContext(GetProfile());
On 2013/09/10 20:48:07, groby wrote:
> GetProfile() is fine - I was just assuming you'd assign it to context and
reuse
> that here :)
> 
> On 2013/09/10 19:27:50, msmith.v3 wrote:
> > On 2013/09/10 19:09:51, groby wrote:
> > > Use context.
> > 
> > What's the proper way to grab the context inside one of these types of
tests?
> I
> > see a way to grab the profile; should I just rename the GetProfile() method
in
> > this case?
> 

Done.

[groby-ooo-7-16, 2013-09-13 17:50:46]

LGTM!

Thank you for your patience during the review (the first one is always the worst
:), and welcome to Chromium!

[msmith.v3, 2013-09-16 13:35:02]

On 2013/09/13 17:50:46, groby wrote:
> LGTM!
> 
> Thank you for your patience during the review (the first one is always the
worst
> :), and welcome to Chromium!

Thanks! The win_rel:swarm_triggered failure - that looks like something
unrelated to my code or am I not reading it correctly?

[please use gerrit instead, 2013-09-16 15:04:41]

Yes, it's unrelated. Go ahead with a commit.

[commit-bot: I haz the power, 2013-09-16 19:29:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/megan.smith@venture3systems.com/238680...

[commit-bot: I haz the power, 2013-09-16 23:19:31]

Failed to apply patch for AUTHORS:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file AUTHORS
  Hunk #1 FAILED at 302.
  1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej

Patch:       AUTHORS
Index: AUTHORS
===================================================================
--- AUTHORS	(revision 222337)
+++ AUTHORS	(working copy)
@@ -302,3 +302,4 @@
 The Chromium Authors <*@chromium.org>
 The MathWorks, Inc. <binod.pant@mathworks.com>
 Torchmobile Inc.
+Venture 3 Systems LLC <*@venture3systems.com>

[Lei Zhang, 2013-09-16 23:30:01]

On 2013/09/16 23:19:31, I haz the power (commit-bot) wrote:
> Failed to apply patch for AUTHORS:
> While running patch -p0 --forward --force --no-backup-if-mismatch;
>   patching file AUTHORS
>   Hunk #1 FAILED at 302.
>   1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej
> 
> Patch:       AUTHORS
> Index: AUTHORS
> ===================================================================
> --- AUTHORS	(revision 222337)
> +++ AUTHORS	(working copy)
> @@ -302,3 +302,4 @@
>  The Chromium Authors <mailto:*@chromium.org>
>  The MathWorks, Inc. <mailto:binod.pant@mathworks.com>
>  Torchmobile Inc.
> +Venture 3 Systems LLC <mailto:*@venture3systems.com>

Sorry, this is my fault. I forgot this was in the queue. You'll have to rebase
and try again.

[msmith.v3, 2013-09-17 13:52:05]

Rebased.

[commit-bot: I haz the power, 2013-09-17 18:27:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/megan.smith@venture3systems.com/238680...

[commit-bot: I haz the power, 2013-09-17 21:29:17]

Change committed as 223699

[Lei Zhang, 2013-09-17 21:51:54]

On 2013/09/17 21:29:17, I haz the power (commit-bot) wrote:
> Change committed as 223699

woohoo!