Add plumbing for accessing latest country in variations service in Java

Add plumbing for accessing latest country in variations service in Java

Checking what the latest country is according to variations service is
an important signal for various features but this is currently not
exposed in variations_service code. This adds the required plumbing from
variations_service to the corresponding Android side classes.

BUG=713381
Review-Url: https://codereview.chromium.org/2828093002
Cr-Commit-Position: refs/heads/master@{#466107}
Committed: https://chromium.googlesource.com/chromium/src/+/d855bfda144dd7d99d9b39f779837c3e45d6b1e9

[Yusuf, 2017-04-19 21:39:22]

yusufo@chromium.org changed reviewers:
+ asvitkine@chromium.org, dfalcantara@chromium.org

[Yusuf, 2017-04-19 21:39:22]

Still verifying that this does what I was hoping it would do, but sending this
out for plumbing sanity check.

[Alexei Svitkine (slow), 2017-04-19 21:42:57]

https://codereview.chromium.org/2828093002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java
(right):

https://codereview.chromium.org/2828093002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:73:
* @return The latest country according to the current variations state.
Expand comment to mention this could return null if not known.

https://codereview.chromium.org/2828093002/diff/1/chrome/browser/android/metr...
File chrome/browser/android/metrics/variations_session.cc (right):

https://codereview.chromium.org/2828093002/diff/1/chrome/browser/android/metr...
chrome/browser/android/metrics/variations_session.cc:51: env,
variations_service->GetLatestCountry());
Can you add checking for empty string here and return null in that case too? So
that null is a consistent result when the value isn't known.

https://codereview.chromium.org/2828093002/diff/1/components/variations/servi...
File components/variations/service/variations_service.h (right):

https://codereview.chromium.org/2828093002/diff/1/components/variations/servi...
components/variations/service/variations_service.h:131: // Returns what
variations will consider to be the latest country.
Mention that this returns the empty string if not available.

[Yusuf, 2017-04-19 22:16:46]

https://codereview.chromium.org/2828093002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java
(right):

https://codereview.chromium.org/2828093002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:73:
* @return The latest country according to the current variations state.
On 2017/04/19 21:42:57, Alexei Svitkine (slow) wrote:
> Expand comment to mention this could return null if not known.

Done.

https://codereview.chromium.org/2828093002/diff/1/chrome/browser/android/metr...
File chrome/browser/android/metrics/variations_session.cc (right):

https://codereview.chromium.org/2828093002/diff/1/chrome/browser/android/metr...
chrome/browser/android/metrics/variations_session.cc:51: env,
variations_service->GetLatestCountry());
On 2017/04/19 21:42:57, Alexei Svitkine (slow) wrote:
> Can you add checking for empty string here and return null in that case too?
So
> that null is a consistent result when the value isn't known.

Done.

https://codereview.chromium.org/2828093002/diff/1/components/variations/servi...
File components/variations/service/variations_service.h (right):

https://codereview.chromium.org/2828093002/diff/1/components/variations/servi...
components/variations/service/variations_service.h:131: // Returns what
variations will consider to be the latest country.
On 2017/04/19 21:42:57, Alexei Svitkine (slow) wrote:
> Mention that this returns the empty string if not available.

Done.

[gone, 2017-04-20 16:31:45]

lgtm

https://codereview.chromium.org/2828093002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java
(right):

https://codereview.chromium.org/2828093002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:80:
protected native String nativeGetLatestCountry();
does this thing need to be marked protected static?

[Alexei Svitkine (slow), 2017-04-20 16:35:41]

lgtm

https://codereview.chromium.org/2828093002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivitySessionTracker.java
(right):

https://codereview.chromium.org/2828093002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivitySessionTracker.java:86:
* @return The latest country according to the current variations state.
Update this comment to mention null too.

[gone, 2017-04-20 16:38:37]

https://codereview.chromium.org/2828093002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivitySessionTracker.java
(right):

https://codereview.chromium.org/2828093002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivitySessionTracker.java:86:
* @return The latest country according to the current variations state.
On 2017/04/20 16:35:41, Alexei Svitkine (slow) wrote:
> Update this comment to mention null too.

Oh, good point... Mark this @Nullable.

[Yusuf, 2017-04-20 18:16:13]

https://codereview.chromium.org/2828093002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivitySessionTracker.java
(right):

https://codereview.chromium.org/2828093002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivitySessionTracker.java:86:
* @return The latest country according to the current variations state.
On 2017/04/20 16:38:37, slow (dfalcantara) wrote:
> On 2017/04/20 16:35:41, Alexei Svitkine (slow) wrote:
> > Update this comment to mention null too.
> 
> Oh, good point... Mark this @Nullable.

Done.

https://codereview.chromium.org/2828093002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java
(right):

https://codereview.chromium.org/2828093002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:80:
protected native String nativeGetLatestCountry();
On 2017/04/20 16:31:45, slow (dfalcantara) wrote:
> does this thing need to be marked protected static?

Doesn't really make much difference with the current usage, I think? I mostly
followed the precedent here. StartVariationsSession also has the same structure.

[Yusuf, 2017-04-20 18:16:27]

The CQ bit was checked by yusufo@chromium.org

[Yusuf, 2017-04-20 18:16:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2828093002/#ps40001
(title: "nits")

[commit-bot: I haz the power, 2017-04-20 18:17:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 20:24:09]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492712187572780,
"parent_rev": "93dc8900a21b04a878eaaa3ea46a9456d9b1e305", "commit_rev":
"d855bfda144dd7d99d9b39f779837c3e45d6b1e9"}

[commit-bot: I haz the power, 2017-04-20 20:24:19]

Description was changed from

==========
Add plumbing for accessing latest country in variations service in Java

Checking what the latest country is according to variations service is
an important signal for various features but this is currently not
exposed in variations_service code. This adds the required plumbing from
variations_service to the corresponding Android side classes.

BUG=713381
==========

to

==========
Add plumbing for accessing latest country in variations service in Java

Checking what the latest country is according to variations service is
an important signal for various features but this is currently not
exposed in variations_service code. This adds the required plumbing from
variations_service to the corresponding Android side classes.

BUG=713381

Review-Url: https://codereview.chromium.org/2828093002
Cr-Commit-Position: refs/heads/master@{#466107}
Committed:
https://chromium.googlesource.com/chromium/src/+/d855bfda144dd7d99d9b39f77983...
==========

[commit-bot: I haz the power, 2017-04-20 20:24:20]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d855bfda144dd7d99d9b39f77983...