New issue
Advanced search Search tips

Issue 627942 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

OriginTrial histograms don't make much sense

Project Member Reported by mek@chromium.org, Jul 13 2016

Issue description

The FeatureEnabled histogram is logged regardless of if a feature is
actually being used on a page, making the data hard to reason about (as it will log an entry for each feature that exists for each token for each context, so every valid token will also be logged as being for the wrong feature for example).

The MessageGenerated histogram is 100% NotRequested after the bindings
were changed to completely not generate bindings for disabled features, making it not useful at all.

So I propose to get rid of both these histograms and replace them with a different histogram that is logged once for each token in each context.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d1c0acb152de57e2d1db5ca9febd3aa67664c90

commit 9d1c0acb152de57e2d1db5ca9febd3aa67664c90
Author: mek <mek@chromium.org>
Date: Fri Jul 22 03:59:59 2016

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 

Review-Url: https://codereview.chromium.org/2123323004
Cr-Commit-Position: refs/heads/master@{#407056}

[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/content/common/origin_trials/trial_token.cc
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/content/common/origin_trials/trial_token.h
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/content/common/origin_trials/trial_token_unittest.cc
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/content/common/origin_trials/trial_token_validator.cc
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/content/common/origin_trials/trial_token_validator.h
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/content/common/origin_trials/trial_token_validator_unittest.cc
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/content/renderer/origin_trials/web_trial_token_validator_impl.cc
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/content/renderer/origin_trials/web_trial_token_validator_impl.h
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/third_party/WebKit/Source/build/scripts/templates/OriginTrials.cpp.tmpl
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/third_party/WebKit/Source/core/origin_trials/testing/InternalsFrobulate.cpp
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/third_party/WebKit/public/platform/WebOriginTrialTokenStatus.h
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/third_party/WebKit/public/platform/WebTrialTokenValidator.h
[modify] https://crrev.com/9d1c0acb152de57e2d1db5ca9febd3aa67664c90/tools/metrics/histograms/histograms.xml

Comment 2 by mek@chromium.org, Jul 22 2016

Status: Fixed (was: Started)

Sign in to add a comment