Set User-Agent and Accept-Language request headers for PWS requests

Set User-Agent and Accept-Language request headers for PWS requests

The HTTP request headers normally used by Chrome for Android are
generated in the native layer. We do not want to load native when the
Physical Web is resolving URLs in the background, so we cannot access
these values directly. Instead, they are reconstructed from values
accessible from the Java layer.

BUG=607660
Committed: https://crrev.com/6bf0bb56d7536accdddeaf0429de7e619953e367
Cr-Commit-Position: refs/heads/master@{#398120}

[mattreynolds, 2016-05-20 22:09:21]

Description was changed from

==========
Set User-Agent and Accept-Language request headers for PWS requests

The HTTP request headers normally used by Chrome for Android are
generated in the native layer. We do not want to load native when the
Physical Web is resolving URLs in the background, so we cannot access
these values directly. Instead, they are reconstructed from values
accessible from the Java layer.

BUG=607660
==========

to

==========
Set User-Agent and Accept-Language request headers for PWS requests

The HTTP request headers normally used by Chrome for Android are
generated in the native layer. We do not want to load native when the
Physical Web is resolving URLs in the background, so we cannot access
these values directly. Instead, they are reconstructed from values
accessible from the Java layer.

BUG=607660
==========

[mattreynolds, 2016-05-20 22:09:21]

mattreynolds@chromium.org changed reviewers:
+ cco3@chromium.org

[cco3, 2016-05-23 18:10:03]

On 2016/05/20 22:09:21, mattreynolds wrote:
> mailto:mattreynolds@chromium.org changed reviewers:
> + mailto:cco3@chromium.org

LGTM, thanks for doing this!

[mattreynolds, 2016-05-24 17:57:17]

mattreynolds@chromium.org changed reviewers:
+ blundell@chromium.org, nyquist@chromium.org

[mattreynolds, 2016-05-24 17:57:18]

blundell@chromium.org: Please review changes in components/*

nyquist@chromium.org: Please review changes in chrome/android/*

[blundell, 2016-05-25 09:03:09]

On 2016/05/24 17:57:18, mattreynolds wrote:
> mailto:blundell@chromium.org: Please review changes in components/*
> 
> mailto:nyquist@chromium.org: Please review changes in chrome/android/*

I'm not at all familiar with the strings magic happening here. Could you get
someone who's knowledgeable about that area to review and then I'll rubberstamp
once that's done?

[mattreynolds, 2016-05-25 17:19:36]

mattreynolds@chromium.org changed reviewers:
+ sdefresne@chromium.org

[mattreynolds, 2016-05-25 17:23:05]

Hi Sylvain, please take a look at components/*.  I'm trying to make
components_locale_settings strings (specifically IDS_ACCEPT_LANGUAGES)
accessible from Java without JNI calls.

[sdefresne, 2016-05-25 17:29:56]

components/ lgtm

[nyquist, 2016-05-25 22:02:48]

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java
(right):

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:48:
private static String sUserAgent;
These are lazily initialized, so could you mention that here?

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:52:
private Context mContext;
Could you add a comment as to the lifetime of this context?

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:251:
language = "he";
It's a little bit awkward to reassign the input param. Could you instead have a
local variable that defaults to the input param?

Or, maybe just use a switch?
switch (language) {
  case "iw": return "he" + "-" + region;
  case "ji": return "yi" + "-" + region;
  case "in": return "id" + "-" + region;
  default: return language + "-" + region;
}

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:264:
* chrome/browser/android/preferences/pref_service_bridge.cc
Should you update that code to refer to this code?

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:280:
if (!acceptLanguages.contains(languageTag)) {
Could you flip this, and just return acceptLanguages here? Then you don't need
the indent, and the code still seems clean to read.

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:284:
if (!acceptLanguages.contains(language + ",") &&
!acceptLanguages.endsWith(language)) {
Is there ever any chance of this happening:

acceptLanguages = "coolfoo,notsobar"
and language being "foo", and thee this code would trigger for "foo,"?

Or is there a guarantee for the length of all the accept languages?
And if there is, could you mention that here in a comment so new readers won't
have the same naive question as me?

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:297:
* net/http/http_util.cc
Do we want a back-reference?

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java
(right):

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java:50:
assertEquals("he-XX", PwsClientImpl.makeLanguageTag("iw", "XX"));
How about adding one more test for the non-exceptional case?

https://codereview.chromium.org/1989963003/diff/40001/components/strings/BUIL...
File components/strings/BUILD.gn (right):

https://codereview.chromium.org/1989963003/diff/40001/components/strings/BUIL...
components/strings/BUILD.gn:307:
android_components_locale_settings_java_resources = [
How much does this increase the binary size?

[mattreynolds, 2016-05-26 00:31:43]

mattreynolds@chromium.org changed reviewers:
+ asanka@chromium.org

[mattreynolds, 2016-05-26 00:31:45]

asanka@chromium.org: Please review changes in net/*

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java
(right):

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:48:
private static String sUserAgent;
On 2016/05/25 22:02:48, nyquist wrote:
> These are lazily initialized, so could you mention that here?

Done.

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:52:
private Context mContext;
On 2016/05/25 22:02:48, nyquist wrote:
> Could you add a comment as to the lifetime of this context?

Done. (also made it final)

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:251:
language = "he";
On 2016/05/25 22:02:48, nyquist wrote:
> It's a little bit awkward to reassign the input param. Could you instead have
a
> local variable that defaults to the input param?
> 
> Or, maybe just use a switch?
> switch (language) {
>   case "iw": return "he" + "-" + region;
>   case "ji": return "yi" + "-" + region;
>   case "in": return "id" + "-" + region;
>   default: return language + "-" + region;
> }

Done.

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:264:
* chrome/browser/android/preferences/pref_service_bridge.cc
On 2016/05/25 22:02:48, nyquist wrote:
> Should you update that code to refer to this code?

Done.

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:280:
if (!acceptLanguages.contains(languageTag)) {
On 2016/05/25 22:02:48, nyquist wrote:
> Could you flip this, and just return acceptLanguages here? Then you don't need
> the indent, and the code still seems clean to read.

Done.

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:284:
if (!acceptLanguages.contains(language + ",") &&
!acceptLanguages.endsWith(language)) {
On 2016/05/25 22:02:48, nyquist wrote:
> Is there ever any chance of this happening:
> 
> acceptLanguages = "coolfoo,notsobar"
> and language being "foo", and thee this code would trigger for "foo,"?
> 
> Or is there a guarantee for the length of all the accept languages?
> And if there is, could you mention that here in a comment so new readers won't
> have the same naive question as me?

It could fail to append the language tag correctly for an arbitrary list of
language tags, but it should never happen here since the strings defined in
Chrome's localized resource files follow a strict format:

https://code.google.com/p/chromium/codesearch#search/&q=id=%5C%22IDS_ACCEPT_L...

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:297:
* net/http/http_util.cc
On 2016/05/25 22:02:48, nyquist wrote:
> Do we want a back-reference?

Done.

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java
(right):

https://codereview.chromium.org/1989963003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/PwsClientImplTest.java:50:
assertEquals("he-XX", PwsClientImpl.makeLanguageTag("iw", "XX"));
On 2016/05/25 22:02:48, nyquist wrote:
> How about adding one more test for the non-exceptional case?

Done.

https://codereview.chromium.org/1989963003/diff/40001/components/strings/BUIL...
File components/strings/BUILD.gn (right):

https://codereview.chromium.org/1989963003/diff/40001/components/strings/BUIL...
components/strings/BUILD.gn:307:
android_components_locale_settings_java_resources = [
On 2016/05/25 22:02:48, nyquist wrote:
> How much does this increase the binary size?

Seems to add ~3KB.

Chrome.apk Release, with this CL: 73115980 bytes
Chrome.apk Release, without this CL: 73113048 bytes

[nyquist, 2016-05-26 22:38:30]

lgtm

[mattreynolds, 2016-05-26 22:45:58]

Hi asanka@, PTAL

[asanka, 2016-05-27 16:57:59]

https://codereview.chromium.org/1989963003/diff/60001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/1989963003/diff/60001/net/http/http_util.cc#n...
net/http/http_util.cc:761: // The two implementations should be kept in sync.
Let's remove this comment. I don't think its feasible to keep C++ and Java login
in sync via comments like these.

Is the PWS a purely Java implementation?

[mattreynolds, 2016-05-27 17:24:11]

https://codereview.chromium.org/1989963003/diff/60001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/1989963003/diff/60001/net/http/http_util.cc#n...
net/http/http_util.cc:761: // The two implementations should be kept in sync.
On 2016/05/27 16:57:59, asanka wrote:
> Let's remove this comment. I don't think its feasible to keep C++ and Java
login
> in sync via comments like these.
> 
> Is the PWS a purely Java implementation?

The Physical Web client in Chrome for Android is by necessity implemented in
Java.  We don't want to load the native layer to resolve URLs or show
notifications when Chrome is in the background.

[asanka, 2016-06-01 20:10:27]

https://codereview.chromium.org/1989963003/diff/60001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/1989963003/diff/60001/net/http/http_util.cc#n...
net/http/http_util.cc:761: // The two implementations should be kept in sync.
On 2016/05/27 at 17:24:11, mattreynolds wrote:
> On 2016/05/27 16:57:59, asanka wrote:
> > Let's remove this comment. I don't think its feasible to keep C++ and Java
login
> > in sync via comments like these.
> > 
> > Is the PWS a purely Java implementation?
> 
> The Physical Web client in Chrome for Android is by necessity implemented in
Java.  We don't want to load the native layer to resolve URLs or show
notifications when Chrome is in the background.

Understood. Thanks for removing the comment.

Would it be possible to determine the UA and Accept-Language headers while the
native portion is loaded and persist it somewhere for the Java code to access?
That way you won't need to worry about the languages preferences mismatches due
to duplicated code going out of sync. I imagine it would reduce the maintenance
burden and reduce the risk of subtle mysterious breakages down the line that
will probably remain unfixed.

https://codereview.chromium.org/1989963003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java
(right):

https://codereview.chromium.org/1989963003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:308:
* @param languageList A comma-delimited list of language codes containing no
whitespace.
Nit: It's important to note that the list of languages should be in decreasing
order of preference. The function just assigns decreasing quality values to each
successive language in the list until it reaches 0.2.

[mattreynolds, 2016-06-01 22:02:49]

https://codereview.chromium.org/1989963003/diff/60001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/1989963003/diff/60001/net/http/http_util.cc#n...
net/http/http_util.cc:761: // The two implementations should be kept in sync.
On 2016/06/01 20:10:26, asanka wrote:
> On 2016/05/27 at 17:24:11, mattreynolds wrote:
> > On 2016/05/27 16:57:59, asanka wrote:
> > > Let's remove this comment. I don't think its feasible to keep C++ and Java
> login
> > > in sync via comments like these.
> > > 
> > > Is the PWS a purely Java implementation?
> > 
> > The Physical Web client in Chrome for Android is by necessity implemented in
> Java.  We don't want to load the native layer to resolve URLs or show
> notifications when Chrome is in the background.
> 
> Understood. Thanks for removing the comment.
> 
> Would it be possible to determine the UA and Accept-Language headers while the
> native portion is loaded and persist it somewhere for the Java code to access?
> That way you won't need to worry about the languages preferences mismatches
due
> to duplicated code going out of sync. I imagine it would reduce the
maintenance
> burden and reduce the risk of subtle mysterious breakages down the line that
> will probably remain unfixed.

Yes, we could store it in SharedPreferences and update at a time when we know
native is loaded (eg, at Chrome startup).  However this would cause the header
to be out of sync whenever the user changes the system locale without restarting
Chrome.

https://codereview.chromium.org/1989963003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java
(right):

https://codereview.chromium.org/1989963003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClientImpl.java:308:
* @param languageList A comma-delimited list of language codes containing no
whitespace.
On 2016/06/01 20:10:26, asanka wrote:
> Nit: It's important to note that the list of languages should be in decreasing
> order of preference. The function just assigns decreasing quality values to
each
> successive language in the list until it reaches 0.2.

Note added.

[mattreynolds, 2016-06-06 17:34:04]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-06-06 17:34:04]

The patchset sent to the CQ was uploaded after l-g-t-m from cco3@chromium.org,
sdefresne@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1989963003/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-06 17:34:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1989963003/120001

[commit-bot: I haz the power, 2016-06-06 21:00:39]

Description was changed from

==========
Set User-Agent and Accept-Language request headers for PWS requests

The HTTP request headers normally used by Chrome for Android are
generated in the native layer. We do not want to load native when the
Physical Web is resolving URLs in the background, so we cannot access
these values directly. Instead, they are reconstructed from values
accessible from the Java layer.

BUG=607660
==========

to

==========
Set User-Agent and Accept-Language request headers for PWS requests

The HTTP request headers normally used by Chrome for Android are
generated in the native layer. We do not want to load native when the
Physical Web is resolving URLs in the background, so we cannot access
these values directly. Instead, they are reconstructed from values
accessible from the Java layer.

BUG=607660
==========

[commit-bot: I haz the power, 2016-06-06 21:00:40]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-06 21:02:43]

Description was changed from

==========
Set User-Agent and Accept-Language request headers for PWS requests

The HTTP request headers normally used by Chrome for Android are
generated in the native layer. We do not want to load native when the
Physical Web is resolving URLs in the background, so we cannot access
these values directly. Instead, they are reconstructed from values
accessible from the Java layer.

BUG=607660
==========

to

==========
Set User-Agent and Accept-Language request headers for PWS requests

The HTTP request headers normally used by Chrome for Android are
generated in the native layer. We do not want to load native when the
Physical Web is resolving URLs in the background, so we cannot access
these values directly. Instead, they are reconstructed from values
accessible from the Java layer.

BUG=607660

Committed: https://crrev.com/6bf0bb56d7536accdddeaf0429de7e619953e367
Cr-Commit-Position: refs/heads/master@{#398120}
==========

[commit-bot: I haz the power, 2016-06-06 21:02:45]

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