Simplify OriginTrialContext and the way it validates tokens.

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}

[Marijn Kruisselbrink, 2016-07-08 00:47:01]

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

[Marijn Kruisselbrink, 2016-07-08 00:47:01]

See also the thread I started on experimentation-dev about this. Given how the
bindings changed in how they used OriginTrialContext, this seems like a
reasonable simplification (and optimization, and a more meaningful histogram).
What do you think? Feel free to disagree of course.

[iclelland, 2016-07-13 14:30:09]

I like this; it simplifies a lot of the code in good ways. Only a couple of
questions below.

https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:219: return;
Should we record this in UMA in this case? We don't currently have a result in
the histograms for "empty token was provided", since we'd never try to validate
it before. If we are now, and we could reach this code here, I think we should
note what the error was.

https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/testing/InternalsFrobulate.cpp
(right):

https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/testing/InternalsFrobulate.cpp:18:
exceptionState.throwDOMException(NotSupportedError, "API is not enabled");
Minor nit -- as a test-only interface, we can be as explicit as we need to be
about what went wrong here. Would it be helpful for future engineers debugging
tests to say something like "The Origin Trials Sample API has not been enabled
in this context"?

https://codereview.chromium.org/2123323004/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2123323004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:84753: +<enum
name="OriginTrialTokenStatus" type="int">
If you're deprecating OriginTrialEnableResult, then we should put a comment
there suggesting that people not use it, as it is likely going to get
out-of-sync with the code.

[Marijn Kruisselbrink, 2016-07-13 19:16:08]

https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:219: return;
On 2016/07/13 at 14:30:09, iclelland wrote:
> Should we record this in UMA in this case? We don't currently have a result in
the histograms for "empty token was provided", since we'd never try to validate
it before. If we are now, and we could reach this code here, I think we should
note what the error was.

Not sure... If we do start logging for empty tokens we should probably make sure
that we're not filtering them out anywhere else first. Happy to do that if you
think it's worthwhile of course, but for now I've just added a comment here
explaining why we're not logging to the histogram.

https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/testing/InternalsFrobulate.cpp
(right):

https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/testing/InternalsFrobulate.cpp:18:
exceptionState.throwDOMException(NotSupportedError, "API is not enabled");
On 2016/07/13 at 14:30:09, iclelland wrote:
> Minor nit -- as a test-only interface, we can be as explicit as we need to be
about what went wrong here. Would it be helpful for future engineers debugging
tests to say something like "The Origin Trials Sample API has not been enabled
in this context"?

Done

https://codereview.chromium.org/2123323004/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2123323004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:84753: +<enum
name="OriginTrialTokenStatus" type="int">
On 2016/07/13 at 14:30:09, iclelland wrote:
> If you're deprecating OriginTrialEnableResult, then we should put a comment
there suggesting that people not use it, as it is likely going to get
out-of-sync with the code.

Done

[Marijn Kruisselbrink, 2016-07-13 19:24:32]

Description was changed from

==========
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=
==========

to

==========
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
==========

[iclelland, 2016-07-15 14:31:11]

https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:219: return;
On 2016/07/13 19:16:08, Marijn Kruisselbrink wrote:
> On 2016/07/13 at 14:30:09, iclelland wrote:
> > Should we record this in UMA in this case? We don't currently have a result
in
> the histograms for "empty token was provided", since we'd never try to
validate
> it before. If we are now, and we could reach this code here, I think we should
> note what the error was.
> 
> Not sure... If we do start logging for empty tokens we should probably make
sure
> that we're not filtering them out anywhere else first. Happy to do that if you
> think it's worthwhile of course, but for now I've just added a comment here
> explaining why we're not logging to the histogram.

So, it turns out we are filtering them out, above, in addToken(), so this code
*shouldn't* be reached. What do you think of adding NOTREACHED here, and then
returning (so that release code does the right thing without crashing if we're
wrong)?

Alternately, we could *not* filter empty tokens above, and then log the issue
here so we're aware of how often it happens.

I don't think this should ever happen; the only way would be for an empty token
to somehow be already present in an ExecutionContext, which is then copied to
another context, but we shouldn't be adding an empty one in the first place.

[Marijn Kruisselbrink, 2016-07-18 18:43:14]

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

[Marijn Kruisselbrink, 2016-07-18 18:43:20]

https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/2123323004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:219: return;
On 2016/07/15 at 14:31:11, iclelland wrote:
> On 2016/07/13 19:16:08, Marijn Kruisselbrink wrote:
> > On 2016/07/13 at 14:30:09, iclelland wrote:
> > > Should we record this in UMA in this case? We don't currently have a
result in
> > the histograms for "empty token was provided", since we'd never try to
validate
> > it before. If we are now, and we could reach this code here, I think we
should
> > note what the error was.
> > 
> > Not sure... If we do start logging for empty tokens we should probably make
sure
> > that we're not filtering them out anywhere else first. Happy to do that if
you
> > think it's worthwhile of course, but for now I've just added a comment here
> > explaining why we're not logging to the histogram.
> 
> So, it turns out we are filtering them out, above, in addToken(), so this code
*shouldn't* be reached. What do you think of adding NOTREACHED here, and then
returning (so that release code does the right thing without crashing if we're
wrong)?

addToken() filters them out, addTokens() doesn't currently filter them out
though. But generally addTokens() is only called with either the tokens from a
different execution context, or the tokens that were returned by
parseHeaderValue, which does filter out empty tokens. So yes, we're more or less
filtering out empty tokens, but not consistently. Since I think it's confusing
that addToken allows (and ignores) empty tokens, but addTokens does not do so,
I've changed addTokens to also filter out empty tokens (even though currently
it's never called with empty tokens).

(btw, adding a NOTREACHED and then returning "so that release code does the
right thing without crashing if we're wrong" is explicitly against the style
guide. You should never have explicit code to deal with the case of a DCHECK or
NOTREACHED failing).

[commit-bot: I haz the power, 2016-07-18 18:43:57]

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, 2016-07-18 20:16:57]

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

[commit-bot: I haz the power, 2016-07-18 20:16:58]

Dry run: This issue passed the CQ dry run.

[Marijn Kruisselbrink, 2016-07-19 18:19:29]

ping?

[Marijn Kruisselbrink, 2016-07-19 22:25:44]

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

[Marijn Kruisselbrink, 2016-07-19 22:25:45]

+chasej since it seems iclelland is OOO

[Marijn Kruisselbrink, 2016-07-20 17:39:53]

On 2016/07/19 at 22:25:45, Marijn Kruisselbrink wrote:
> +chasej since it seems iclelland is OOO

chasej: ping?

[chasej, 2016-07-21 05:01:07]

Looks good. A few minor points below.

https://codereview.chromium.org/2123323004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/2123323004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:164: for
(const String& token : tokens) {
Earlier comments suggested that you were adding an explicit check for empty
tokens, similar to addToken(). I would expect to see that here?

https://codereview.chromium.org/2123323004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:237:
m_enabledFeatures.add(featureName);
What if the feature was already enabled by another token? Should this guard
against duplicates? For example, an origin could have multiple tokens, with
overlapping periods of validity, so it provides all tokens for each context.

[Yuki, 2016-07-21 06:41:17]

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

[Yuki, 2016-07-21 06:41:17]

Just drop-by, and mechanical changes in bindings/ LGTM.

[haraken, 2016-07-21 09:48:11]

WebKit/Source/ LGTM

[Marijn Kruisselbrink, 2016-07-21 17:42:41]

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

[commit-bot: I haz the power, 2016-07-21 17:43:05]

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

[Marijn Kruisselbrink, 2016-07-21 17:44:09]

mek@chromium.org changed reviewers:
+ haraken@chromium.org

[Marijn Kruisselbrink, 2016-07-21 17:44:10]

https://codereview.chromium.org/2123323004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/2123323004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:164: for
(const String& token : tokens) {
On 2016/07/21 at 05:01:07, chasej wrote:
> Earlier comments suggested that you were adding an explicit check for empty
tokens, similar to addToken(). I would expect to see that here?

Argh, yes. I changed the loop so I could add it, but then forgot to actually add
the empty check. Done now.

https://codereview.chromium.org/2123323004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:237:
m_enabledFeatures.add(featureName);
On 2016/07/21 at 05:01:07, chasej wrote:
> What if the feature was already enabled by another token? Should this guard
against duplicates? For example, an origin could have multiple tokens, with
overlapping periods of validity, so it provides all tokens for each context.

Adding something to a HashSet that's already in there is just simply a no-op. So
there shouldn't be a need to have any explicit checks to guard against
duplicates.

[commit-bot: I haz the power, 2016-07-21 19:06:33]

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

[commit-bot: I haz the power, 2016-07-21 19:06:35]

Dry run: This issue passed the CQ dry run.

[chasej, 2016-07-21 19:14:46]

LGTM.

[Marijn Kruisselbrink, 2016-07-21 19:22:21]

mek@chromium.org changed reviewers:
+ chrishtr@chromium.org, mpearson@chromium.org

[Marijn Kruisselbrink, 2016-07-21 19:22:22]

+mpearson for histograms OWNERS
+chrishtr for WebKit/public OWNERS

[chrishtr, 2016-07-21 22:48:32]

lgtm

[Mark P, 2016-07-21 23:16:58]

histograms.xml mostly lgtm with one comment below

https://codereview.chromium.org/2123323004/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2123323004/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:37095:
enum="OriginTrialMessageGeneratedResult">
Please mark this enum as obsolete as well.  (It appears to be.)

[Marijn Kruisselbrink, 2016-07-21 23:22:36]

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

[commit-bot: I haz the power, 2016-07-21 23:23:03]

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

[Marijn Kruisselbrink, 2016-07-21 23:29:54]

https://codereview.chromium.org/2123323004/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2123323004/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:37095:
enum="OriginTrialMessageGeneratedResult">
On 2016/07/21 at 23:16:58, Mark P (no metrics til 7-26) wrote:
> Please mark this enum as obsolete as well.  (It appears to be.)

Done

[Marijn Kruisselbrink, 2016-07-21 23:31:48]

The CQ bit was unchecked by mek@chromium.org

[Marijn Kruisselbrink, 2016-07-21 23:31:50]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-07-21 23:31:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yukishiino@chromium.org, chrishtr@chromium.org,
mpearson@chromium.org, chasej@chromium.org
Link to the patchset: https://codereview.chromium.org/2123323004/#ps120001
(title: "mark enum as obsolete")

[commit-bot: I haz the power, 2016-07-21 23:32:38]

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, 2016-07-22 02:47:31]

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

[commit-bot: I haz the power, 2016-07-22 02:47:33]

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...)

[Marijn Kruisselbrink, 2016-07-22 03:23:38]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2016-07-22 03:24:59]

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, 2016-07-22 04:00:14]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-22 04:02:12]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-07-22 04:02:14]

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