Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(172)

Issue 2123323004: Simplify OriginTrialContext and the way it validates tokens. (Closed)

Created:
4 years, 5 months ago by Marijn Kruisselbrink
Modified:
4 years, 5 months ago
CC:
asvitkine+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-bindings_chromium.org, chasej+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, jam, mlamouri+watch-content_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Simplify OriginTrialContext and the way it validates tokens. The way OriginTrialContext and the WebTrialTokenValidator API was designed made a lot of sense when the bindings were still done in such a way that only when a feature is first used we validate if there are tokens for that feature. But now that we check every feature in every context the current design is both not very efficient, and also needlessly complex. This simplifies things a lot by validating tokens as soon as AddToken is called (as all tokens will be validated anyway). This way we only have to validate each token once rather than once for each feature. Also gets rid of the old histograms as they weren't very meaningful anymore: The FeatureEnabled was logged regardless of if a feature was actually being used on a page, making the data hard to reason about. And the MessageGenerated histogram was 100% NotRequested after the bindings changes, making it not useful at all. BUG=627942 Committed: https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90 Cr-Commit-Position: refs/heads/master@{#407056}

Patch Set 1 #

Patch Set 2 #

Total comments: 8

Patch Set 3 : rebase #

Patch Set 4 : address comments #

Patch Set 5 : filter empty tokens in addTokens, and DCHECK in validateToken #

Total comments: 4

Patch Set 6 : actually skip empty tokens in addTokens #

Total comments: 2

Patch Set 7 : mark enum as obsolete #

Unified diffs Side-by-side diffs Delta from patch set Stats (+171 lines, -435 lines) Patch
M content/common/origin_trials/trial_token.h View 1 1 chunk +4 lines, -6 lines 0 comments Download
M content/common/origin_trials/trial_token.cc View 1 chunk +2 lines, -6 lines 0 comments Download
M content/common/origin_trials/trial_token_unittest.cc View 1 chunk +5 lines, -20 lines 0 comments Download
M content/common/origin_trials/trial_token_validator.h View 1 1 chunk +3 lines, -1 line 0 comments Download
M content/common/origin_trials/trial_token_validator.cc View 1 chunk +12 lines, -10 lines 0 comments Download
M content/common/origin_trials/trial_token_validator_unittest.cc View 1 chunk +23 lines, -22 lines 0 comments Download
M content/renderer/origin_trials/web_trial_token_validator_impl.h View 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/origin_trials/web_trial_token_validator_impl.cc View 2 chunks +8 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/build/scripts/templates/OriginTrials.cpp.tmpl View 1 chunk +3 lines, -21 lines 0 comments Download
M third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl View 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h View 2 chunks +5 lines, -23 lines 0 comments Download
M third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp View 1 2 3 4 5 3 chunks +27 lines, -171 lines 0 comments Download
M third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp View 5 chunks +27 lines, -132 lines 0 comments Download
M third_party/WebKit/Source/core/origin_trials/testing/InternalsFrobulate.cpp View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/public/platform/WebOriginTrialTokenStatus.h View 1 chunk +6 lines, -8 lines 0 comments Download
M third_party/WebKit/public/platform/WebTrialTokenValidator.h View 1 1 chunk +4 lines, -3 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 5 chunks +34 lines, -0 lines 0 comments Download

Messages

Total messages: 44 (22 generated)
Marijn Kruisselbrink
See also the thread I started on experimentation-dev about this. Given how the bindings changed ...
4 years, 5 months ago (2016-07-08 00:47:01 UTC) #2
iclelland
I like this; it simplifies a lot of the code in good ways. Only a ...
4 years, 5 months ago (2016-07-13 14:30:09 UTC) #3
Marijn Kruisselbrink
https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp#newcode219 third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:219: return; On 2016/07/13 at 14:30:09, iclelland wrote: > Should ...
4 years, 5 months ago (2016-07-13 19:16:08 UTC) #4
iclelland
https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp#newcode219 third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:219: return; On 2016/07/13 19:16:08, Marijn Kruisselbrink wrote: > On ...
4 years, 5 months ago (2016-07-15 14:31:11 UTC) #6
Marijn Kruisselbrink
https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp#newcode219 third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:219: return; On 2016/07/15 at 14:31:11, iclelland wrote: > On ...
4 years, 5 months ago (2016-07-18 18:43:20 UTC) #8
Marijn Kruisselbrink
ping?
4 years, 5 months ago (2016-07-19 18:19:29 UTC) #12
Marijn Kruisselbrink
+chasej since it seems iclelland is OOO
4 years, 5 months ago (2016-07-19 22:25:45 UTC) #14
Marijn Kruisselbrink
On 2016/07/19 at 22:25:45, Marijn Kruisselbrink wrote: > +chasej since it seems iclelland is OOO ...
4 years, 5 months ago (2016-07-20 17:39:53 UTC) #15
chasej
Looks good. A few minor points below. https://codereview.chromium.org/2123323004/diff/80001/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/2123323004/diff/80001/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp#newcode164 third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:164: for (const ...
4 years, 5 months ago (2016-07-21 05:01:07 UTC) #16
Yuki
Just drop-by, and mechanical changes in bindings/ LGTM.
4 years, 5 months ago (2016-07-21 06:41:17 UTC) #18
haraken
WebKit/Source/ LGTM
4 years, 5 months ago (2016-07-21 09:48:11 UTC) #19
Marijn Kruisselbrink
https://codereview.chromium.org/2123323004/diff/80001/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/2123323004/diff/80001/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp#newcode164 third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:164: for (const String& token : tokens) { On 2016/07/21 ...
4 years, 5 months ago (2016-07-21 17:44:10 UTC) #23
chasej
LGTM.
4 years, 5 months ago (2016-07-21 19:14:46 UTC) #26
Marijn Kruisselbrink
+mpearson for histograms OWNERS +chrishtr for WebKit/public OWNERS
4 years, 5 months ago (2016-07-21 19:22:22 UTC) #28
chrishtr
lgtm
4 years, 5 months ago (2016-07-21 22:48:32 UTC) #29
Mark P
histograms.xml mostly lgtm with one comment below https://codereview.chromium.org/2123323004/diff/100001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2123323004/diff/100001/tools/metrics/histograms/histograms.xml#oldcode37095 tools/metrics/histograms/histograms.xml:37095: enum="OriginTrialMessageGeneratedResult"> Please ...
4 years, 5 months ago (2016-07-21 23:16:58 UTC) #30
Marijn Kruisselbrink
https://codereview.chromium.org/2123323004/diff/100001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2123323004/diff/100001/tools/metrics/histograms/histograms.xml#oldcode37095 tools/metrics/histograms/histograms.xml:37095: enum="OriginTrialMessageGeneratedResult"> On 2016/07/21 at 23:16:58, Mark P (no metrics ...
4 years, 5 months ago (2016-07-21 23:29:54 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2123323004/120001
4 years, 5 months ago (2016-07-21 23:32:38 UTC) #37
commit-bot: I haz the power
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_android_rel_ng/builds/108583)
4 years, 5 months ago (2016-07-22 02:47:33 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2123323004/120001
4 years, 5 months ago (2016-07-22 03:24:59 UTC) #41
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 5 months ago (2016-07-22 04:00:14 UTC) #42
commit-bot: I haz the power
4 years, 5 months ago (2016-07-22 04:02:14 UTC) #44
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90
Cr-Commit-Position: refs/heads/master@{#407056}

Powered by Google App Engine
This is Rietveld 408576698