Hide ContentClient getters from embedders so that they they don't reuse content's embedder API. The…

Hide ContentClient getters from embedders so that they they don't reuse content's embedder API. The main reasons are that it makes content's embedder interfaces harder to change if they're reused by the embedder, and that its makes it confusing for embedder code when there are multiple ways to do the same thing.

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

[jam, 2013-04-03 22:11:38]

joth: src\android_webview
isherman: src\components\autofill

[Ilya Sherman, 2013-04-03 22:54:22]

Hmm, I'm looking at three different files: metrics_log.cc, autofill_country.cc,
and fingerprint.cc.  Each of these had a use of
|content::GetContentClient()->browser()->GetApplicationLocale()|, but each now
has different code replacing that.  Could you make these consistent?  I know
you're working on consolidating these APIs in your other CL, but at least for
now, I'd appreciate having one primary recommended approach that's used for each
of these files.

[jam, 2013-04-03 23:06:32]

On 2013/04/03 22:54:22, Ilya Sherman wrote:
> Hmm, I'm looking at three different files: metrics_log.cc,
autofill_country.cc,
> and fingerprint.cc.  Each of these had a use of
> |content::GetContentClient()->browser()->GetApplicationLocale()|, but each now
> has different code replacing that.  Could you make these consistent?  I know
> you're working on consolidating these APIs in your other CL, but at least for
> now, I'd appreciate having one primary recommended approach that's used for
each
> of these files.

I felt the same way too actually, and have been looking into this in parallel.
Now that that I see bug 149970 (get locale from profile), this basically means
that bug 225830 (get locale from one central place) is obsolete. It's
unfortunate that the autofill code had its own separate cache, as to fix things
now I will have to remove AutofillCountry::ApplicationLocale and instead plumb
the locale from the g_browser_process for now until it's per-profile.

[brettw, 2013-04-03 23:40:26]

lgtm

https://codereview.chromium.org/13409003/diff/121001/content/shell/shell_cont...
File content/shell/shell_content_renderer_client.cc (right):

https://codereview.chromium.org/13409003/diff/121001/content/shell/shell_cont...
content/shell/shell_content_renderer_client.cc:45: namespace {
Nit: blank line after this.

[joth, 2013-04-03 23:43:43]

lgtm / one suggestion.

https://codereview.chromium.org/13409003/diff/121001/android_webview/browser/...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/13409003/diff/121001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.cc:32: AwBrowserContext*
g_browser_context;
Rather than stash this as a global here, it would be better to move the
singleton instance to be a member of another singleton, e.g. AwMainDelegate, and
pass into AwContentBrowserClient as another c'tor param. 

It would also need passing into
AwBrowserDependencyFactoryImpl::InstallInstance() and then things like quota
manager would get it via
AwBrowserDependencyFactory::GetInstance()->GetBrowserContext().
(aside: AwBrowserDependencyFactory is mostly obsolete now we no longer have
egregious dependencies from android_webview to chrome, so maybe cleaner I just
delete it than try and clean it)

If you agree with the approach but don't want to pull more files/iterations into
this patch (it's already pretty large) feel free to put a TODO(joth): here for
it and ping me when you land this and I'm happy to follow up on that.

[Ilya Sherman, 2013-04-03 23:51:18]

https://codereview.chromium.org/13409003/diff/121001/components/autofill/brow...
File components/autofill/browser/autofill_country.cc (right):

https://codereview.chromium.org/13409003/diff/121001/components/autofill/brow...
components/autofill/browser/autofill_country.cc:677: application_locale_ =
base::i18n::GetConfiguredLocale();
Can you use g_browser_process->GetApplicationLocale() here?

https://codereview.chromium.org/13409003/diff/121001/components/autofill/brow...
File components/autofill/browser/risk/fingerprint.cc (right):

https://codereview.chromium.org/13409003/diff/121001/components/autofill/brow...
components/autofill/browser/risk/fingerprint.cc:321:
machine->set_browser_language(AutofillCountry::ApplicationLocale());
Can you use g_browser_process->GetApplicationLocale() here?

[jam, 2013-04-04 02:45:14]

https://codereview.chromium.org/13409003/diff/121001/components/autofill/brow...
File components/autofill/browser/autofill_country.cc (right):

https://codereview.chromium.org/13409003/diff/121001/components/autofill/brow...
components/autofill/browser/autofill_country.cc:677: application_locale_ =
base::i18n::GetConfiguredLocale();
On 2013/04/03 23:51:18, Ilya Sherman wrote:
> Can you use g_browser_process->GetApplicationLocale() here?

no, since src\components can't depend on src\chrome.
https://codereview.chromium.org/13488009/ removes these uses, but I can't figure
out how to remove the use in address.cc. either Get/SetRawInfo need to take a
locale, or perhaps the locale should just be stored on these classes instead of
passing it through all these different locations?

[jam, 2013-04-04 02:46:09]

https://codereview.chromium.org/13409003/diff/121001/android_webview/browser/...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/13409003/diff/121001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.cc:32: AwBrowserContext*
g_browser_context;
On 2013/04/03 23:43:44, joth wrote:
> Rather than stash this as a global here, it would be better to move the
> singleton instance to be a member of another singleton, e.g. AwMainDelegate,
and
> pass into AwContentBrowserClient as another c'tor param. 
> 
> It would also need passing into
> AwBrowserDependencyFactoryImpl::InstallInstance() and then things like quota
> manager would get it via
> AwBrowserDependencyFactory::GetInstance()->GetBrowserContext().
> (aside: AwBrowserDependencyFactory is mostly obsolete now we no longer have
> egregious dependencies from android_webview to chrome, so maybe cleaner I just
> delete it than try and clean it)
> 
> If you agree with the approach but don't want to pull more files/iterations
into
> this patch (it's already pretty large) feel free to put a TODO(joth): here for
> it and ping me when you land this and I'm happy to follow up on that.
> 

That would be great if you can help with that, thanks.

[Ilya Sherman, 2013-04-04 04:05:35]

LGTM

https://codereview.chromium.org/13409003/diff/121001/components/autofill/brow...
File components/autofill/browser/autofill_country.cc (right):

https://codereview.chromium.org/13409003/diff/121001/components/autofill/brow...
components/autofill/browser/autofill_country.cc:677: application_locale_ =
base::i18n::GetConfiguredLocale();
On 2013/04/04 02:45:15, jam wrote:
> On 2013/04/03 23:51:18, Ilya Sherman wrote:
> > Can you use g_browser_process->GetApplicationLocale() here?
> 
> no, since src\components can't depend on src\chrome.

Ah, right, we've been componentized.  *sigh*

> https://codereview.chromium.org/13488009/ removes these uses, but I can't
figure
> out how to remove the use in address.cc. either Get/SetRawInfo need to take a
> locale, or perhaps the locale should just be stored on these classes instead
of
> passing it through all these different locations?

We can discuss this more on the other CL, but the short answer is that the TODO
in Address::SetRawInfo() should be fixed, i.e. we shouldn't try to fix up data
being saved via SetRawInfo().  This requires careful changes to the Sync code,
though, which is why it's still a TODO, rather than a TO-DONE.

[jam, 2013-04-04 17:58:35]

https://codereview.chromium.org/13409003/diff/121001/components/autofill/brow...
File components/autofill/browser/autofill_country.cc (right):

https://codereview.chromium.org/13409003/diff/121001/components/autofill/brow...
components/autofill/browser/autofill_country.cc:677: application_locale_ =
base::i18n::GetConfiguredLocale();
On 2013/04/04 04:05:36, Ilya Sherman wrote:
> On 2013/04/04 02:45:15, jam wrote:
> > On 2013/04/03 23:51:18, Ilya Sherman wrote:
> > > Can you use g_browser_process->GetApplicationLocale() here?
> > 
> > no, since src\components can't depend on src\chrome.
> 
> Ah, right, we've been componentized.  *sigh*
> 
> > https://codereview.chromium.org/13488009/ removes these uses, but I can't
> figure
> > out how to remove the use in address.cc. either Get/SetRawInfo need to take
a
> > locale, or perhaps the locale should just be stored on these classes instead
> of
> > passing it through all these different locations?
> 
> We can discuss this more on the other CL, but the short answer is that the
TODO
> in Address::SetRawInfo() should be fixed, i.e. we shouldn't try to fix up data
> being saved via SetRawInfo().  This requires careful changes to the Sync code,
> though, which is why it's still a TODO, rather than a TO-DONE.

are you planning on fixing this soon? I had looked through this and seeing sync
code, didn't feel like touching that code as I'm not familiar with it.

The problem is if we leave AutofillCountry::ApplicationLocale cache behind then
it will be used by other places. we can change the call sites in address.cc to
go to ICU directly, however that has the tradeoff that it'll be the only code
that will call to ICU to get the default language, and that seems like a pattern
that we don't want to spread as well.