Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1752463002:
Cleanup TrialToken in preparation of moving it to content/common. (Closed)
|
Created:
4 years, 9 months ago by Marijn Kruisselbrink Modified:
4 years, 9 months ago CC:
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. |
DescriptionCleanup TrialToken in preparation of moving it to content/common.
This includes the following cleanups:
- Pass StringPiece by value rather than const ref, as its documentation
suggests.
- Represent origins as url::Origin/blink::WebSecurityOrigin.
- Pass feature_name to TrialToken::IsAppropriate and related methods as
StringPiece. Currently doesn't make a difference yet as TrialTokenValidator
still converts through std::string, but that can be fixed, and new content
side code can avoid having to construct strings for feature names this way.
- Store expiry time as base::Time instead of raw int64. Under the hood a
base::Time is an int64 anyway, so this just moves the conversion from int64
to base::Time earlier, making the code more accurately reflect what it
represents.
- C-style cast in passing int64 to base::Time::FromDoubleT is against the
style guide, and unnecessary since this is done implicitly by the compiler.
- Fix order of methods in trial_token.cc to match that in trial_token.h
- Use correct style for argument names in trial_token.h (featureName =>
feature_name).
BUG=590873
Committed: https://crrev.com/6cad14e7709b9578100415a641ec1ea3f0268e27
Cr-Commit-Position: refs/heads/master@{#380537}
Patch Set 1 #
Total comments: 6
Patch Set 2 : remove unneeded includes #Patch Set 3 : no more TrialToken::Verify #Patch Set 4 : rebase #
Total comments: 4
Patch Set 5 : nits #Messages
Total messages: 39 (20 generated)
Description was changed from ========== WIP Cleanup TrialToken in preparation of moving it content/common. BUG= ========== to ========== Cleanup TrialToken in preparation of moving it content/common. In addition to adding a TrialToken::Verify method to lookup signing keys and verifying the validity of a token (which was previously done in TrialTokenValidator), this also includes the following cleanups: - Pass StringPiece by value rather than const ref, as its documentation suggests. - Represent origins as url::Origin/blink::WebSecurityOrigin. - Pass feature_name to TrialToken::IsAppropriate and related methods as StringPiece. Currently doesn't make a difference yet as TrialTokenValidator still converts through std::string, but that can be fixed, and new content side code can avoid having to construct strings for feature names this way. - Store expiry time as base::Time instead of raw int64. Under the hood a base::Time is an int64 anyway, so this just moves the conversion from int64 to base::Time earlier, making the code more accurately reflect what it represents. - C-style cast in passing int64 to base::Time::FromDoubleT is against the style guide, and unnecessary since this is done implicitly by the compiler. - Fix order of methods in trial_token.cc to match that in trial_token.h BUG= ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Cleanup TrialToken in preparation of moving it content/common. In addition to adding a TrialToken::Verify method to lookup signing keys and verifying the validity of a token (which was previously done in TrialTokenValidator), this also includes the following cleanups: - Pass StringPiece by value rather than const ref, as its documentation suggests. - Represent origins as url::Origin/blink::WebSecurityOrigin. - Pass feature_name to TrialToken::IsAppropriate and related methods as StringPiece. Currently doesn't make a difference yet as TrialTokenValidator still converts through std::string, but that can be fixed, and new content side code can avoid having to construct strings for feature names this way. - Store expiry time as base::Time instead of raw int64. Under the hood a base::Time is an int64 anyway, so this just moves the conversion from int64 to base::Time earlier, making the code more accurately reflect what it represents. - C-style cast in passing int64 to base::Time::FromDoubleT is against the style guide, and unnecessary since this is done implicitly by the compiler. - Fix order of methods in trial_token.cc to match that in trial_token.h BUG= ========== to ========== Cleanup TrialToken in preparation of moving it content/common. In addition to adding a TrialToken::Verify method to lookup signing keys and verifying the validity of a token (which was previously done in TrialTokenValidator), this also includes the following cleanups: - Pass StringPiece by value rather than const ref, as its documentation suggests. - Represent origins as url::Origin/blink::WebSecurityOrigin. - Pass feature_name to TrialToken::IsAppropriate and related methods as StringPiece. Currently doesn't make a difference yet as TrialTokenValidator still converts through std::string, but that can be fixed, and new content side code can avoid having to construct strings for feature names this way. - Store expiry time as base::Time instead of raw int64. Under the hood a base::Time is an int64 anyway, so this just moves the conversion from int64 to base::Time earlier, making the code more accurately reflect what it represents. - C-style cast in passing int64 to base::Time::FromDoubleT is against the style guide, and unnecessary since this is done implicitly by the compiler. - Fix order of methods in trial_token.cc to match that in trial_token.h - Use correct style for argument names in trial_token.h (featureName => feature_name). BUG= ==========
Description was changed from ========== Cleanup TrialToken in preparation of moving it content/common. In addition to adding a TrialToken::Verify method to lookup signing keys and verifying the validity of a token (which was previously done in TrialTokenValidator), this also includes the following cleanups: - Pass StringPiece by value rather than const ref, as its documentation suggests. - Represent origins as url::Origin/blink::WebSecurityOrigin. - Pass feature_name to TrialToken::IsAppropriate and related methods as StringPiece. Currently doesn't make a difference yet as TrialTokenValidator still converts through std::string, but that can be fixed, and new content side code can avoid having to construct strings for feature names this way. - Store expiry time as base::Time instead of raw int64. Under the hood a base::Time is an int64 anyway, so this just moves the conversion from int64 to base::Time earlier, making the code more accurately reflect what it represents. - C-style cast in passing int64 to base::Time::FromDoubleT is against the style guide, and unnecessary since this is done implicitly by the compiler. - Fix order of methods in trial_token.cc to match that in trial_token.h - Use correct style for argument names in trial_token.h (featureName => feature_name). BUG= ========== to ========== Cleanup TrialToken in preparation of moving it content/common. In addition to adding a TrialToken::Verify method to lookup signing keys and verifying the validity of a token (which was previously done in TrialTokenValidator), this also includes the following cleanups: - Pass StringPiece by value rather than const ref, as its documentation suggests. - Represent origins as url::Origin/blink::WebSecurityOrigin. - Pass feature_name to TrialToken::IsAppropriate and related methods as StringPiece. Currently doesn't make a difference yet as TrialTokenValidator still converts through std::string, but that can be fixed, and new content side code can avoid having to construct strings for feature names this way. - Store expiry time as base::Time instead of raw int64. Under the hood a base::Time is an int64 anyway, so this just moves the conversion from int64 to base::Time earlier, making the code more accurately reflect what it represents. - C-style cast in passing int64 to base::Time::FromDoubleT is against the style guide, and unnecessary since this is done implicitly by the compiler. - Fix order of methods in trial_token.cc to match that in trial_token.h - Use correct style for argument names in trial_token.h (featureName => feature_name). BUG=590873 ==========
mek@chromium.org changed reviewers: + iclelland@chromium.org
Description was changed from ========== Cleanup TrialToken in preparation of moving it content/common. In addition to adding a TrialToken::Verify method to lookup signing keys and verifying the validity of a token (which was previously done in TrialTokenValidator), this also includes the following cleanups: - Pass StringPiece by value rather than const ref, as its documentation suggests. - Represent origins as url::Origin/blink::WebSecurityOrigin. - Pass feature_name to TrialToken::IsAppropriate and related methods as StringPiece. Currently doesn't make a difference yet as TrialTokenValidator still converts through std::string, but that can be fixed, and new content side code can avoid having to construct strings for feature names this way. - Store expiry time as base::Time instead of raw int64. Under the hood a base::Time is an int64 anyway, so this just moves the conversion from int64 to base::Time earlier, making the code more accurately reflect what it represents. - C-style cast in passing int64 to base::Time::FromDoubleT is against the style guide, and unnecessary since this is done implicitly by the compiler. - Fix order of methods in trial_token.cc to match that in trial_token.h - Use correct style for argument names in trial_token.h (featureName => feature_name). BUG=590873 ========== to ========== Cleanup TrialToken in preparation of moving it to content/common. In addition to adding a TrialToken::Verify method to lookup signing keys and verifying the validity of a token (which was previously done in TrialTokenValidator), this also includes the following cleanups: - Pass StringPiece by value rather than const ref, as its documentation suggests. - Represent origins as url::Origin/blink::WebSecurityOrigin. - Pass feature_name to TrialToken::IsAppropriate and related methods as StringPiece. Currently doesn't make a difference yet as TrialTokenValidator still converts through std::string, but that can be fixed, and new content side code can avoid having to construct strings for feature names this way. - Store expiry time as base::Time instead of raw int64. Under the hood a base::Time is an int64 anyway, so this just moves the conversion from int64 to base::Time earlier, making the code more accurately reflect what it represents. - C-style cast in passing int64 to base::Time::FromDoubleT is against the style guide, and unnecessary since this is done implicitly by the compiler. - Fix order of methods in trial_token.cc to match that in trial_token.h - Use correct style for argument names in trial_token.h (featureName => feature_name). BUG=590873 ==========
ALl of this looks pretty good -- I'm just not sure about the reasoning behind moving the GetContentClient() calls into the token object. https://codereview.chromium.org/1752463002/diff/20001/content/renderer/origin... File content/renderer/origin_trials/trial_token.cc (right): https://codereview.chromium.org/1752463002/diff/20001/content/renderer/origin... content/renderer/origin_trials/trial_token.cc:98: ContentClient* content_client = GetContentClient(); Why is this logic moved here? The separation between the token and the validator was originally so that the token could exist outside of any particular renderer, but would be considered valid (or not) only within the context of a ContentRenderClient, which the validator was bound to. https://codereview.chromium.org/1752463002/diff/20001/content/renderer/origin... File content/renderer/origin_trials/trial_token.h (right): https://codereview.chromium.org/1752463002/diff/20001/content/renderer/origin... content/renderer/origin_trials/trial_token.h:14: #include "url/gurl.h" Is this header needed anymore? https://codereview.chromium.org/1752463002/diff/20001/content/renderer/origin... File content/renderer/origin_trials/trial_token_validator.cc (right): https://codereview.chromium.org/1752463002/diff/20001/content/renderer/origin... content/renderer/origin_trials/trial_token_validator.cc:11: #include "third_party/WebKit/public/platform/URLConversion.h" What's this include for?
https://codereview.chromium.org/1752463002/diff/20001/content/renderer/origin... File content/renderer/origin_trials/trial_token.cc (right): https://codereview.chromium.org/1752463002/diff/20001/content/renderer/origin... content/renderer/origin_trials/trial_token.cc:98: ContentClient* content_client = GetContentClient(); On 2016/03/02 at 22:36:29, iclelland wrote: > Why is this logic moved here? The separation between the token and the validator was originally so that the token could exist outside of any particular renderer, but would be considered valid (or not) only within the context of a ContentRenderClient, which the validator was bound to. I could have left this in part 2 of my change instead of course. But this had to move somewhere other than TrialTokenValidator because TrialTokenValidator is renderer only and the implementation of a blink interface. And the whole point is that I'm trying to enable verification of tokens outside of the renderer. If you'd rather keep it separate, I can rename the existing TrialTokenValidator to WebTrialTokenValidatorImpl, and add a new TrialTokenValidator class that just does the validation? (and I could even move this part of the CL to my other CL where I make the actual move to content/common, if that makes you happier). https://codereview.chromium.org/1752463002/diff/20001/content/renderer/origin... File content/renderer/origin_trials/trial_token.h (right): https://codereview.chromium.org/1752463002/diff/20001/content/renderer/origin... content/renderer/origin_trials/trial_token.h:14: #include "url/gurl.h" On 2016/03/02 at 22:36:29, iclelland wrote: > Is this header needed anymore? No, removed. https://codereview.chromium.org/1752463002/diff/20001/content/renderer/origin... File content/renderer/origin_trials/trial_token_validator.cc (right): https://codereview.chromium.org/1752463002/diff/20001/content/renderer/origin... content/renderer/origin_trials/trial_token_validator.cc:11: #include "third_party/WebKit/public/platform/URLConversion.h" On 2016/03/02 at 22:36:29, iclelland wrote: > What's this include for? Nothing. Leftover from before I changed the origin parameter to be a blink::WebSecurityOrigin.
Patchset #3 (id:60001) has been deleted
Description was changed from ========== Cleanup TrialToken in preparation of moving it to content/common. In addition to adding a TrialToken::Verify method to lookup signing keys and verifying the validity of a token (which was previously done in TrialTokenValidator), this also includes the following cleanups: - Pass StringPiece by value rather than const ref, as its documentation suggests. - Represent origins as url::Origin/blink::WebSecurityOrigin. - Pass feature_name to TrialToken::IsAppropriate and related methods as StringPiece. Currently doesn't make a difference yet as TrialTokenValidator still converts through std::string, but that can be fixed, and new content side code can avoid having to construct strings for feature names this way. - Store expiry time as base::Time instead of raw int64. Under the hood a base::Time is an int64 anyway, so this just moves the conversion from int64 to base::Time earlier, making the code more accurately reflect what it represents. - C-style cast in passing int64 to base::Time::FromDoubleT is against the style guide, and unnecessary since this is done implicitly by the compiler. - Fix order of methods in trial_token.cc to match that in trial_token.h - Use correct style for argument names in trial_token.h (featureName => feature_name). BUG=590873 ========== to ========== Cleanup TrialToken in preparation of moving it to content/common. This includes the following cleanups: - Pass StringPiece by value rather than const ref, as its documentation suggests. - Represent origins as url::Origin/blink::WebSecurityOrigin. - Pass feature_name to TrialToken::IsAppropriate and related methods as StringPiece. Currently doesn't make a difference yet as TrialTokenValidator still converts through std::string, but that can be fixed, and new content side code can avoid having to construct strings for feature names this way. - Store expiry time as base::Time instead of raw int64. Under the hood a base::Time is an int64 anyway, so this just moves the conversion from int64 to base::Time earlier, making the code more accurately reflect what it represents. - C-style cast in passing int64 to base::Time::FromDoubleT is against the style guide, and unnecessary since this is done implicitly by the compiler. - Fix order of methods in trial_token.cc to match that in trial_token.h - Use correct style for argument names in trial_token.h (featureName => feature_name). BUG=590873 ==========
In the latest version of this CL I undid the changes to add TrialToken::Verify. I'll redo that differently in the follow-up patch where I actually move things to content/common.
On 2016/03/03 23:59:26, Marijn Kruisselbrink wrote: > In the latest version of this CL I undid the changes to add TrialToken::Verify. > I'll redo that differently in the follow-up patch where I actually move things > to content/common. Thanks -- this one LGTM then, and I'll take a close look at the follow-up.
mek@chromium.org changed reviewers: + jam@chromium.org, rbyers@chromium.org
+jam for content/ OWNERS +rbyers for third_party/WebKit OWNERS
On 2016/03/08 20:25:02, Marijn Kruisselbrink wrote: > +jam for content/ OWNERS > +rbyers for third_party/WebKit OWNERS WebKit LGTM
mek@chromium.org changed reviewers: + avi@chromium.org
+avi for content/ OWNERS
lgtm nits https://codereview.chromium.org/1752463002/diff/100001/content/renderer/origi... File content/renderer/origin_trials/trial_token.cc (left): https://codereview.chromium.org/1752463002/diff/100001/content/renderer/origi... content/renderer/origin_trials/trial_token.cc:113: // active keys here. https://crbug.com/543220 Is the removal of the comment intentional? https://codereview.chromium.org/1752463002/diff/100001/content/renderer/origi... File content/renderer/origin_trials/trial_token_validator.cc (left): https://codereview.chromium.org/1752463002/diff/100001/content/renderer/origi... content/renderer/origin_trials/trial_token_validator.cc:27: Why the removal of these lines?
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/1752463002/diff/100001/content/renderer/origi... File content/renderer/origin_trials/trial_token.cc (left): https://codereview.chromium.org/1752463002/diff/100001/content/renderer/origi... content/renderer/origin_trials/trial_token.cc:113: // active keys here. https://crbug.com/543220 On 2016/03/10 at 22:55:29, Avi wrote: > Is the removal of the comment intentional? No, in an earlier version of this CL I moved to comment to a new method, but I then moved that new method to a separate CL. Put it back here for now. https://codereview.chromium.org/1752463002/diff/100001/content/renderer/origi... File content/renderer/origin_trials/trial_token_validator.cc (left): https://codereview.chromium.org/1752463002/diff/100001/content/renderer/origi... content/renderer/origin_trials/trial_token_validator.cc:27: On 2016/03/10 at 22:55:29, Avi wrote: > Why the removal of these lines? In an earlier version of this CL I got rid of most of the implementation of this method, and I must somehow have messed up undoing that change.
The CQ bit was checked by mek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iclelland@chromium.org, avi@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/1752463002/#ps140001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1752463002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1752463002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mek@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1752463002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1752463002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mek@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1752463002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1752463002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mek@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1752463002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1752463002/140001
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Cleanup TrialToken in preparation of moving it to content/common. This includes the following cleanups: - Pass StringPiece by value rather than const ref, as its documentation suggests. - Represent origins as url::Origin/blink::WebSecurityOrigin. - Pass feature_name to TrialToken::IsAppropriate and related methods as StringPiece. Currently doesn't make a difference yet as TrialTokenValidator still converts through std::string, but that can be fixed, and new content side code can avoid having to construct strings for feature names this way. - Store expiry time as base::Time instead of raw int64. Under the hood a base::Time is an int64 anyway, so this just moves the conversion from int64 to base::Time earlier, making the code more accurately reflect what it represents. - C-style cast in passing int64 to base::Time::FromDoubleT is against the style guide, and unnecessary since this is done implicitly by the compiler. - Fix order of methods in trial_token.cc to match that in trial_token.h - Use correct style for argument names in trial_token.h (featureName => feature_name). BUG=590873 ========== to ========== Cleanup TrialToken in preparation of moving it to content/common. This includes the following cleanups: - Pass StringPiece by value rather than const ref, as its documentation suggests. - Represent origins as url::Origin/blink::WebSecurityOrigin. - Pass feature_name to TrialToken::IsAppropriate and related methods as StringPiece. Currently doesn't make a difference yet as TrialTokenValidator still converts through std::string, but that can be fixed, and new content side code can avoid having to construct strings for feature names this way. - Store expiry time as base::Time instead of raw int64. Under the hood a base::Time is an int64 anyway, so this just moves the conversion from int64 to base::Time earlier, making the code more accurately reflect what it represents. - C-style cast in passing int64 to base::Time::FromDoubleT is against the style guide, and unnecessary since this is done implicitly by the compiler. - Fix order of methods in trial_token.cc to match that in trial_token.h - Use correct style for argument names in trial_token.h (featureName => feature_name). BUG=590873 Committed: https://crrev.com/6cad14e7709b9578100415a641ec1ea3f0268e27 Cr-Commit-Position: refs/heads/master@{#380537} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6cad14e7709b9578100415a641ec1ea3f0268e27 Cr-Commit-Position: refs/heads/master@{#380537} |
