[Origin Trials] Support updates of disabled token list

[Origin Trials] Support updates of disabled token list

This CL adds support for updating the list of disabled tokens, via the
existing component updater. Like other origin trials component updates,
the list is plumbed through to the ChromeOriginTrialPolicy via prefs
and the browser command line.

For now, the tokens will be pushed down as a list of base64-encoded
token signatures. Next step is to use a bloom filter, to be more
space-efficient (as in the design doc).

BUG=582042
Review-Url: https://codereview.chromium.org/2733023002
Cr-Commit-Position: refs/heads/master@{#457547}
Committed: https://chromium.googlesource.com/chromium/src/+/dfec22ae07134f50601cf55a828b10ddac091ac0

[chasej, 2017-03-06 15:23:10]

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

[commit-bot: I haz the power, 2017-03-06 15:23:38]

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-03-06 15:58:20]

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

[commit-bot: I haz the power, 2017-03-06 15:58:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chasej, 2017-03-06 16:40:32]

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

[commit-bot: I haz the power, 2017-03-06 16:40:58]

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-03-06 16:54:30]

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

[commit-bot: I haz the power, 2017-03-06 16:54:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chasej, 2017-03-06 17:32:42]

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

[commit-bot: I haz the power, 2017-03-06 17:32:53]

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-03-06 18:19:44]

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

[commit-bot: I haz the power, 2017-03-06 18:19:45]

Dry run: This issue passed the CQ dry run.

[chasej, 2017-03-06 20:10:16]

chasej@chromium.org changed reviewers:
+ iclelland@chromium.org

[chasej, 2017-03-06 20:10:17]

iclelland, please take a look.

I'm still working on adding tests for Chrome main, to verify that stored prefs
get propagated onto the command line. The rest of changes are good to go, so no
point blocking on the last bit of tests.

[chasej, 2017-03-07 21:15:34]

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

[commit-bot: I haz the power, 2017-03-07 21:16:39]

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-03-07 21:42:12]

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

[commit-bot: I haz the power, 2017-03-07 21:42:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chasej, 2017-03-08 02:54:53]

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

[commit-bot: I haz the power, 2017-03-08 02:55:27]

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-03-08 04:02:53]

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

[commit-bot: I haz the power, 2017-03-08 04:02:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chasej, 2017-03-08 19:00:27]

chasej@chromium.org changed reviewers:
+ mek@chromium.org
- iclelland@chromium.org

[chasej, 2017-03-08 19:00:28]

mek, please take a look.

(Removing iclelland as a reviewer, who is OOO until next week).

[chasej, 2017-03-09 20:49:43]

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

[commit-bot: I haz the power, 2017-03-09 20:50:02]

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-03-09 21:49:05]

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

[commit-bot: I haz the power, 2017-03-09 21:49:06]

Dry run: This issue passed the CQ dry run.

[chasej, 2017-03-10 21:09:11]

mek, ping, please take look.

[Marijn Kruisselbrink, 2017-03-14 05:54:42]

sorry for the delay, looks good, just wondering if the tests could be written
somewhat nicer

https://codereview.chromium.org/2733023002/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_origin_trials_browsertest.cc (right):

https://codereview.chromium.org/2733023002/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_origin_trials_browsertest.cc:84: if
(command_line->HasSwitch(switches::kOriginTrialPublicKey)) {
rather than having this if, would it make sense to just change the previous
EXPECT_TRUE into an ASSERT_TRUE?

https://codereview.chromium.org/2733023002/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_origin_trials_browsertest.cc:120:
IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,
Rather than having all these essentially identical tests, just with different
data, would it make sense to write this as parameterized tests (using
IN_PROC_BROWSER_TEST_P/INSTANTIATE_TEST_CASE_P? I assume that would still work
correctly even with the PRE_ tests, although I've never tried that.

[chasej, 2017-03-14 19:52:32]

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

[commit-bot: I haz the power, 2017-03-14 19:53:20]

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

[chasej, 2017-03-14 20:00:08]

mek, please take another look.

https://codereview.chromium.org/2733023002/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_origin_trials_browsertest.cc (right):

https://codereview.chromium.org/2733023002/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_origin_trials_browsertest.cc:84: if
(command_line->HasSwitch(switches::kOriginTrialPublicKey)) {
On 2017/03/14 05:54:42, Marijn Kruisselbrink wrote:
> rather than having this if, would it make sense to just change the previous
> EXPECT_TRUE into an ASSERT_TRUE?

I created a helper method GetCommandLineSwitch(), since there were multiple
lines of boilerplate code to get and assert. I can't use ASSERT_TRUE
because the helper method returns a value. I did remove the extra
"if command_line->HasSwitch" check, as it will just return an empty string
if not found.

https://codereview.chromium.org/2733023002/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_origin_trials_browsertest.cc:120:
IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,
On 2017/03/14 05:54:42, Marijn Kruisselbrink wrote:
> Rather than having all these essentially identical tests, just with different
> data, would it make sense to write this as parameterized tests (using
> IN_PROC_BROWSER_TEST_P/INSTANTIATE_TEST_CASE_P? I assume that would still work
> correctly even with the PRE_ tests, although I've never tried that.

Done. Thanks for the suggestion. I had to add separate fixtures, but
the result is still much shorter and cleaner.

[Marijn Kruisselbrink, 2017-03-14 20:13:35]

lgtm

[chasej, 2017-03-14 20:39:54]

chasej@chromium.org changed reviewers:
+ cpu@chromium.org

[chasej, 2017-03-14 20:39:55]

cpu, please take a look at chrome/browser/*.

[commit-bot: I haz the power, 2017-03-14 21:12:42]

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

[commit-bot: I haz the power, 2017-03-14 21:12:43]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-03-15 15:53:40]

iclelland@chromium.org changed reviewers:
+ iclelland@chromium.org

[iclelland, 2017-03-15 15:53:41]

Drive-by review with my drafts from last week before I went OOO -- feel free to
ignore as necessary :)

https://codereview.chromium.org/2733023002/diff/40001/chrome/common/origin_tr...
File chrome/common/origin_trials/chrome_origin_trial_policy_unittest.cc (right):

https://codereview.chromium.org/2733023002/diff/40001/chrome/common/origin_tr...
chrome/common/origin_trials/chrome_origin_trial_policy_unittest.cc:61:
"zX9ztEn1/+/"
This formatting is really odd -- did clang-format assume that the "/" was
punctuation?

https://codereview.chromium.org/2733023002/diff/40001/chrome/common/origin_tr...
chrome/common/origin_trials/chrome_origin_trial_policy_unittest.cc:108:
std::string three_disabled_tokens_;
Another way to do this might be to make a utility method that takes a
std::vector<uint8_t[]> and does the base64 encoding and string concatenation.
That way we wouldn't have to worry about keeping the base64 signatures in sync,
or constructing and storing a string for every combination of things we want to
test.

[cpu_(ooo_6.6-7.5), 2017-03-16 17:14:42]

only looked at chrome/browser lgtm

to note an old, old grievance about passing too much data in the command line,
which limited on windows. Maybe one day we'll address this. eventually.

[chasej, 2017-03-16 17:19:54]

On 2017/03/16 17:14:42, cpu wrote:
> only looked at chrome/browser lgtm
> 
> to note an old, old grievance about passing too much data in the command line,
> which limited on windows. Maybe one day we'll address this. eventually.

Thanks for the review.

Based on your comments, I'm guessing we don't yet have an alternate approach? If
there is an approved replacement you could point at, I could tackle that in a
followup CL.

[chasej, 2017-03-16 19:48:01]

On 2017/03/15 15:53:41, iclelland wrote:
> Drive-by review with my drafts from last week before I went OOO -- feel free
to
> ignore as necessary :)
> 
>
https://codereview.chromium.org/2733023002/diff/40001/chrome/common/origin_tr...
> File chrome/common/origin_trials/chrome_origin_trial_policy_unittest.cc
(right):
> 
>
https://codereview.chromium.org/2733023002/diff/40001/chrome/common/origin_tr...
> chrome/common/origin_trials/chrome_origin_trial_policy_unittest.cc:61:
> "zX9ztEn1/+/"
> This formatting is really odd -- did clang-format assume that the "/" was
> punctuation?
> 
I did auto-format, so I assume this is result of treating it as punctuation.
I decided not to fight the result of running the format command.

>
https://codereview.chromium.org/2733023002/diff/40001/chrome/common/origin_tr...
> chrome/common/origin_trials/chrome_origin_trial_policy_unittest.cc:108:
> std::string three_disabled_tokens_;
> Another way to do this might be to make a utility method that takes a
> std::vector<uint8_t[]> and does the base64 encoding and string concatenation.
> That way we wouldn't have to worry about keeping the base64 signatures in
sync,
> or constructing and storing a string for every combination of things we want
to
> test.

Yes, I had started down that path. I found the result to be more complicated,
primarily because you needed multiple utility methods to get rid of the string
variables (one to build a list, another to stringify a single signature, .etc).
There might be an easier approach, but I also had to switch to vectors for the 
signature constants (e.g. kToken1Signature). So, I figured the complexity
wasn't worth saving the storage of a few strings for the few tests.

[chasej, 2017-03-16 19:48:15]

The CQ bit was checked by chasej@chromium.org

[commit-bot: I haz the power, 2017-03-16 19:49:21]

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-03-16 20:59:08]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489693695790990,
"parent_rev": "d6fe18d8eb8c01d1f420727005653e24dee8a431", "commit_rev":
"dfec22ae07134f50601cf55a828b10ddac091ac0"}

[commit-bot: I haz the power, 2017-03-16 20:59:51]

Description was changed from

==========
[Origin Trials] Support updates of disabled token list

This CL adds support for updating the list of disabled tokens, via the
existing component updater. Like other origin trials component updates,
the list is plumbed through to the ChromeOriginTrialPolicy via prefs
and the browser command line.

For now, the tokens will be pushed down as a list of base64-encoded
token signatures. Next step is to use a bloom filter, to be more
space-efficient (as in the design doc).

BUG=582042
==========

to

==========
[Origin Trials] Support updates of disabled token list

This CL adds support for updating the list of disabled tokens, via the
existing component updater. Like other origin trials component updates,
the list is plumbed through to the ChromeOriginTrialPolicy via prefs
and the browser command line.

For now, the tokens will be pushed down as a list of base64-encoded
token signatures. Next step is to use a bloom filter, to be more
space-efficient (as in the design doc).

BUG=582042

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

[commit-bot: I haz the power, 2017-03-16 20:59:52]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/dfec22ae07134f50601cf55a828b...