Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1909633003:
Collect UMA data for Origin Trials (Closed)
|
Created:
4 years, 8 months ago by chasej Modified:
4 years, 7 months ago CC:
asvitkine+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chasej+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, jam, mkwst+moarreviews-renderer_chromium.org, 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. |
DescriptionCollect UMA data for Origin Trials
This CL records UMA metrics for the results of feature enabled checks,
one per context. Also records metrics for when error messages are
generated.
Token validation is controlled by the embedder, but the enabled check is
performed in Blink. To collect the desired metrics, it was necessary to
plumb a detailed validation return code through the chain.
BUG=587575
Committed: https://crrev.com/455dbe71939225cb3765da12c6aed749b72276ea
Cr-Commit-Position: refs/heads/master@{#391505}
Patch Set 1 #
Total comments: 33
Patch Set 2 : Address some comments #
Total comments: 6
Patch Set 3 : Use one central enum, and address other comments #Patch Set 4 : Rebase #Patch Set 5 : Fix/update unit tests #
Total comments: 24
Patch Set 6 : Address comments #
Total comments: 6
Patch Set 7 : Address more comments #Patch Set 8 : Rebase #
Total comments: 2
Messages
Total messages: 29 (9 generated)
Description was changed from ========== Collect UMA data for Origin Trials This CL records UMA metrics for the results of feature enabled checks, one per context. Also records metrics for when error messages are generated. Token validation is controlled by the embedder, but the enabled check is performed in blink. The collect the desired metrics, it was necessary to plumb a detailed validation return code through the chain. BUG=587575 ========== to ========== Collect UMA data for Origin Trials This CL records UMA metrics for the results of feature enabled checks, one per context. Also records metrics for when error messages are generated. Token validation is controlled by the embedder, but the enabled check is performed in Blink. To collect the desired metrics, it was necessary to plumb a detailed validation return code through the chain. BUG=587575 ==========
chasej@chromium.org changed reviewers: + mek@chromium.org
mek, please take a look.
https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... File content/common/origin_trials/trial_token.h (right): https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... content/common/origin_trials/trial_token.h:46: std::unique_ptr<TrialToken>& out_token); Pass-by-(non-const)-reference is against the styleguide. If you want to return something through an argument you should use a pointer. I also wonder if since this method is still called "From" it might not make sense to still return a std::unique_ptr, and have the status result through a (pointer) out-parameter. That would at least avoid the somewhat weird situation of having a pointer to a unique pointer. It's also not entirely clear to me why TrialToken is still a class that we even bother instantiating though. Maybe as an implementation detail of TrialTokenValidator it makes sense, but TrialToken instances are always short-lived, yet heap-allocated, and there doesn't even really seem to be any benefit to try to keep one around. Of course if in the future we would somehow cache parsed tokens rather than token strings there might be some point in having a TrialToken instance, but even that wouldn't really work in the presence of multiple public keys. https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... content/common/origin_trials/trial_token.h:68: std::unique_ptr<std::string>& out_token_json); Here too, don't use non-const reference arguments. And in fact now you have an explicit TrialTokenStatus result the out_token_json doesn't have to be a unique_ptr anymore either. So just use std::string* out_token_json or something like that. https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... File content/common/origin_trials/trial_token_status.h (right): https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... content/common/origin_trials/trial_token_status.h:10: enum TrialTokenStatus { Rather than having two enums and converting between them, you only need the blink enum. Just move it to a separate header file, and add that file to the long list of "strictly enum/POD header-only types" that are allowed to be included in content/common/DEPS https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:22: enum EnableResult { I would make these enum class EnableResult { Success, NotSupported, ... (also below) It's also a bit unfortunate that we need a new enum separate from TokenValidationResult for this, just because there are a few error conditions that aren't included in TokenValidationResult. It might be cleaner to just add all the failure modes to the one main enum. https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:23: EnableResultSuccess = 0, Either give all enum members explicit values, or rely on the compiler to do the right thing. It doesn't really make sense to explicitly set the first member to zero but still rely on the compiler to do the right thing for the rest. https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:47: DEFINE_STATIC_LOCAL(EnumerationHistogram, histogram, ("OriginTrials.FeatureEnabled", EnableResultLast)); Since we actually want to support trials in workers as well, these should probably be DEFINE_THREAD_SAFE_STATIC_LOCAL? DEFINE_STATIC_LOCAL is not thread safe after all. https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:94: ASSERT_NOT_REACHED(); ASSERT_NOT_REACHED() is deprecated. Use NOTREACHED() instead. https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:218: *errorMessage = getInvalidTokenMessage(featureName); Would it make sense to refine the error message based on the result of checkFeatureEnabled? https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:226: int OriginTrialContext::checkFeatureEnabled(const String& featureName, String* errorMessage, WebTrialTokenValidator* validator) Why is this returning "int" rather than the correct enum type? https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebTrialTokenValidator.h (right): https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebTrialTokenValidator.h:5: #ifndef THIRD_PARTY_WEBKIT_PUBLIC_PLATFORM_WEBTRIALTOKENVALIDATOR_H_ Why this change?
https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... File content/common/origin_trials/trial_token.h (right): https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... content/common/origin_trials/trial_token.h:46: std::unique_ptr<TrialToken>& out_token); On 2016/04/21 18:18:42, Marijn Kruisselbrink wrote: > Pass-by-(non-const)-reference is against the styleguide. If you want to return > something through an argument you should use a pointer. > I also wonder if since this method is still called "From" it might not make > sense to still return a std::unique_ptr, and have the status result through a > (pointer) out-parameter. That would at least avoid the somewhat weird situation > of having a pointer to a unique pointer. > TL;DR - I made the result enum be the out parameter, and went back to returning a unique_ptr. You're correct, I missed the part in the Google style guide about reference arguments. I didn't see anything about how to handle out parameters that were smart pointers in the various docs on ownership and passing objects. Common wisdom (outside of our style guides) suggested that a non-const ref was the best way to have a unique_ptr as an out parameter. As you say, maybe the best answer is to swap the return and out values. I originally considered that, but it seemed the convention was typically to return the result code, and have the maybe-created object as the out parameter. In this case though, I think the swap is justifiable, especially with the "From" naming. > It's also not entirely clear to me why TrialToken is still a class that we even > bother instantiating though. Maybe as an implementation detail of > TrialTokenValidator it makes sense, but TrialToken instances are always > short-lived, yet heap-allocated, and there doesn't even really seem to be any > benefit to try to keep one around. Of course if in the future we would somehow > cache parsed tokens rather than token strings there might be some point in > having a TrialToken instance, but even that wouldn't really work in the presence > of multiple public keys. The plan was to have some caching of parsed tokens, or at least the validation results. Of course, we don't have any code like that as yet. I don't really understand what you mean that caching wouldn't work with multiple public keys. Regardless, if we want to get rid of TrialToken, I'd suggest that should be a separate bug/CL. https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... content/common/origin_trials/trial_token.h:68: std::unique_ptr<std::string>& out_token_json); On 2016/04/21 18:18:42, Marijn Kruisselbrink wrote: > Here too, don't use non-const reference arguments. And in fact now you have an > explicit TrialTokenStatus result the out_token_json doesn't have to be a > unique_ptr anymore either. So just use std::string* out_token_json or something > like that. Done. https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... File content/common/origin_trials/trial_token_status.h (right): https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... content/common/origin_trials/trial_token_status.h:10: enum TrialTokenStatus { On 2016/04/21 18:18:42, Marijn Kruisselbrink wrote: > Rather than having two enums and converting between them, you only need the > blink enum. Just move it to a separate header file, and add that file to the > long list of "strictly enum/POD header-only types" that are allowed to be > included in content/common/DEPS I started with one enum. However, it seemed strange that TrialTokenValidator had to reference a Blink enum when it really had no dependency on/knowledge of Blink. IIRC, the intent was to have TrialTokenValidator be independent so it could be used outside of the render process. I found this comment from an earlier CL: https://codereview.chromium.org/1752463002#msg9. Converting between two enums isn't ideal, but it seem logical where it's currently being done in WebTrialTokenValidatorImpl. More importantly, if the TrialTokenValidator ends up being used from the browser/service workers, would it still be valid/make sense to reference a Blink enum there? Of course, I could use the Blink enum now, and defer that question. What do you think? https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:22: enum EnableResult { On 2016/04/21 18:18:43, Marijn Kruisselbrink wrote: > I would make these enum class EnableResult { Success, NotSupported, ... > (also below) > > It's also a bit unfortunate that we need a new enum separate from > TokenValidationResult for this, just because there are a few error conditions > that aren't included in TokenValidationResult. It might be cleaner to just add > all the failure modes to the one main enum. Yes, it's not ideal to have the separate enum. I considered renaming TokenValidationResult to TokenStatus, like on the content side. However, it seem clearer to keep the naming, as the enum was used exclusively by the WebTrialTokenValidator. Also, it seemed strange to have values in the enum (i.e. NoTokens, Insecure) that didn't apply at all to the WebTrialTokenValidator. I see the options as: 1) Rename TokenValidationResult to WebTrialTokenStatus with additional values (in a separate file like WebCachePolicy.h) 2) Leave as is, with separate EnableResult enum I'll hold off on the enum class changes, until the above is resolved. https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:23: EnableResultSuccess = 0, On 2016/04/21 18:18:42, Marijn Kruisselbrink wrote: > Either give all enum members explicit values, or rely on the compiler to do the > right thing. It doesn't really make sense to explicitly set the first member to > zero but still rely on the compiler to do the right thing for the rest. Done. https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:47: DEFINE_STATIC_LOCAL(EnumerationHistogram, histogram, ("OriginTrials.FeatureEnabled", EnableResultLast)); On 2016/04/21 18:18:43, Marijn Kruisselbrink wrote: > Since we actually want to support trials in workers as well, these should > probably be DEFINE_THREAD_SAFE_STATIC_LOCAL? DEFINE_STATIC_LOCAL is not thread > safe after all. Done. https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:94: ASSERT_NOT_REACHED(); On 2016/04/21 18:18:42, Marijn Kruisselbrink wrote: > ASSERT_NOT_REACHED() is deprecated. Use NOTREACHED() instead. Done. https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:218: *errorMessage = getInvalidTokenMessage(featureName); On 2016/04/21 18:18:43, Marijn Kruisselbrink wrote: > Would it make sense to refine the error message based on the result of > checkFeatureEnabled? I supposed we could now - before it wasn't possible with just a boolean result. Also, the new token format isn't human readable, so you can't visually inspect tokens for some the problems like wrong feature/origin. On the other hand, if we're able to make the desired bindings changes, there could be no need for detailed error messages. Ditto, if/when there is devtools support. I'll consider that with other changes in the next patch. https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:226: int OriginTrialContext::checkFeatureEnabled(const String& featureName, String* errorMessage, WebTrialTokenValidator* validator) On 2016/04/21 18:18:43, Marijn Kruisselbrink wrote: > Why is this returning "int" rather than the correct enum type? The EnableResult enum is declared local to the source file. My intent was to keep the enum out of the header file, since it is purely an implementation detail. Having this function return int was the one issue with that approach. This may be a moot point, depending on the resolution of the larger questions about the various enum declarations. If not, what makes the most sense here? Add a comment explaining the int return value, or move the EnableResult to the header file? https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebTrialTokenValidator.h (right): https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebTrialTokenValidator.h:5: #ifndef THIRD_PARTY_WEBKIT_PUBLIC_PLATFORM_WEBTRIALTOKENVALIDATOR_H_ On 2016/04/21 18:18:43, Marijn Kruisselbrink wrote: > Why this change? A side effect of the battle between git cl format, git cl lint, and webkit style. It passed the presubmit checks, so I guess it's now allowed although not typical in Blink? Is it better to revert?
https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... File content/common/origin_trials/trial_token.h (right): https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... content/common/origin_trials/trial_token.h:46: std::unique_ptr<TrialToken>& out_token); On 2016/04/22 at 18:50:06, chasej wrote: > On 2016/04/21 18:18:42, Marijn Kruisselbrink wrote: > > It's also not entirely clear to me why TrialToken is still a class that we even > > bother instantiating though. Maybe as an implementation detail of > > TrialTokenValidator it makes sense, but TrialToken instances are always > > short-lived, yet heap-allocated, and there doesn't even really seem to be any > > benefit to try to keep one around. Of course if in the future we would somehow > > cache parsed tokens rather than token strings there might be some point in > > having a TrialToken instance, but even that wouldn't really work in the presence > > of multiple public keys. > > The plan was to have some caching of parsed tokens, or at least the validation results. Of course, we don't have any code like that as yet. I don't really understand what you mean that caching wouldn't work with multiple public keys. > > Regardless, if we want to get rid of TrialToken, I'd suggest that should be a separate bug/CL. Never mind, you're right. I think I was thinking that somehow the validating the signature before parsing would mean that the parsed tokens are now somehow tied to the public key used to parse them. But even if that is the case, it doesn't make it any less valid to cache them, as once the key is validated it really doesn't matter anymore what key they were signed with in the first place. https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... File content/common/origin_trials/trial_token_status.h (right): https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... content/common/origin_trials/trial_token_status.h:10: enum TrialTokenStatus { On 2016/04/22 at 18:50:06, chasej wrote: > On 2016/04/21 18:18:42, Marijn Kruisselbrink wrote: > > Rather than having two enums and converting between them, you only need the > > blink enum. Just move it to a separate header file, and add that file to the > > long list of "strictly enum/POD header-only types" that are allowed to be > > included in content/common/DEPS > > I started with one enum. However, it seemed strange that TrialTokenValidator had to reference a Blink enum when it really had no dependency on/knowledge of Blink. IIRC, the intent was to have TrialTokenValidator be independent so it could be used outside of the render process. I found this comment from an earlier CL: > https://codereview.chromium.org/1752463002#msg9. > > Converting between two enums isn't ideal, but it seem logical where it's currently being done in WebTrialTokenValidatorImpl. More importantly, if the TrialTokenValidator ends up being used from the browser/service workers, would it still be valid/make sense to reference a Blink enum there? Of course, I could use the Blink enum now, and defer that question. > > What do you think? I agree that both options are not ideal. Not having duplicate enums seems nice, and there are plenty of enums already that are defined in blink an used at all layers in content. But I agree that usually is because it's a value that eventually would be passed to/from blink anyway, while for something like https://codereview.chromium.org/1763943002 (which I wasn't planning on landing/cleaning up until there is a need for it) it wouldn't ever end up in blink code. I don't feel particularly strongly either way about this, but at least from a policy/layering/technical point of view there is no reason not to use the same enum in blink, content/renderer, content/common and possibly content/browser (as long as the enum is defined in its own header file, allowing DEPS files to be configured to only allow access to the enum and not any other blink interfaces). https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:22: enum EnableResult { On 2016/04/22 at 18:50:06, chasej wrote: > On 2016/04/21 18:18:43, Marijn Kruisselbrink wrote: > > I would make these enum class EnableResult { Success, NotSupported, ... > > (also below) > > > > It's also a bit unfortunate that we need a new enum separate from > > TokenValidationResult for this, just because there are a few error conditions > > that aren't included in TokenValidationResult. It might be cleaner to just add > > all the failure modes to the one main enum. > > Yes, it's not ideal to have the separate enum. I considered renaming TokenValidationResult to TokenStatus, like on the content side. However, it seem clearer to keep the naming, as the enum was used exclusively by the WebTrialTokenValidator. Also, it seemed strange to have values in the enum (i.e. NoTokens, Insecure) that didn't apply at all to the WebTrialTokenValidator. > > I see the options as: > 1) Rename TokenValidationResult to WebTrialTokenStatus with additional values (in a separate file like WebCachePolicy.h) > 2) Leave as is, with separate EnableResult enum > > I'll hold off on the enum class changes, until the above is resolved. My preference would be to just have one central enum with all possible values. But then I've been known to try to future-proof code too much (in this case one potential future thing I could imagine would be if/when we check tokens in the browser process for some features we'll probably want to collect the same (or similar) UMA stats for those checks. At that point we'll need some enum somewhere that also includes the NoTokens and Insecure values). But since it is easy enough to merge any of these enums later, I don't feel particularly strongly about that either; other than the general less code to write is less code to maintain. And having three separate enums for essentially the same thing seems quite a bit of unnecessary (but at least trivial) overhead. https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:218: *errorMessage = getInvalidTokenMessage(featureName); On 2016/04/22 at 18:50:06, chasej wrote: > On 2016/04/21 18:18:43, Marijn Kruisselbrink wrote: > > Would it make sense to refine the error message based on the result of > > checkFeatureEnabled? > > I supposed we could now - before it wasn't possible with just a boolean result. Also, the new token format isn't human readable, so you can't visually inspect tokens for some the problems like wrong feature/origin. > > On the other hand, if we're able to make the desired bindings changes, there could be no need for detailed error messages. Ditto, if/when there is devtools support. > > I'll consider that with other changes in the next patch. sounds good (not sure how bindings changes would eliminate the need for detailed error messages though?) https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:226: int OriginTrialContext::checkFeatureEnabled(const String& featureName, String* errorMessage, WebTrialTokenValidator* validator) On 2016/04/22 at 18:50:06, chasej wrote: > On 2016/04/21 18:18:43, Marijn Kruisselbrink wrote: > > Why is this returning "int" rather than the correct enum type? > > The EnableResult enum is declared local to the source file. My intent was to keep the enum out of the header file, since it is purely an implementation detail. Having this function return int was the one issue with that approach. > > This may be a moot point, depending on the resolution of the larger questions about the various enum declarations. If not, what makes the most sense here? Add a comment explaining the int return value, or move the EnableResult to the header file? I think anything that results in having the method return the correct type rather than an int would be better (so probably define the enum as private in the header file). Everything that is "private" is purely an implementation detail, yet we don't go out of our way (in chromium) to try to hide all of that from header files, so not sure why this would be different. But if you do want to keep implementation details out of the header file, you could just get rid of the entirely separate checkFeatureEnabled method. It's kind of confusing to have to methods with nearly identical names, taking identical arguments that do subtly different things, even if one is only a private method. I'm not entirely sure what we gain from having a separate method (if you do want a separate methods, maybe you could have a file local non-member method that does all of the current checkFeatureEnabled minus the secure context check by passing the list of tokens to said method. That way you could even have correct return type without defining the enum anywhere else). https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebTrialTokenValidator.h (right): https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebTrialTokenValidator.h:5: #ifndef THIRD_PARTY_WEBKIT_PUBLIC_PLATFORM_WEBTRIALTOKENVALIDATOR_H_ On 2016/04/22 at 18:50:06, chasej wrote: > On 2016/04/21 18:18:43, Marijn Kruisselbrink wrote: > > Why this change? > > A side effect of the battle between git cl format, git cl lint, and webkit style. It passed the presubmit checks, so I guess it's now allowed although not typical in Blink? Is it better to revert? Since I don't see any code anywhere in blink with this style of include guard, I think it is indeed better to revert. https://codereview.chromium.org/1909633003/diff/20001/content/common/origin_t... File content/common/origin_trials/trial_token.cc (right): https://codereview.chromium.org/1909633003/diff/20001/content/common/origin_t... content/common/origin_trials/trial_token.cc:46: // Static the lowercase version of this comment is vastly more common than the uppercase version, so not sure why you changed it (separately, the Extract and Parse method should probably have a // static comment as well to be consistent). https://codereview.chromium.org/1909633003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/1909633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:126: // There could be zero, one or multiple trial tokens provided for a given I wonder if this comment should maybe be in the getTokenValidationResultPriorityFor method rather than here... I can see why it makes sense to have the comment here, but having the comment and the implementation of the comment as far apart as they are now makes it likely that somebody might end up updating one without updating the other... Also without any comments in getTokenValidationResultPriorityFor it is completely non-obvious if higher numbers are higher or lower priority (and even the comment doesn't make it explicit that lower numbers mean higher priority). https://codereview.chromium.org/1909633003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp (right): https://codereview.chromium.org/1909633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:54: void reset() Maybe while you're editing this file anyway, you could just get rid of reset() since it isn't actually used anywhere.
https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... File content/common/origin_trials/trial_token_status.h (right): https://codereview.chromium.org/1909633003/diff/1/content/common/origin_trial... content/common/origin_trials/trial_token_status.h:10: enum TrialTokenStatus { On 2016/04/22 21:51:06, Marijn Kruisselbrink wrote: > On 2016/04/22 at 18:50:06, chasej wrote: > > On 2016/04/21 18:18:42, Marijn Kruisselbrink wrote: > > > Rather than having two enums and converting between them, you only need the > > > blink enum. Just move it to a separate header file, and add that file to the > > > long list of "strictly enum/POD header-only types" that are allowed to be > > > included in content/common/DEPS > > > > I started with one enum. However, it seemed strange that TrialTokenValidator > had to reference a Blink enum when it really had no dependency on/knowledge of > Blink. IIRC, the intent was to have TrialTokenValidator be independent so it > could be used outside of the render process. I found this comment from an > earlier CL: > > https://codereview.chromium.org/1752463002#msg9. > > > > Converting between two enums isn't ideal, but it seem logical where it's > currently being done in WebTrialTokenValidatorImpl. More importantly, if the > TrialTokenValidator ends up being used from the browser/service workers, would > it still be valid/make sense to reference a Blink enum there? Of course, I could > use the Blink enum now, and defer that question. > > > > What do you think? > > I agree that both options are not ideal. Not having duplicate enums seems nice, > and there are plenty of enums already that are defined in blink an used at all > layers in content. But I agree that usually is because it's a value that > eventually would be passed to/from blink anyway, while for something like > https://codereview.chromium.org/1763943002 (which I wasn't planning on > landing/cleaning up until there is a need for it) it wouldn't ever end up in > blink code. > > I don't feel particularly strongly either way about this, but at least from a > policy/layering/technical point of view there is no reason not to use the same > enum in blink, content/renderer, content/common and possibly content/browser (as > long as the enum is defined in its own header file, allowing DEPS files to be > configured to only allow access to the enum and not any other blink interfaces). I've changed to share a single WebOriginTrialTokenStatus enum, defined in a separate file. I chose to add "Origin" to the name, to be more explicit. As this will live in public/platform, it doesn't have the a parent origin_trials folder to add context. Arguably, TrialToken and TrialTokenValidator should have "Origin" in the name as well, but I don't believe it's worth renaming existing classes at this point. https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:22: enum EnableResult { On 2016/04/22 21:51:06, Marijn Kruisselbrink wrote: > On 2016/04/22 at 18:50:06, chasej wrote: > > On 2016/04/21 18:18:43, Marijn Kruisselbrink wrote: > > > I would make these enum class EnableResult { Success, NotSupported, ... > > > (also below) > > > > > > It's also a bit unfortunate that we need a new enum separate from > > > TokenValidationResult for this, just because there are a few error > conditions > > > that aren't included in TokenValidationResult. It might be cleaner to just > add > > > all the failure modes to the one main enum. > > > > Yes, it's not ideal to have the separate enum. I considered renaming > TokenValidationResult to TokenStatus, like on the content side. However, it seem > clearer to keep the naming, as the enum was used exclusively by the > WebTrialTokenValidator. Also, it seemed strange to have values in the enum (i.e. > NoTokens, Insecure) that didn't apply at all to the WebTrialTokenValidator. > > > > I see the options as: > > 1) Rename TokenValidationResult to WebTrialTokenStatus with additional values > (in a separate file like WebCachePolicy.h) > > 2) Leave as is, with separate EnableResult enum > > > > I'll hold off on the enum class changes, until the above is resolved. > > My preference would be to just have one central enum with all possible values. > But then I've been known to try to future-proof code too much (in this case one > potential future thing I could imagine would be if/when we check tokens in the > browser process for some features we'll probably want to collect the same (or > similar) UMA stats for those checks. At that point we'll need some enum > somewhere that also includes the NoTokens and Insecure values). But since it is > easy enough to merge any of these enums later, I don't feel particularly > strongly about that either; other than the general less code to write is less > code to maintain. And having three separate enums for essentially the same thing > seems quite a bit of unnecessary (but at least trivial) overhead. As in earlier comment, I've changed to use a central enum, with all possible values. https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:218: *errorMessage = getInvalidTokenMessage(featureName); On 2016/04/22 21:51:06, Marijn Kruisselbrink wrote: > On 2016/04/22 at 18:50:06, chasej wrote: > > On 2016/04/21 18:18:43, Marijn Kruisselbrink wrote: > > > Would it make sense to refine the error message based on the result of > > > checkFeatureEnabled? > > > > I supposed we could now - before it wasn't possible with just a boolean > result. Also, the new token format isn't human readable, so you can't visually > inspect tokens for some the problems like wrong feature/origin. > > > > On the other hand, if we're able to make the desired bindings changes, there > could be no need for detailed error messages. Ditto, if/when there is devtools > support. > > > > I'll consider that with other changes in the next patch. > > sounds good (not sure how bindings changes would eliminate the need for detailed > error messages though?) Today, features are still available to JavaScript, even when disabled. While the bindings return undefined, the enabled check still happens on each access (which generates the messages). The goal is to have disabled features hidden completely from the bindings. If that is achieved, any access attempts would not perform the enabled check - so no message generated. https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:226: int OriginTrialContext::checkFeatureEnabled(const String& featureName, String* errorMessage, WebTrialTokenValidator* validator) On 2016/04/22 21:51:06, Marijn Kruisselbrink wrote: > On 2016/04/22 at 18:50:06, chasej wrote: > > On 2016/04/21 18:18:43, Marijn Kruisselbrink wrote: > > > Why is this returning "int" rather than the correct enum type? > > > > The EnableResult enum is declared local to the source file. My intent was to > keep the enum out of the header file, since it is purely an implementation > detail. Having this function return int was the one issue with that approach. > > > > This may be a moot point, depending on the resolution of the larger questions > about the various enum declarations. If not, what makes the most sense here? Add > a comment explaining the int return value, or move the EnableResult to the > header file? > > I think anything that results in having the method return the correct type > rather than an int would be better (so probably define the enum as private in > the header file). Everything that is "private" is purely an implementation > detail, yet we don't go out of our way (in chromium) to try to hide all of that > from header files, so not sure why this would be different. > > But if you do want to keep implementation details out of the header file, you > could just get rid of the entirely separate checkFeatureEnabled method. It's > kind of confusing to have to methods with nearly identical names, taking > identical arguments that do subtly different things, even if one is only a > private method. I'm not entirely sure what we gain from having a separate method > (if you do want a separate methods, maybe you could have a file local non-member > method that does all of the current checkFeatureEnabled minus the secure context > check by passing the list of tokens to said method. That way you could even have > correct return type without defining the enum anywhere else). The return type is a non-issue, now that I'm using a central enum. With regards to the two similar named methods, I didn't think the differences were that subtle (at least based on the implementation). I figured only one method is public, so there shouldn't be much confusion about which to use. The private method does the actual enabled check. The public method wraps the core check in handling for UMA and message generation. Granted there is some overlap in that either method could generate the error message. I had it all in one method at one point, but it was too convoluted because I could no longer use simple return statements to exit early. I updated the comments on the method declarations. Suggestions welcome if there is better naming to make the differences more obvious. https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebTrialTokenValidator.h (right): https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebTrialTokenValidator.h:5: #ifndef THIRD_PARTY_WEBKIT_PUBLIC_PLATFORM_WEBTRIALTOKENVALIDATOR_H_ On 2016/04/22 21:51:06, Marijn Kruisselbrink wrote: > On 2016/04/22 at 18:50:06, chasej wrote: > > On 2016/04/21 18:18:43, Marijn Kruisselbrink wrote: > > > Why this change? > > > > A side effect of the battle between git cl format, git cl lint, and webkit > style. It passed the presubmit checks, so I guess it's now allowed although not > typical in Blink? Is it better to revert? > > Since I don't see any code anywhere in blink with this style of include guard, I > think it is indeed better to revert. Done. https://codereview.chromium.org/1909633003/diff/20001/content/common/origin_t... File content/common/origin_trials/trial_token.cc (right): https://codereview.chromium.org/1909633003/diff/20001/content/common/origin_t... content/common/origin_trials/trial_token.cc:46: // Static On 2016/04/22 21:51:06, Marijn Kruisselbrink wrote: > the lowercase version of this comment is vastly more common than the uppercase > version, so not sure why you changed it (separately, the Extract and Parse > method should probably have a // static comment as well to be consistent). Done. https://codereview.chromium.org/1909633003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/1909633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:126: // There could be zero, one or multiple trial tokens provided for a given On 2016/04/22 21:51:06, Marijn Kruisselbrink wrote: > I wonder if this comment should maybe be in the > getTokenValidationResultPriorityFor method rather than here... I can see why it > makes sense to have the comment here, but having the comment and the > implementation of the comment as far apart as they are now makes it likely that > somebody might end up updating one without updating the other... > > Also without any comments in getTokenValidationResultPriorityFor it is > completely non-obvious if higher numbers are higher or lower priority (and even > the comment doesn't make it explicit that lower numbers mean higher priority). Done. https://codereview.chromium.org/1909633003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp (right): https://codereview.chromium.org/1909633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:54: void reset() On 2016/04/22 21:51:06, Marijn Kruisselbrink wrote: > Maybe while you're editing this file anyway, you could just get rid of reset() > since it isn't actually used anywhere. Done.
lgtm But I think you still need to update some of the tests in OriginTrialContextTest to work with these changes? https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:218: *errorMessage = getInvalidTokenMessage(featureName); On 2016/04/25 at 20:32:52, chasej wrote: > Today, features are still available to JavaScript, even when disabled. While the bindings return undefined, the enabled check still happens on each access (which generates the messages). The goal is to have disabled features hidden completely from the bindings. If that is achieved, any access attempts would not perform the enabled check - so no message generated. Ah, so it's not like bindings changes would eliminate the need for detailed error messages, but rather that it becomes harder/impossible to actually emit error messages? I think it would be unfortunate to get rid of messages to point out why features aren't working... I wonder what the plans are around secure-context only features, where at spec level feature are also supposed to be completely hidden from bindings rather then the current exceptions that are being thrown; do you know if they (whoever is working on that, presumably some of the same people from the bindings side) are also planning on getting rid of error messages for accessing secure context only APIs from an insecure context? https://codereview.chromium.org/1909633003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:226: int OriginTrialContext::checkFeatureEnabled(const String& featureName, String* errorMessage, WebTrialTokenValidator* validator) On 2016/04/25 at 20:32:52, chasej wrote: > I updated the comments on the method declarations. Suggestions welcome if there is better naming to make the differences more obvious. I think personally I would have made the split roughly the other way around: most of the code that is currently in isFeatureEnabled seems to be related to possibly generating the correct error message, so move that to a separate method and keep all the token checks in one method. But with the comments this works too. So either way fine with me.
Patchset #5 (id:80001) has been deleted
chasej@chromium.org changed reviewers: + asvitkine@chromium.org, iclelland@chromium.org
asvitkine, please take a look. Beyond the change to histograms.xml, I want to make sure I'm using the histograms appropriately in the Blink code (OriginTrialContext.cpp). iclelland, please take a look. Especially that I've captured the correct metrics.
https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... File content/common/origin_trials/trial_token.cc (right): https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... content/common/origin_trials/trial_token.cc:88: return blink::WebOriginTrialTokenStatus::Malformed; would an "Empty" status be more descriptive in this case? https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... content/common/origin_trials/trial_token.cc:103: return blink::WebOriginTrialTokenStatus::Malformed; This should definitely be a different status code -- if we start issuing v3 tokens, then we could see a lot of "Malformed" responses in UMA from older Chrome clients, when that's really not the case. https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... File content/common/origin_trials/trial_token.h (right): https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... content/common/origin_trials/trial_token.h:67: // If the string represents a properly signed and well-formed token, the JSON Extract explicitly doesn't care whether the payload is JSON; it will return any string which is correctly signed and wrapped. At a higher level, we presume that we wouldn't choose to sign a token which didn't contain valid JSON data, but that's not a guarantee that Extract makes. https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... content/common/origin_trials/trial_token.h:77: static std::unique_ptr<TrialToken> Parse(const std::string& token_json); There's an opportunity to change this to return a detailed status code here as well, but since any error would essentially be "Malformed", then I'm also fine with the way you've done it in From(). https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... File content/common/origin_trials/trial_token_validator.cc (right): https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... content/common/origin_trials/trial_token_validator.cc:33: return trial_token->IsValidForFeature(origin, feature_name, I'd be happier with a DCHECK(trial_token) here before we dereference it. I can see by inspection that you can't get Success with a nullptr, but an assertion would be good to ensure it. https://codereview.chromium.org/1909633003/diff/100001/content/renderer/origi... File content/renderer/origin_trials/web_trial_token_validator_impl.h (right): https://codereview.chromium.org/1909633003/diff/100001/content/renderer/origi... content/renderer/origin_trials/web_trial_token_validator_impl.h:16: enum class WebOriginTrialTokenStatus; wrap this in "namespace blink" instead, if you need to declare it here. (The one you're using below is being declared by including WebTrialTokenValidator.h, I think.) https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:298: WebOriginTrialTokenStatus failedValidationResult = WebOriginTrialTokenStatus::Success; If you initialize this to NoTokens, can you avoid checking m_tokens.size() in the return? https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right): https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:70: // |errorMessage| parameter may be updated with message for disabled "may be updated with message" is vague -- I'm not sure how easily we can be more preceise, though. Can we say that we will always provide an error description if the status is not Success (and if non-null, and if the framework is enabled)? https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp (right): https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:40: const char kGoodToken[] = "AnySignatureWillDo|https://www.example.com|Frobulate|2000000000"; We clearly don't actually do any parsing of this string, or else tests would fail. Could you change it to a string that makes that more obvious? https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebTrialTokenValidator.h (right): https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTrialTokenValidator.h:29: virtual WebOriginTrialTokenStatus validateToken( Do these need to be wrapped like this now? I've never noticed line length limits in public/platform files before.
https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... File content/common/origin_trials/trial_token.cc (right): https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... content/common/origin_trials/trial_token.cc:88: return blink::WebOriginTrialTokenStatus::Malformed; On 2016/04/28 17:00:58, iclelland wrote: > would an "Empty" status be more descriptive in this case? It would be more descriptive, but I don't see the value in recording the distinction in UMA. If a web developer makes the effort to add the meta tag, I'd think it's unlikely they would forget/omit the actual token (especially if the email provides the meta tag for copy/paste). Empty is a subset of "found a meta tag, but it didn't have a well-formed token". I think we would handle empty vs malformed most the same (e.g. guidance to make sure the token is copied as-is into the meta tag). Do you have any reasons why we need to distinguish "Empty" from the more general "Malformed" case? https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... content/common/origin_trials/trial_token.cc:103: return blink::WebOriginTrialTokenStatus::Malformed; On 2016/04/28 17:00:58, iclelland wrote: > This should definitely be a different status code -- if we start issuing v3 > tokens, then we could see a lot of "Malformed" responses in UMA from older > Chrome clients, when that's really not the case. Done. https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... File content/common/origin_trials/trial_token.h (right): https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... content/common/origin_trials/trial_token.h:67: // If the string represents a properly signed and well-formed token, the JSON On 2016/04/28 17:00:58, iclelland wrote: > Extract explicitly doesn't care whether the payload is JSON; it will return any > string which is correctly signed and wrapped. At a higher level, we presume that > we wouldn't choose to sign a token which didn't contain valid JSON data, but > that's not a guarantee that Extract makes. To me, this comment doesn't guarantee that valid JSON is returned, especially since the return value is a string. That said, I can see the potential for confusion. I've changed the parameter names. Note that this applied to Parse() as well. https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... content/common/origin_trials/trial_token.h:77: static std::unique_ptr<TrialToken> Parse(const std::string& token_json); On 2016/04/28 17:00:58, iclelland wrote: > There's an opportunity to change this to return a detailed status code here as > well, but since any error would essentially be "Malformed", then I'm also fine > with the way you've done it in From(). Yes, I decided not to return a status code because it made the code (slightly) more complicated for no real benefit. https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... File content/common/origin_trials/trial_token_validator.cc (right): https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... content/common/origin_trials/trial_token_validator.cc:33: return trial_token->IsValidForFeature(origin, feature_name, On 2016/04/28 17:00:58, iclelland wrote: > I'd be happier with a DCHECK(trial_token) here before we dereference it. I can > see by inspection that you can't get Success with a nullptr, but an assertion > would be good to ensure it. Does a DCHECK buy you much here? The pointer is provided by an internal method, not from an unknown caller. A unit test would give you equivalent/better assurances, and the documentation benefit seems minimal. https://codereview.chromium.org/1909633003/diff/100001/content/renderer/origi... File content/renderer/origin_trials/web_trial_token_validator_impl.h (right): https://codereview.chromium.org/1909633003/diff/100001/content/renderer/origi... content/renderer/origin_trials/web_trial_token_validator_impl.h:16: enum class WebOriginTrialTokenStatus; On 2016/04/28 17:00:58, iclelland wrote: > wrap this in "namespace blink" instead, if you need to declare it here. > > (The one you're using below is being declared by including > WebTrialTokenValidator.h, I think.) Wasn't needed, so just removed the declaration. https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:298: WebOriginTrialTokenStatus failedValidationResult = WebOriginTrialTokenStatus::Success; On 2016/04/28 17:00:58, iclelland wrote: > If you initialize this to NoTokens, can you avoid checking m_tokens.size() in > the return? Yes, you can. Done. https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right): https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:70: // |errorMessage| parameter may be updated with message for disabled On 2016/04/28 17:00:58, iclelland wrote: > "may be updated with message" is vague -- I'm not sure how easily we can be more > preceise, though. Can we say that we will always provide an error description if > the status is not Success (and if non-null, and if the framework is enabled)? No, it can't say it will always provide a message, unless all the message generation logic is moved into the method. The point of having this separate method was to separate message generation from the enabled check. A message will only be provided for insecure contexts, because it's a side effect of the secure context check. I've updated the comment to be more explicit. https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp (right): https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:40: const char kGoodToken[] = "AnySignatureWillDo|https://www.example.com|Frobulate|2000000000"; On 2016/04/28 17:00:58, iclelland wrote: > We clearly don't actually do any parsing of this string, or else tests would > fail. Could you change it to a string that makes that more obvious? Done. I've renamed the constant, and simplified the usage, to make it clear that it's just a placeholder. https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebTrialTokenValidator.h (right): https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTrialTokenValidator.h:29: virtual WebOriginTrialTokenStatus validateToken( On 2016/04/28 17:00:58, iclelland wrote: > Do these need to be wrapped like this now? I've never noticed line length limits > in public/platform files before. Victim of git cl format. Format seems to apply Chromium style, which sometimes violates the webkit-style presubmit check (and sometimes not). Frustrating! I've changed it back for now.
https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... File content/common/origin_trials/trial_token.cc (right): https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... content/common/origin_trials/trial_token.cc:88: return blink::WebOriginTrialTokenStatus::Malformed; On 2016/05/02 15:53:10, chasej wrote: > On 2016/04/28 17:00:58, iclelland wrote: > > would an "Empty" status be more descriptive in this case? > > It would be more descriptive, but I don't see the value in recording the > distinction in UMA. If a web developer makes the effort to add the meta tag, I'd > think it's unlikely they would forget/omit the actual token (especially if the > email provides the meta tag for copy/paste). Empty is a subset of "found a meta > tag, but it didn't have a well-formed token". I think we would handle empty vs > malformed most the same (e.g. guidance to make sure the token is copied as-is > into the meta tag). Do you have any reasons why we need to distinguish "Empty" > from the more general "Malformed" case? I suppose it's just an interesting specific variant -- it seems like it would have different fundamental causes than a general 'present-but-malformed' token. If we consider this as the status of the whole tag, or the whole header, then 'malformed' would make sense here, it just doesn't strike me as the right fit for the status of a single tag. Another thing to consider is what we are going to do with the UMA data -- can we use it to learn what kind of mistakes developers are making in the field, and try to adapt the API or the token-delivery mechanism to fit the developers? If all we have is 'malformed: xx%', then we don't get a lot of actionable data. If we know the breakdown of missing token vs truncated token vs completely-garbled (not-even-base64) token, then we might be able to do something with that data. (Maybe we can we get that kind of data outside of UMA? Perhaps through crawling, or httparchive?) https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... File content/common/origin_trials/trial_token.h (right): https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... content/common/origin_trials/trial_token.h:77: static std::unique_ptr<TrialToken> Parse(const std::string& token_json); On 2016/05/02 15:53:11, chasej wrote: > On 2016/04/28 17:00:58, iclelland wrote: > > There's an opportunity to change this to return a detailed status code here as > > well, but since any error would essentially be "Malformed", then I'm also fine > > with the way you've done it in From(). > > Yes, I decided not to return a status code because it made the code (slightly) > more complicated for no real benefit. Acknowledged. https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... File content/common/origin_trials/trial_token_validator.cc (right): https://codereview.chromium.org/1909633003/diff/100001/content/common/origin_... content/common/origin_trials/trial_token_validator.cc:33: return trial_token->IsValidForFeature(origin, feature_name, On 2016/05/02 15:53:11, chasej wrote: > On 2016/04/28 17:00:58, iclelland wrote: > > I'd be happier with a DCHECK(trial_token) here before we dereference it. I can > > see by inspection that you can't get Success with a nullptr, but an assertion > > would be good to ensure it. > > Does a DCHECK buy you much here? The pointer is provided by an internal method, > not from an unknown caller. A unit test would give you equivalent/better > assurances, and the documentation benefit seems minimal. Fair enough; a crash is a crash :) Documentation-wise, I guess issue could only come up if a future change (bug) is somehow made to ::From that causes the return to be null with a Success status. In that case, any documentation should really be on ::From, that it must return a valid object on success. https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right): https://codereview.chromium.org/1909633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:70: // |errorMessage| parameter may be updated with message for disabled On 2016/05/02 15:53:11, chasej wrote: > On 2016/04/28 17:00:58, iclelland wrote: > > "may be updated with message" is vague -- I'm not sure how easily we can be > more > > preceise, though. Can we say that we will always provide an error description > if > > the status is not Success (and if non-null, and if the framework is enabled)? > > No, it can't say it will always provide a message, unless all the message > generation logic is moved into the method. The point of having this separate > method was to separate message generation from the enabled check. A message will > only be provided for insecure contexts, because it's a side effect of the secure > context check. I've updated the comment to be more explicit. Acknowledged; makes sense. I like the new comment as well -- it conveys more useful information for future readers. https://codereview.chromium.org/1909633003/diff/120001/content/common/origin_... File content/common/origin_trials/trial_token_unittest.cc (right): https://codereview.chromium.org/1909633003/diff/120001/content/common/origin_... content/common/origin_trials/trial_token_unittest.cc:221: EXPECT_EQ(blink::WebOriginTrialTokenStatus::WrongVersion, status); Let's fix this todo in this CL. It shouldn't be hard to recreate; let me know if you want me to do it.
lgtm
https://codereview.chromium.org/1909633003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/1909633003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:108: if (currentResult == WebOriginTrialTokenStatus::NoTokens || getTokenValidationResultPriority(newResult) < getTokenValidationResultPriority(currentResult)) { Not sure if the code would be more or less clear if rather then explicitly handling NoTokens here you would just change getTokenValidationResultPriority to return the highest value for NoTokens. https://codereview.chromium.org/1909633003/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1909633003/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:79197: +</enum> You're missing WrongVersion
https://codereview.chromium.org/1909633003/diff/120001/content/common/origin_... File content/common/origin_trials/trial_token_unittest.cc (right): https://codereview.chromium.org/1909633003/diff/120001/content/common/origin_... content/common/origin_trials/trial_token_unittest.cc:221: EXPECT_EQ(blink::WebOriginTrialTokenStatus::WrongVersion, status); On 2016/05/02 16:17:55, iclelland wrote: > Let's fix this todo in this CL. It shouldn't be hard to recreate; let me know if > you want me to do it. Yes, it was my intent to fix in this CL, once the bigger changes were in good shape. Done. https://codereview.chromium.org/1909633003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/1909633003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:108: if (currentResult == WebOriginTrialTokenStatus::NoTokens || getTokenValidationResultPriority(newResult) < getTokenValidationResultPriority(currentResult)) { On 2016/05/02 18:31:34, Marijn Kruisselbrink wrote: > Not sure if the code would be more or less clear if rather then explicitly > handling NoTokens here you would just change getTokenValidationResultPriority to > return the highest value for NoTokens. I think it's probably clearer as you suggest. Done. https://codereview.chromium.org/1909633003/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1909633003/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:79197: +</enum> On 2016/05/02 18:31:34, Marijn Kruisselbrink wrote: > You're missing WrongVersion Good catch - added.
chasej@chromium.org changed reviewers: + jochen@chromium.org
jochen, please take a look. Primarily need owner review for content/common/DEPS and third_party/WebKit/*.
lgtm https://codereview.chromium.org/1909633003/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebOriginTrialTokenStatus.h (right): https://codereview.chromium.org/1909633003/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebOriginTrialTokenStatus.h:14: Success = 0, what's the point of specifying the values? as far as I can see, they match exactly what would be auto-assigned
lgtm
https://codereview.chromium.org/1909633003/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebOriginTrialTokenStatus.h (right): https://codereview.chromium.org/1909633003/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebOriginTrialTokenStatus.h:14: Success = 0, On 2016/05/04 10:55:26, jochen wrote: > what's the point of specifying the values? as far as I can see, they match > exactly what would be auto-assigned I wanted the values to be explicit, to make it easier to match against the histogram enum values. I think being explicit also helps guard against future additions/deletion changing the values.
The CQ bit was checked by chasej@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mek@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1909633003/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1909633003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1909633003/160001
Message was sent while issue was closed.
Description was changed from ========== Collect UMA data for Origin Trials This CL records UMA metrics for the results of feature enabled checks, one per context. Also records metrics for when error messages are generated. Token validation is controlled by the embedder, but the enabled check is performed in Blink. To collect the desired metrics, it was necessary to plumb a detailed validation return code through the chain. BUG=587575 ========== to ========== Collect UMA data for Origin Trials This CL records UMA metrics for the results of feature enabled checks, one per context. Also records metrics for when error messages are generated. Token validation is controlled by the embedder, but the enabled check is performed in Blink. To collect the desired metrics, it was necessary to plumb a detailed validation return code through the chain. BUG=587575 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Collect UMA data for Origin Trials This CL records UMA metrics for the results of feature enabled checks, one per context. Also records metrics for when error messages are generated. Token validation is controlled by the embedder, but the enabled check is performed in Blink. To collect the desired metrics, it was necessary to plumb a detailed validation return code through the chain. BUG=587575 ========== to ========== Collect UMA data for Origin Trials This CL records UMA metrics for the results of feature enabled checks, one per context. Also records metrics for when error messages are generated. Token validation is controlled by the embedder, but the enabled check is performed in Blink. To collect the desired metrics, it was necessary to plumb a detailed validation return code through the chain. BUG=587575 Committed: https://crrev.com/455dbe71939225cb3765da12c6aed749b72276ea Cr-Commit-Position: refs/heads/master@{#391505} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/455dbe71939225cb3765da12c6aed749b72276ea Cr-Commit-Position: refs/heads/master@{#391505} |
