Implement HttpUserAgentSettings delegate for Android WebView

Implement HttpUserAgentSettings delegate for Android WebView

Instead of using a static accept language and user agent,
we set a HttpUserAgentSettings delegate to allow those
values to change dynamically.

BUG=35049
Committed: https://crrev.com/67e9830ff10d26bf8840598dde988e4af9581fde
Cr-Commit-Position: refs/heads/master@{#341931}

[ctserng, 2015-06-09 19:00:27]

tserng@amazon.com changed reviewers:
+ torne@chromium.org

[ctserng, 2015-06-09 19:00:27]

Hello,

Here's a proposed patch for point #1 in:

https://code.google.com/p/chromium/issues/detail?id=35049#c25

Thanks,
Chris

[ctserng, 2015-06-12 13:52:22]

tserng@amazon.com changed reviewers:
+ boliu@chromium.org

[ctserng, 2015-06-12 13:52:22]

+boliu for an extra set of eyes.

[Torne, 2015-06-12 14:00:24]

Does this actually work in a real app when the locale change is initiated by the
android framework? Not sure how that works and whether it's equivalent to using
Locale. (would need to test when the app handles locale change with or without
an activity relaunch).

Also, does this end up checking the locale for every request? I suspect we'd
ideally want to do this less frequently; perhaps when we create a webview (to
catch the activity relaunch case) and when the configuration change event is
fired? Not sure what the cost of this is.

[ctserng, 2015-06-12 14:41:16]

On 2015/06/12 at 14:00:24, torne wrote:
> Does this actually work in a real app when the locale change is initiated by
the android framework? Not sure how that works and whether it's equivalent to
using Locale. (would need to test when the app handles locale change with or
without an activity relaunch).

Yes it does.  This was checked in the following scenario.

1. Open http://myhttp.info/ in a WebView
2. Look at "Accept Language"
3. Go to Settings... Language & Input... Language
4. Change the language
5. Navigate back to WebView
6. Reload http://myhttp.info/ and see that "Accept Language" has changed.

> 
> Also, does this end up checking the locale for every request? I suspect we'd
ideally want to do this less frequently; perhaps when we create a webview (to
catch the activity relaunch case) and when the configuration change event is
fired? Not sure what the cost of this is.

I'm not sure how to quantify the cost of getting the locale, but I can see if
there is a way to implement this w/out calling it on every request.

If we solve this, will probably need to fix it in chromecast as well.

https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/browser...

[boliu, 2015-06-12 15:31:57]

Yeah I share the concern for polling locale on every request. Those methods are
const for a reason.

I think you can listen to locale change from
http://developer.android.com/reference/android/content/ComponentCallbacks.htm...

torne can review this one

[boliu, 2015-06-12 15:32:11]

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

[Torne, 2015-06-12 15:41:43]

On 2015/06/12 14:41:16, ctserng wrote:
> On 2015/06/12 at 14:00:24, torne wrote:
> > Does this actually work in a real app when the locale change is initiated by
> the android framework? Not sure how that works and whether it's equivalent to
> using Locale. (would need to test when the app handles locale change with or
> without an activity relaunch).
> 
> Yes it does.  This was checked in the following scenario.
> 
> 1. Open http://myhttp.info/ in a WebView
> 2. Look at "Accept Language"
> 3. Go to Settings... Language & Input... Language
> 4. Change the language
> 5. Navigate back to WebView
> 6. Reload http://myhttp.info/ and see that "Accept Language" has changed.

You'll need to at least test this with an activity that relaunches on locale
change and one that doesn't (i.e. that has configChanges="locale" in the
manifest).

> > 
> > Also, does this end up checking the locale for every request? I suspect we'd
> ideally want to do this less frequently; perhaps when we create a webview (to
> catch the activity relaunch case) and when the configuration change event is
> fired? Not sure what the cost of this is.
> 
> I'm not sure how to quantify the cost of getting the locale, but I can see if
> there is a way to implement this w/out calling it on every request.
> 
> If we solve this, will probably need to fix it in chromecast as well.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/browser...

[ctserng, 2015-06-18 18:30:24]

It's looking pretty involved to plumb locale configuration change to the right
location to cache it.  Still looking into it.

[ctserng, 2015-06-23 15:33:33]

So I put up a prototype along the lines of what I'm thinking.

It seems that AwBrowserContext is the right place to store this locale, however
there currently is no linkage between the native and java object.

Seems like it might make sense to make a patch first to link these (and
correspondingly move the aw_browser_context.cc file into native/)?

Furthermore, I would modify AwContents.java to initialize and cache the locale
and only update on locale changes.

I just wanted to put something up before doing further work.

Thanks!

[Torne, 2015-06-29 13:14:38]

On 2015/06/23 15:33:33, ctserng wrote:
> So I put up a prototype along the lines of what I'm thinking.
> 
> It seems that AwBrowserContext is the right place to store this locale,
however
> there currently is no linkage between the native and java object.
> 
> Seems like it might make sense to make a patch first to link these (and
> correspondingly move the aw_browser_context.cc file into native/)?

Since the native AwBrowserContext is an implementation of a content browser
interface I think we want to keep it in browser, not native. I'm not sure what
the nicest way to deal with this is. Technically they are both singletons, so
having a link isn't critical..

> Furthermore, I would modify AwContents.java to initialize and cache the locale
> and only update on locale changes.
> 
> I just wanted to put something up before doing further work.
> 
> Thanks!

[ctserng, 2015-07-02 16:48:49]

Ok, so I'm going to see if there is a different way to plumb this (maybe through
JniDependencyFactory?).

The other issue is if we hook the OnConfigurationChanged handler, then the
original test I supplied no longer works (as it's a programmatic change to
locale).  I assume this is acceptable as the main use case is if the user
changes the locale via settings.

Still need to verify the case where the activity is relaunched.

[ctserng, 2015-07-02 17:50:29]

Seems like simplest way is to create a static AwContents method that allows us
to access the current locale.

[ctserng, 2015-07-02 21:37:14]

Exposed AwContents static method for retrieving locale.  However since
android_webview/browser can't access native, tried plumbing this through jni
dependency factory.  

Removed the test as the new implementation doesn't handle programmatic setLocale
changes.

Tested this implementation with both activity being restarted and not restarted.

[ctserng, 2015-07-13 14:29:44]

torne,

You mind taking a look at what I have so far.

Thanks!

[Torne, 2015-07-15 14:56:00]

On 2015/07/13 14:29:44, ctserng wrote:
> torne,
> 
> You mind taking a look at what I have so far.
> 
> Thanks!

Sorry; have been on vacation. I'll try to take a look at this this week.

[Torne, 2015-07-17 13:33:19]

Seems generally reasonable, I have some style nits and also one issue:

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

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:142: AwLocaleManager*
g_locale_manager = NULL;
This should be a scoped_ptr belonging to AwContentBrowserClient; globals
shouldn't be having their lifetime managed by an object's constructor/destructor
even if that object is a singleton.

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
File android_webview/browser/aw_locale_manager.cc (right):

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
android_webview/browser/aw_locale_manager.cc:9:
AwLocaleManager::~AwLocaleManager() {}
You can inline this safely since this type is just an interface.

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
File android_webview/browser/aw_locale_manager.h (right):

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
android_webview/browser/aw_locale_manager.h:13: class AwLocaleManager {
This should be DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
File android_webview/browser/net/aw_http_user_agent_settings.cc (right):

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
android_webview/browser/net/aw_http_user_agent_settings.cc:16:
last_aw_accept_language_ =
Maybe just do this lazily to avoid duplicating the code?

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
File android_webview/browser/net/aw_http_user_agent_settings.h (right):

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
android_webview/browser/net/aw_http_user_agent_settings.h:21: explicit
AwHttpUserAgentSettings();
explicit is only for 1-arg constructors.

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
android_webview/browser/net/aw_http_user_agent_settings.h:22: // Must be called
on the IO thread.
Explicitly requiring this to be destructed on a different thread than it's
created on seems really weird. It's generally preferred for objects to be used
only on a single thread where possible.

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.h:74:
scoped_ptr<AwHttpUserAgentSettings> aw_http_user_agent_settings_;
Should be a pointer to net::HttpUserAgentSettings (the interface type)

https://codereview.chromium.org/1172093002/diff/80001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1172093002/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:615:
public void onConfigurationChanged(Configuration configuration) {
Hm, so there's a bit of a problem here potentially: we only have our component
callbacks registered during the time when we are attached to a view hierarchy.
This means that we will miss locale changes that occur while the webview is
detached, and we'll never notice locale changes at all for apps which don't
attach the webview (some apps use it just to draw in an "offscreen" mode).

We wouldn't want to call through all these layers up to java for every network
request either, though. We might have to accept the latter as a limitation, but
perhaps we should recheck the locale when reattached to the view hierarchy? I'm
not sure if this will ever matter in practise but it seems possible that the
embedding activity might detach the webview on config change (so it can keep the
webview alive while recreating its layouts) - I'm not 100% sure that this method
will be called in that case, the order of operations is non-obvious..

[ctserng, 2015-07-17 16:51:15]

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

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:142: AwLocaleManager*
g_locale_manager = NULL;
On 2015/07/17 at 13:33:18, Torne wrote:
> This should be a scoped_ptr belonging to AwContentBrowserClient; globals
shouldn't be having their lifetime managed by an object's constructor/destructor
even if that object is a singleton.

Ok, the reason its global is to access it through the static function
AwContentBrowserClient::GetAcceptLangsImpl().  A similar construct is used in
aw_browser_context.cc.

https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/br...

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
File android_webview/browser/aw_locale_manager.cc (right):

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
android_webview/browser/aw_locale_manager.cc:9:
AwLocaleManager::~AwLocaleManager() {}
On 2015/07/17 at 13:33:18, Torne wrote:
> You can inline this safely since this type is just an interface.

Will do.

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
File android_webview/browser/aw_locale_manager.h (right):

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
android_webview/browser/aw_locale_manager.h:13: class AwLocaleManager {
On 2015/07/17 at 13:33:18, Torne wrote:
> This should be DISALLOW_COPY_AND_ASSIGN.

Will do.

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
File android_webview/browser/net/aw_http_user_agent_settings.cc (right):

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
android_webview/browser/net/aw_http_user_agent_settings.cc:16:
last_aw_accept_language_ =
On 2015/07/17 at 13:33:18, Torne wrote:
> Maybe just do this lazily to avoid duplicating the code?

Will do.

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
File android_webview/browser/net/aw_http_user_agent_settings.h (right):

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
android_webview/browser/net/aw_http_user_agent_settings.h:21: explicit
AwHttpUserAgentSettings();
On 2015/07/17 at 13:33:19, Torne wrote:
> explicit is only for 1-arg constructors.

Will do.

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
android_webview/browser/net/aw_http_user_agent_settings.h:22: // Must be called
on the IO thread.
On 2015/07/17 at 13:33:19, Torne wrote:
> Explicitly requiring this to be destructed on a different thread than it's
created on seems really weird. It's generally preferred for objects to be used
only on a single thread where possible.

I understand this, but the life cycle of AwURLRequestContextGetter seems to
imply that the object is created on the UI thread but then exists on the IO
thread.

This code is templated from chrome_http_user_agent_settings.h

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/1172093002/diff/80001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.h:74:
scoped_ptr<AwHttpUserAgentSettings> aw_http_user_agent_settings_;
On 2015/07/17 at 13:33:19, Torne wrote:
> Should be a pointer to net::HttpUserAgentSettings (the interface type)

Will do.

https://codereview.chromium.org/1172093002/diff/80001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1172093002/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:615:
public void onConfigurationChanged(Configuration configuration) {
On 2015/07/17 at 13:33:19, Torne wrote:
> Hm, so there's a bit of a problem here potentially: we only have our component
callbacks registered during the time when we are attached to a view hierarchy.
This means that we will miss locale changes that occur while the webview is
detached, and we'll never notice locale changes at all for apps which don't
attach the webview (some apps use it just to draw in an "offscreen" mode).
> 
> We wouldn't want to call through all these layers up to java for every network
request either, though. We might have to accept the latter as a limitation, but
perhaps we should recheck the locale when reattached to the view hierarchy? I'm
not sure if this will ever matter in practise but it seems possible that the
embedding activity might detach the webview on config change (so it can keep the
webview alive while recreating its layouts) - I'm not 100% sure that this method
will be called in that case, the order of operations is non-obvious..

Good point.  I'll see if it's possible to handle the reattach case.

[Torne, 2015-07-24 10:55:39]

Not sure if you were ready for review on this or not (rietveld doesn't email
when you upload a new patch, if you didn't know) but I did anyway.

Generally looks reasonable but we really need some test coverage on this
somehow; not sure what the best way for that would be though..

This still makes a JNI call for every request because the cached locale is
stored on the Java side. Is it possible to cache that on the native side without
a lot of trouble?

https://codereview.chromium.org/1172093002/diff/140001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1172093002/diff/140001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:619: if
(!sCurrentLocale.equals(newLocale)) {
May not be worth checking if they're equal here since it doesn't really matter?
You check in two of the three places you set sCurrentLocale but not the third,
so should be consistent one way or the other at least.

[ctserng, 2015-07-24 18:16:49]

On 2015/07/24 at 10:55:39, torne wrote:
> Not sure if you were ready for review on this or not (rietveld doesn't email
when you upload a new patch, if you didn't know) but I did anyway.
> 
> Generally looks reasonable but we really need some test coverage on this
somehow; not sure what the best way for that would be though..
> 
> This still makes a JNI call for every request because the cached locale is
stored on the Java side. Is it possible to cache that on the native side without
a lot of trouble?
 
Cached the call on the native side.

As to testing, do you have any suggestions?  Doesn't seem easy to do
automatically.

[ctserng, 2015-07-24 18:17:22]

https://codereview.chromium.org/1172093002/diff/140001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1172093002/diff/140001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:619: if
(!sCurrentLocale.equals(newLocale)) {
On 2015/07/24 at 10:55:39, Torne wrote:
> May not be worth checking if they're equal here since it doesn't really
matter? You check in two of the three places you set sCurrentLocale but not the
third, so should be consistent one way or the other at least.

The caching was left over from a previous patch where it mattered.  I've kept
the caching as with the new implementation it will avoid a call into native if
the locale hasn't changed.

[Torne, 2015-07-28 13:04:07]

Yeah, I can't think of an easy way to write a test for the full functionality
here. Maybe we could have a test that at least verifies that the useragent and
accept-lang are correct for the current locale, though? I don't think we cover
that in chromium currently and it would at least establish that this change
didn't regress anything.

[ctserng, 2015-08-04 17:55:21]

On 2015/07/28 at 13:04:07, torne wrote:
> Yeah, I can't think of an easy way to write a test for the full functionality
here. Maybe we could have a test that at least verifies that the useragent and
accept-lang are correct for the current locale, though? I don't think we cover
that in chromium currently and it would at least establish that this change
didn't regress anything.

Added an instrumentation test to just check that accept lang matches the default
locale.

There is already a user agent test that checks for some formatting in the UA
string.  I think this is sufficient to catch if this breaks user agent settings.

[Torne, 2015-08-05 12:15:48]

Thanks; LGTM.

[ctserng, 2015-08-05 13:40:13]

tserng@amazon.com changed reviewers:
+ yfriedman@chromium.org

[ctserng, 2015-08-05 13:40:13]

+ yfriedman as owner for LocaleUtils.java changes

[Yaron, 2015-08-05 13:46:21]

lgtm

[ctserng, 2015-08-05 16:28:20]

The CQ bit was checked by tserng@amazon.com

[ctserng, 2015-08-05 16:28:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org, torne@chromium.org
Link to the patchset: https://codereview.chromium.org/1172093002/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-05 16:29:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1172093002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1172093002/200001

[commit-bot: I haz the power, 2015-08-05 17:04:17]

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

[commit-bot: I haz the power, 2015-08-05 17:04:18]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[ctserng, 2015-08-05 17:49:46]

The CQ bit was checked by tserng@amazon.com

[commit-bot: I haz the power, 2015-08-05 17:51:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1172093002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1172093002/200001

[commit-bot: I haz the power, 2015-08-05 19:01:51]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-08-05 19:02:31]

Patchset 11 (id:??) landed as
https://crrev.com/67e9830ff10d26bf8840598dde988e4af9581fde
Cr-Commit-Position: refs/heads/master@{#341931}