Enable variations restrict param for Java-side fetches.

Enable variations restrict param for Java-side fetches.

Prior to this change, when we fetched the variations seed from Java could, it was
not using the correct restrict param in the URL. This change plumbs it through.
See bug for more details.

BUG=708713
Review-Url: https://codereview.chromium.org/2804043002
Cr-Commit-Position: refs/heads/master@{#463033}
Committed: https://chromium.googlesource.com/chromium/src/+/0f6eda879ba71dde2e5e050892dfaf36d425f0ec

[Alexei Svitkine (slow), 2017-04-06 17:51:24]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 17:52:12]

Dry run: 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-06 18:20:37]

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

[commit-bot: I haz the power, 2017-04-06 18:20:38]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Alexei Svitkine (slow), 2017-04-06 18:40:44]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 18:42:08]

Dry run: 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-06 19:16:29]

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

[commit-bot: I haz the power, 2017-04-06 19:16:30]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Alexei Svitkine (slow), 2017-04-06 20:39:29]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 20:40:36]

Dry run: 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-06 22:16:06]

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

[commit-bot: I haz the power, 2017-04-06 22:16:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Alexei Svitkine (slow), 2017-04-07 15:15:52]

Description was changed from

==========
Enable variations restrict param for Java-side fetches.

BUG=708713
==========

to

==========
Enable variations restrict param for Java-side fetches.

Prior to this change, when we fetched the variations seed from Java could, it
was
not using the correct restrict param in the URL. This change plumbs it through.
See bug for more details.

BUG=708713
==========

[Alexei Svitkine (slow), 2017-04-07 15:29:11]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 15:29:25]

Dry run: 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-07 16:12:22]

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

[commit-bot: I haz the power, 2017-04-07 16:12:23]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-04-07 17:45:23]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 17:46:09]

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

[Alexei Svitkine (slow), 2017-04-07 18:16:25]

Patchset #1 (id:1) has been deleted

[Alexei Svitkine (slow), 2017-04-07 18:16:32]

Patchset #2 (id:40001) has been deleted

[Alexei Svitkine (slow), 2017-04-07 18:16:38]

Patchset #1 (id:20001) has been deleted

[Alexei Svitkine (slow), 2017-04-07 18:16:47]

Patchset #1 (id:60001) has been deleted

[Alexei Svitkine (slow), 2017-04-07 18:17:52]

asvitkine@chromium.org changed reviewers:
+ tedchoc@chromium.org

[commit-bot: I haz the power, 2017-04-07 18:23:53]

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

[commit-bot: I haz the power, 2017-04-07 18:23:55]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Alexei Svitkine (slow), 2017-04-07 18:45:31]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[Ted C, 2017-04-07 18:45:42]

https://codereview.chromium.org/2804043002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java
(right):

https://codereview.chromium.org/2804043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:75:
private String mRestrictMode;
final?

https://codereview.chromium.org/2804043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:78:
this.mRestrictMode = restrictMode;
no need for this. since the names are different.

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

https://codereview.chromium.org/2804043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:35:
getRestrictMode(context, new Callback<String>() {
should this use getRestrictModeValue?

It seems weird to me that mRestrictMode is being set in here and below.  I think
it would be cleaner to have it set in one place.  Maybe in getRestrictModeValue,
we wouldn't actually call getRestrictMode more than once and we'd return
mRestrictMode if it has ever been fetched (I think we'd need to keep a boolean
in that case since you could have null I presume or would it be the empty string
in which case you could get by with just mRestrictMode being != null).

https://codereview.chromium.org/2804043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:61:
public void getRestrictModeValue(Context context, final Callback<String>
callback) {
should this be final?

[commit-bot: I haz the power, 2017-04-07 18:46:11]

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

[Alexei Svitkine (slow), 2017-04-07 19:04:40]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 19:04:56]

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

[Ted C, 2017-04-07 19:13:43]

lgtm w/ long winded probably unnecessary comment

https://codereview.chromium.org/2804043002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java
(right):

https://codereview.chromium.org/2804043002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:53:
getRestrictMode(context, new Callback<String>() {
Ok...overthinking this even more (something that very likely could be done in a
follow up...or never if it doesn't matter).

If we want to restrict this to only call getRestrictMode once.  I think we would
need to support a queue of callbacks waiting on the initial response from
getRestrictMode.

private List<Callback<String>> mPendingRestrictModeCallbacks = new
ArrayList<>();

...

ThreadUtils.assertOnUiThread();
if (mRestrictMode != null) {
   ...
   return;
}

mPendingRestrictModeCallbacks.add(callback);
if (mPendingRestrictModeCallbacks.size() > 1) return;

getRestrictMode(context, new Callback<String>() {
    @Override
    public void onResult(String restrictMode) {
        ThreadUtils.assertOnUiThread();
        assert restrictMode != null;
        mRestrictMode = restrictMode;
        for (int i = 0; i < mPendingRestrictModeCallbacks.size(); i++) {
            mPendingRestrictModeCallbacks.get(i).onResult(restrictMode);
        }
        mPendingRestrictModeCallbacks.clear();
    }
});


Again...probably way more than needed and potentially more complicated to all.

[Alexei Svitkine (slow), 2017-04-07 19:18:53]

Thanks!

Will submit once I verify this is working as intended with a local build.

(One thing I realized is this does add an extra delay to the fetching code -
since it needs to get accounts from the system - hopefully that's generally
fast.)

https://codereview.chromium.org/2804043002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java
(right):

https://codereview.chromium.org/2804043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:75:
private String mRestrictMode;
On 2017/04/07 18:45:42, Ted C wrote:
> final?

Done.

https://codereview.chromium.org/2804043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:78:
this.mRestrictMode = restrictMode;
On 2017/04/07 18:45:41, Ted C wrote:
> no need for this. since the names are different.

Done.

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

https://codereview.chromium.org/2804043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:35:
getRestrictMode(context, new Callback<String>() {
On 2017/04/07 18:45:42, Ted C wrote:
> should this use getRestrictModeValue?
> 
> It seems weird to me that mRestrictMode is being set in here and below.  I
think
> it would be cleaner to have it set in one place.  Maybe in
getRestrictModeValue,
> we wouldn't actually call getRestrictMode more than once and we'd return
> mRestrictMode if it has ever been fetched (I think we'd need to keep a boolean
> in that case since you could have null I presume or would it be the empty
string
> in which case you could get by with just mRestrictMode being != null).

Done. Didn't need a boolean since all the current code uses empty string when
not set. I also expanded comments to explicitly call our that requirement
(non-null).

https://codereview.chromium.org/2804043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:61:
public void getRestrictModeValue(Context context, final Callback<String>
callback) {
On 2017/04/07 18:45:42, Ted C wrote:
> should this be final?

Done.

https://codereview.chromium.org/2804043002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java
(right):

https://codereview.chromium.org/2804043002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:53:
getRestrictMode(context, new Callback<String>() {
On 2017/04/07 19:13:43, Ted C wrote:
> Ok...overthinking this even more (something that very likely could be done in
a
> follow up...or never if it doesn't matter).
> 
> If we want to restrict this to only call getRestrictMode once.  I think we
would
> need to support a queue of callbacks waiting on the initial response from
> getRestrictMode.
> 
> private List<Callback<String>> mPendingRestrictModeCallbacks = new
> ArrayList<>();
> 
> ...
> 
> ThreadUtils.assertOnUiThread();
> if (mRestrictMode != null) {
>    ...
>    return;
> }
> 
> mPendingRestrictModeCallbacks.add(callback);
> if (mPendingRestrictModeCallbacks.size() > 1) return;
> 
> getRestrictMode(context, new Callback<String>() {
>     @Override
>     public void onResult(String restrictMode) {
>         ThreadUtils.assertOnUiThread();
>         assert restrictMode != null;
>         mRestrictMode = restrictMode;
>         for (int i = 0; i < mPendingRestrictModeCallbacks.size(); i++) {
>             mPendingRestrictModeCallbacks.get(i).onResult(restrictMode);
>         }
>         mPendingRestrictModeCallbacks.clear();
>     }
> });
> 
> 
> Again...probably way more than needed and potentially more complicated to all.

Right. I don't think the above is necessary at this point - since this will be
called: a) on FRE and b) on session start (i.e. app foregrounded & native
initialized).

The two are unlikely to overlap much in practice, so I don't think we need the
more complicated solution currently.

[Alexei Svitkine (slow), 2017-04-07 19:43:13]

Confirmed this to be working on a device. Submitting.

[Alexei Svitkine (slow), 2017-04-07 19:43:21]

The CQ bit was unchecked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2017-04-07 19:43:25]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2017-04-07 19:43:37]

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-07 19:54:43]

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

[commit-bot: I haz the power, 2017-04-07 19:54:45]

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

[Alexei Svitkine (slow), 2017-04-07 20:01:12]

On 2017/04/07 19:54:45, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Hmm, presubmit chokes on Java style check:


Traceback (most recent call last):
  File "/b/build/scripts/slave/.recipe_deps/depot_tools/presubmit_support.py",
line 1564, in <module>
    sys.exit(main())
  File "/b/build/scripts/slave/.recipe_deps/depot_tools/presubmit_support.py",
line 1548, in main
    options.dry_run)
  File "/b/build/scripts/slave/.recipe_deps/depot_tools/presubmit_support.py",
line 1291, in DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/b/build/scripts/slave/.recipe_deps/depot_tools/presubmit_support.py",
line 1201, in ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 2401, in CheckChangeOnCommit
  File "<string>", line 2153, in _CommonChecks
  File "<string>", line 1482, in _CheckJavaStyle
  File "/b/build/slave/linux/build/src/tools/android/checkstyle/checkstyle.py",
line 60, in RunCheckstyle
    formatted_checkstyle_output = FormatCheckstyleOutput(stdout)
  File "/b/build/slave/linux/build/src/tools/android/checkstyle/checkstyle.py",
line 21, in FormatCheckstyleOutput
    if 'Checkstyle ends with' in lines[-1]:
IndexError: list index out of range
step returned non-zero exit code: 1

However, this doesn't repro locally for me. "git cl presubmit" doesn't error and
the CL is "git cl format" clean.

I'll try hitting the cq box again, in case it's a flake... Otherwise, any ideas?

[Alexei Svitkine (slow), 2017-04-07 20:01:18]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2017-04-07 20:02:03]

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

[Alexei Svitkine (slow), 2017-04-07 20:03:38]

Looks like checkstyle was just updated by zpeng:

https://chromium.googlesource.com/chromium/src/+/dbce02f63421f7d446d6ef317b45...

Checking with them.

[commit-bot: I haz the power, 2017-04-07 20:12:17]

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

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

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

[Alexei Svitkine (slow), 2017-04-07 20:14:41]

Confirmed it's related to that change when I patch it in locally, it give that
error. Android Chrome DEPS are not at that point yet, so had to patch it in
rather than just sync.

zpeng@ is going to revert. Meanwhile, I'm putting PRESUBMIT=false on this CL
since it passes without previous PRESUBMIT revision.

[Alexei Svitkine (slow), 2017-04-07 20:14:57]

Description was changed from

==========
Enable variations restrict param for Java-side fetches.

Prior to this change, when we fetched the variations seed from Java could, it
was
not using the correct restrict param in the URL. This change plumbs it through.
See bug for more details.

BUG=708713
==========

to

==========
Enable variations restrict param for Java-side fetches.

Prior to this change, when we fetched the variations seed from Java could, it
was
not using the correct restrict param in the URL. This change plumbs it through.
See bug for more details.

BUG=708713
PRESUBMIT=false
==========

[Alexei Svitkine (slow), 2017-04-07 20:15:03]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2017-04-07 20:15:34]

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

[Alexei Svitkine (slow), 2017-04-07 20:16:15]

On 2017/04/07 20:14:41, Alexei Svitkine (slow) wrote:
> Confirmed it's related to that change when I patch it in locally, it give that
> error. Android Chrome DEPS are not at that point yet, so had to patch it in
> rather than just sync.
> 
> zpeng@ is going to revert. Meanwhile, I'm putting PRESUBMIT=false on this CL
> since it passes without previous PRESUBMIT revision.

Passes *with* the previous PRESUBMIT revision.

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

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

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

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

[Alexei Svitkine (slow), 2017-04-07 20:28:02]

PRESUBMIT=false apparently didn't work. Removing it and just waiting for the
revert to land: https://codereview.chromium.org/2808553002/

[Alexei Svitkine (slow), 2017-04-07 20:28:15]

Description was changed from

==========
Enable variations restrict param for Java-side fetches.

Prior to this change, when we fetched the variations seed from Java could, it
was
not using the correct restrict param in the URL. This change plumbs it through.
See bug for more details.

BUG=708713
PRESUBMIT=false
==========

to

==========
Enable variations restrict param for Java-side fetches.

Prior to this change, when we fetched the variations seed from Java could, it
was
not using the correct restrict param in the URL. This change plumbs it through.
See bug for more details.

BUG=708713
==========

[Alexei Svitkine (slow), 2017-04-07 22:29:15]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2017-04-07 22:29:48]

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-07 23:01:45]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491604155532740,
"parent_rev": "9d09b0aeddb1407f22981575e6b4fd8473830c5d", "commit_rev":
"0f6eda879ba71dde2e5e050892dfaf36d425f0ec"}

[commit-bot: I haz the power, 2017-04-07 23:18:00]

Description was changed from

==========
Enable variations restrict param for Java-side fetches.

Prior to this change, when we fetched the variations seed from Java could, it
was
not using the correct restrict param in the URL. This change plumbs it through.
See bug for more details.

BUG=708713
==========

to

==========
Enable variations restrict param for Java-side fetches.

Prior to this change, when we fetched the variations seed from Java could, it
was
not using the correct restrict param in the URL. This change plumbs it through.
See bug for more details.

BUG=708713

Review-Url: https://codereview.chromium.org/2804043002
Cr-Commit-Position: refs/heads/master@{#463033}
Committed:
https://chromium.googlesource.com/chromium/src/+/0f6eda879ba71dde2e5e050892df...
==========

[commit-bot: I haz the power, 2017-04-07 23:18:01]

Committed patchset #3 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/0f6eda879ba71dde2e5e050892df...