Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2449853008:
Determine the Win10-specific Welcome page variant to display (Closed)
|
Created:
4 years, 1 month ago by Patrick Monette Modified:
4 years, 1 month ago CC:
arv+watch_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDetermine the Win10-specific Welcome page variant to display
An asynchronous check to determine Chrome's pinned state is done in order
to decide if the "Pin Chrome to taskbar" instructions should be displayed.
BUG=648686
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/b9f756c3b1853a17cd4cfbfcb003f52bd2ae8083
Cr-Commit-Position: refs/heads/master@{#430793}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Responding to comments #
Total comments: 6
Patch Set 3 : comments #2 #
Total comments: 2
Patch Set 4 : Nit #
Total comments: 7
Patch Set 5 : Nits + last minute request #
Total comments: 2
Patch Set 6 : Comments #
Total comments: 5
Patch Set 7 : More nits #
Messages
Total messages: 40 (15 generated)
Description was changed from ========== Determine the Win10-specific Welcome page variant to display An asynchronous check to determine Chrome's pinned state is done in order to decide if the "Pin Chrome to taskbar" instructions should be displayed. BUG=648686 ========== to ========== Determine the Win10-specific Welcome page variant to display An asynchronous check to determine Chrome's pinned state is done in order to decide if the "Pin Chrome to taskbar" instructions should be displayed. BUG=648686 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
pmonette@chromium.org changed reviewers: + tmartino@chromium.org
Hey Tommy! Care to take a look?
Looks great, just some minor stuff for clarity, and one idea for a perf improvement. Thanks! https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline.js (right): https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.js:46: // Show the combined variant if Chrome is not pinned. Can we document this in the first place the variable is introduced (I believe line 34 above)? https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.cc:49: base::TimeDelta::FromMilliseconds(2000); 2s seems quite high. https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.cc:53: // Start the utility process that will handle the IsPinnedToTaskbar() call. Would it be possible to start the check in the ctor and cache the result? Then when the page asks whether or not we're pinned, we either returned the cached value if we have it, or kick off the timer and do the callback at the first of {result returned, timer expires}. (Feel free to push back if you don't think the possible perf improvement would justify the added complexity.) https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_handler.h (right): https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.h:36: // Called when the pinned state is received from the utility process. The relationship between this method and OnIsPinnedToTaskbarDetermined isn't clear to me from the documentation. Could you address that point directly? https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_ui.cc (right): https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:35: bool ShouldShouldInlineStyleVariant(const GURL& url) { ShouldShould -> ShouldShow? https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:36: constexpr base::Feature kWelcomeWin10InlineStyle{ Let's put this (and any other variations/experiments/etc. relating to startup/first run) in https://cs.chromium.org/chromium/src/chrome/browser/ui/startup/startup_featur... I made it just for this :)
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Thanks for the comments. Take another look! https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline.js (right): https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.js:46: // Show the combined variant if Chrome is not pinned. On 2016/10/28 15:18:04, tmartino wrote: > Can we document this in the first place the variable is introduced (I believe > line 34 above)? Done. https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.cc:49: base::TimeDelta::FromMilliseconds(2000); On 2016/10/28 15:18:04, tmartino wrote: > 2s seems quite high. I've put 200ms. On my debug version of Chrome, the result is always available before getPinnedToTaskbarState() is called, so it should be fine. https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.cc:53: // Start the utility process that will handle the IsPinnedToTaskbar() call. On 2016/10/28 15:18:04, tmartino wrote: > Would it be possible to start the check in the ctor and cache the result? Then > when the page asks whether or not we're pinned, we either returned the cached > value if we have it, or kick off the timer and do the callback at the first of > {result returned, timer expires}. > > (Feel free to push back if you don't think the possible perf improvement would > justify the added complexity.) Seems reasonable. Done. https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_handler.h (right): https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.h:36: // Called when the pinned state is received from the utility process. On 2016/10/28 15:18:04, tmartino wrote: > The relationship between this method and OnIsPinnedToTaskbarDetermined isn't > clear to me from the documentation. Could you address that point directly? Done. https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_ui.cc (right): https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:35: bool ShouldShouldInlineStyleVariant(const GURL& url) { On 2016/10/28 15:18:04, tmartino wrote: > ShouldShould -> ShouldShow? Done. https://codereview.chromium.org/2449853008/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:36: constexpr base::Feature kWelcomeWin10InlineStyle{ On 2016/10/28 15:18:04, tmartino wrote: > Let's put this (and any other variations/experiments/etc. relating to > startup/first run) in > https://cs.chromium.org/chromium/src/chrome/browser/ui/startup/startup_featur... > > I made it just for this :) Done.
lgtm https://codereview.chromium.org/2449853008/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2449853008/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.cc:16: // The check is started as early as possible becasue waiting for the page to becasue -> because https://codereview.chromium.org/2449853008/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.cc:17: // be fully loaded is unnecessarily wasiting time. wasiting -> wasting https://codereview.chromium.org/2449853008/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_handler.h (right): https://codereview.chromium.org/2449853008/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.h:57: base::Optional<bool> pinned_state_result_; Any meaningful race conditions between the JS handler and the callback kicked off in the ctor?
pmonette@chromium.org changed reviewers: + michaelpg@chromium.org
tmartino@ Thanks for the review! Can you take a look please michaelpg@? https://codereview.chromium.org/2449853008/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2449853008/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.cc:16: // The check is started as early as possible becasue waiting for the page to On 2016/10/28 21:44:15, tmartino wrote: > becasue -> because Done. https://codereview.chromium.org/2449853008/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.cc:17: // be fully loaded is unnecessarily wasiting time. On 2016/10/28 21:44:15, tmartino wrote: > wasiting -> wasting Done. https://codereview.chromium.org/2449853008/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_handler.h (right): https://codereview.chromium.org/2449853008/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.h:57: base::Optional<bool> pinned_state_result_; On 2016/10/28 21:44:15, tmartino wrote: > Any meaningful race conditions between the JS handler and the callback kicked > off in the ctor? It's all happening on the same thread. I added dchecks for it.
lgtm https://codereview.chromium.org/2449853008/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2449853008/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/welcome_win10_handler.cc:56: static constexpr base::TimeDelta kPinnedToTaskbarTimeout = so it seems this is allowed because it's |constexpr|, but why make this static at all?
pmonette@chromium.org changed reviewers: + pkasting@chromium.org
Thanks! Hi pkasting@, can you take a look at startup_features please. https://codereview.chromium.org/2449853008/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2449853008/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/welcome_win10_handler.cc:56: static constexpr base::TimeDelta kPinnedToTaskbarTimeout = On 2016/10/31 21:04:59, michaelpg wrote: > so it seems this is allowed because it's |constexpr|, but why make this static > at all? Removed static.
LGTM https://codereview.chromium.org/2449853008/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2449853008/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/welcome_win10_handler.cc:43: CHECK_EQ(1U, args->GetSize()); Nit: Normally CHECK is only for temporary debugging (to see what assertion is failing in the field) or for security-critical issues where if an assertion is violated we'll have an exploitable hole. If these conditions cannot be violated (because you know the caller guarantees them), then you should probably be using DCHECK instead. If they can be (with unvalidated input from the user or something), then you should be checking them conditionally and handling failure. https://codereview.chromium.org/2449853008/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/welcome_win10_handler.cc:46: CHECK(args->GetString(0, &pinned_state_callback_id_)); Nit: While technically this is safe because CHECK always runs, I suggest not doing work with side effects in a CHECK. It's easy to change it to a DCHECK later (e.g. due to my above comment) and not realize that now the behavior differs between builds. https://codereview.chromium.org/2449853008/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/welcome_win10_handler.cc:78: client_.reset(new content::UtilityProcessMojoClient<mojom::ShellHandler>( Nit: Prefer base::MakeUnique() to raw new: client_ = base::MakeUnique<content::UtilityProcessMojoClient<mojom::ShellHandler>>( ...);
/cc dbeam for interesting concerns about CHECKing args, which seems to only happen in Settings. In fact, WebUIMessageHandler has a ton of Extract*Value helpers for this (which use NOTREACHED instead of CHECK) but those only handle one arg. https://codereview.chromium.org/2449853008/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2449853008/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/welcome_win10_handler.cc:43: CHECK_EQ(1U, args->GetSize()); On 2016/11/02 00:03:21, Peter Kasting wrote: > Nit: Normally CHECK is only for temporary debugging (to see what assertion is > failing in the field) or for security-critical issues where if an assertion is > violated we'll have an exploitable hole. > > If these conditions cannot be violated (because you know the caller guarantees > them), then you should probably be using DCHECK instead. If they can be (with > unvalidated input from the user or something), then you should be checking them > conditionally and handling failure. +dbeam These CHECKs are very common in MD Settings WebUI handlers, for some reason. https://cs.chromium.org/search/?q=%5Cscheck_eq%5C(%5Cd.?,%5Csargs+file:webui&...
On 2016/11/02 01:04:45, michaelpg wrote: > /cc dbeam for interesting concerns about CHECKing args, which seems to only > happen in Settings. In fact, WebUIMessageHandler has a ton of Extract*Value > helpers for this (which use NOTREACHED instead of CHECK) but those only handle > one arg. Yeah, some of those could be made simpler or something. FWIW, here's the relevant Chromium style guide section on all this: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
On 2016/11/02 01:12:35, Peter Kasting wrote: > On 2016/11/02 01:04:45, michaelpg wrote: > > /cc dbeam for interesting concerns about CHECKing args, which seems to only > > happen in Settings. In fact, WebUIMessageHandler has a ton of Extract*Value > > helpers for this (which use NOTREACHED instead of CHECK) but those only handle > > one arg. > > Yeah, some of those could be made simpler or something. > > FWIW, here's the relevant Chromium style guide section on all this: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Not sure if I've always know this rule, but we've always been fairly loose about using release/debug combinations. I don't know which of these failing in settings would be a potential security vulnerability (i suspect few). Happy to change to debug-only versions if that helps make it clearer for those just copying code.
On 2016/11/02 01:19:06, Dan Beam wrote: > On 2016/11/02 01:12:35, Peter Kasting wrote: > > On 2016/11/02 01:04:45, michaelpg wrote: > > > /cc dbeam for interesting concerns about CHECKing args, which seems to only > > > happen in Settings. In fact, WebUIMessageHandler has a ton of Extract*Value > > > helpers for this (which use NOTREACHED instead of CHECK) but those only > handle > > > one arg. > > > > Yeah, some of those could be made simpler or something. > > > > FWIW, here's the relevant Chromium style guide section on all this: > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > Not sure if I've always know this rule, but we've always been fairly loose about > using release/debug combinations. I don't know which of these failing in > settings would be a potential security vulnerability (i suspect few). Happy to > change to debug-only versions if that helps make it clearer for those just > copying code. if this rule has always existed (and I just didn't know it) or what**
On 2016/11/02 01:19:33, Dan Beam wrote: > On 2016/11/02 01:19:06, Dan Beam wrote: > > On 2016/11/02 01:12:35, Peter Kasting wrote: > > > On 2016/11/02 01:04:45, michaelpg wrote: > > > > /cc dbeam for interesting concerns about CHECKing args, which seems to > only > > > > happen in Settings. In fact, WebUIMessageHandler has a ton of > Extract*Value > > > > helpers for this (which use NOTREACHED instead of CHECK) but those only > > handle > > > > one arg. > > > > > > Yeah, some of those could be made simpler or something. > > > > > > FWIW, here's the relevant Chromium style guide section on all this: > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > Not sure if I've always know this rule, but we've always been fairly loose > about > > using release/debug combinations. I don't know which of these failing in > > settings would be a potential security vulnerability (i suspect few). Happy > to > > change to debug-only versions if that helps make it clearer for those just > > copying code. > > if this rule has always existed (and I just didn't know it) or what** It has, but we've tried to make things a bit clearer over time. Sounds like probably just changing to DCHECK is the right thing here.
Hey, sorry for the last-minute comment. Can we add a URL param to manually trigger whether or not we show the pinned content? It was requested yesterday by PM/UX to aid the review process.
Patchset #5 (id:140001) has been deleted
Thanks Peter. Can you take a last look for the final addition michaelpg@? I implemented the logic using the least intrusive method possible. https://codereview.chromium.org/2449853008/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2449853008/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/welcome_win10_handler.cc:43: CHECK_EQ(1U, args->GetSize()); On 2016/11/02 01:04:45, michaelpg wrote: > On 2016/11/02 00:03:21, Peter Kasting wrote: > > Nit: Normally CHECK is only for temporary debugging (to see what assertion is > > failing in the field) or for security-critical issues where if an assertion is > > violated we'll have an exploitable hole. > > > > If these conditions cannot be violated (because you know the caller guarantees > > them), then you should probably be using DCHECK instead. If they can be (with > > unvalidated input from the user or something), then you should be checking > them > > conditionally and handling failure. > > +dbeam > > These CHECKs are very common in MD Settings WebUI handlers, for some reason. > > https://cs.chromium.org/search/?q=%5Cscheck_eq%5C(%5Cd.?,%5Csargs+file:webui&... I had effectively just copied code in other MD settings handlers. I switched to DCHECK. https://codereview.chromium.org/2449853008/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/welcome_win10_handler.cc:46: CHECK(args->GetString(0, &pinned_state_callback_id_)); On 2016/11/02 00:03:21, Peter Kasting wrote: > Nit: While technically this is safe because CHECK always runs, I suggest not > doing work with side effects in a CHECK. It's easy to change it to a DCHECK > later (e.g. due to my above comment) and not realize that now the behavior > differs between builds. Done. https://codereview.chromium.org/2449853008/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/welcome_win10_handler.cc:78: client_.reset(new content::UtilityProcessMojoClient<mojom::ShellHandler>( On 2016/11/02 00:03:21, Peter Kasting wrote: > Nit: Prefer base::MakeUnique() to raw new: > > client_ = > base::MakeUnique<content::UtilityProcessMojoClient<mojom::ShellHandler>>( > ...); Done.
The CQ bit was checked by pmonette@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2449853008/diff/160001/chrome/browser/resourc... File chrome/browser/resources/welcome/win10/inline.js (right): https://codereview.chromium.org/2449853008/diff/160001/chrome/browser/resourc... chrome/browser/resources/welcome/win10/inline.js:49: if (window.location.href.includes('variant=defaultonly')) Please use URLSearchParams: https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams/URLSearchParams (you can use location.search.slice(1) as the query string)
https://codereview.chromium.org/2449853008/diff/160001/chrome/browser/resourc... File chrome/browser/resources/welcome/win10/inline.js (right): https://codereview.chromium.org/2449853008/diff/160001/chrome/browser/resourc... chrome/browser/resources/welcome/win10/inline.js:49: if (window.location.href.includes('variant=defaultonly')) On 2016/11/04 21:27:03, michaelpg wrote: > Please use URLSearchParams: > https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams/URLSearchParams > > (you can use location.search.slice(1) as the query string) Done.
lgtm, but "if/if" should be "if/else if". https://codereview.chromium.org/2449853008/diff/180001/chrome/browser/resourc... File chrome/browser/resources/welcome/win10/inline.js (right): https://codereview.chromium.org/2449853008/diff/180001/chrome/browser/resourc... chrome/browser/resources/welcome/win10/inline.js:54: if (params.get('variant') === 'combined') nit: else if https://codereview.chromium.org/2449853008/diff/180001/chrome/browser/resourc... File chrome/browser/resources/welcome/win10/sectioned.js (right): https://codereview.chromium.org/2449853008/diff/180001/chrome/browser/resourc... chrome/browser/resources/welcome/win10/sectioned.js:61: if (params.get('variant') === 'combined') (same)
https://codereview.chromium.org/2449853008/diff/180001/chrome/browser/resourc... File chrome/browser/resources/welcome/win10/inline.js (right): https://codereview.chromium.org/2449853008/diff/180001/chrome/browser/resourc... chrome/browser/resources/welcome/win10/inline.js:51: if (params.has('variant')) { also, making this key a constant would be cool. bonus point for making the values an "enum" e.g. https://cs.chromium.org/search/?q=@enum+file:chrome/browser/resources/.*%5C.j...
Thanks PTAL https://codereview.chromium.org/2449853008/diff/180001/chrome/browser/resourc... File chrome/browser/resources/welcome/win10/inline.js (right): https://codereview.chromium.org/2449853008/diff/180001/chrome/browser/resourc... chrome/browser/resources/welcome/win10/inline.js:51: if (params.has('variant')) { On 2016/11/07 03:29:14, michaelpg wrote: > also, making this key a constant would be cool. bonus point for making the > values an "enum" e.g. > https://cs.chromium.org/search/?q=@enum+file:chrome/browser/resources/.*%5C.j... You got me at "bonus point". https://codereview.chromium.org/2449853008/diff/180001/chrome/browser/resourc... chrome/browser/resources/welcome/win10/inline.js:54: if (params.get('variant') === 'combined') On 2016/11/04 23:59:24, michaelpg wrote: > nit: else if Done.
slgtm i mean.. er... A++ and you don't have to take the midterm
Thanks!
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tmartino@chromium.org, pkasting@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2449853008/#ps200001 (title: "More nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Determine the Win10-specific Welcome page variant to display An asynchronous check to determine Chrome's pinned state is done in order to decide if the "Pin Chrome to taskbar" instructions should be displayed. BUG=648686 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Determine the Win10-specific Welcome page variant to display An asynchronous check to determine Chrome's pinned state is done in order to decide if the "Pin Chrome to taskbar" instructions should be displayed. BUG=648686 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b9f756c3b1853a17cd4cfbfcb003f52bd2ae8083 Cr-Commit-Position: refs/heads/master@{#430793} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b9f756c3b1853a17cd4cfbfcb003f52bd2ae8083 Cr-Commit-Position: refs/heads/master@{#430793} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
