Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2236663002:
Add origin of settings reset request to feedback reports. (Closed)
|
Created:
4 years, 4 months ago by alito Modified:
4 years, 3 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, michaelpg+watch-md-settings_chromium.org, arv+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd origin of settings reset request to feedback reports.
BUG=636435
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/241030d4936cb71e5acf0d4ef71c2ba95110209f
Cr-Commit-Position: refs/heads/master@{#419807}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed Robert's comments #
Total comments: 4
Patch Set 3 : Addressed Vasilii's comments #
Total comments: 12
Patch Set 4 : Adressed Dan's comments #
Total comments: 23
Patch Set 5 : Addressed Dan's comments, take 2 #
Total comments: 24
Patch Set 6 : Rebased to pick up the refactor in issue 2334003006 and addressed Dan's comments. #Patch Set 7 : Small change in proto enum order. #
Total comments: 6
Patch Set 8 : Addressed Dan's comments, take 3 #
Total comments: 6
Patch Set 9 : nits #
Total comments: 5
Patch Set 10 : Defined hash string #Patch Set 11 : tiny fix. #Patch Set 12 : Move cct hash definition to options::ResetProfileSettingsHandler #Messages
Total messages: 47 (11 generated)
Description was changed from ========== Add origin of settings reset request to feedback reports. BUG=636435 ========== to ========== Add origin of settings reset request to feedback reports. BUG=636435 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
alito@chromium.org changed reviewers: + robertshield@google.com
PTAL. This only works for the current settings page until crbug.com/636408 has been fixed.
alito@chromium.org changed reviewers: + robertshield@chromium.org - robertshield@google.com
Added the wrong reviewer email!
https://codereview.chromium.org/2236663002/diff/1/chrome/browser/profile_rese... File chrome/browser/profile_resetter/profile_reset_report.proto (right): https://codereview.chromium.org/2236663002/diff/1/chrome/browser/profile_rese... chrome/browser/profile_resetter/profile_reset_report.proto:65: // Specified where the request for the settings reset came from. nit: s/Specified/Specifies/ https://codereview.chromium.org/2236663002/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/reset_profile_settings_overlay.js:63: // launching Chrome with startup URL nit: with *the* startup URL https://codereview.chromium.org/2236663002/diff/1/chrome/browser/ui/startup/s... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2236663002/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator.cc:530: static const char kCctHash[] = "CCT"; Do you need a # before the CCT?
Addressed Robert's comments. https://codereview.chromium.org/2236663002/diff/1/chrome/browser/profile_rese... File chrome/browser/profile_resetter/profile_reset_report.proto (right): https://codereview.chromium.org/2236663002/diff/1/chrome/browser/profile_rese... chrome/browser/profile_resetter/profile_reset_report.proto:65: // Specified where the request for the settings reset came from. On 2016/08/10 20:35:41, robertshield wrote: > nit: s/Specified/Specifies/ Done. https://codereview.chromium.org/2236663002/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/reset_profile_settings_overlay.js:63: // launching Chrome with startup URL On 2016/08/10 20:35:41, robertshield wrote: > nit: with *the* startup URL Done. https://codereview.chromium.org/2236663002/diff/1/chrome/browser/ui/startup/s... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2236663002/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator.cc:530: static const char kCctHash[] = "CCT"; On 2016/08/10 20:35:42, robertshield wrote: > Do you need a # before the CCT? Argh. The hash fell off during cleanup. Good catch!
Description was changed from ========== Add origin of settings reset request to feedback reports. BUG=636435 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add origin of settings reset request to feedback reports. BUG=636435 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
alito@chromium.org changed reviewers: + vasilii@chromium.org
Hi Vasilii, battre@ is OOO so could you please TAL? Feel free to suggest other reviewer as appropriate for changes to the profile_reset_report.proto. Thx.
Adding dbeam@ as reviewer. Dan, feel free to suggest other reviewers as appropriate.
https://codereview.chromium.org/2236663002/diff/20001/chrome/browser/profile_... File chrome/browser/profile_resetter/profile_reset_report.proto (right): https://codereview.chromium.org/2236663002/diff/20001/chrome/browser/profile_... chrome/browser/profile_resetter/profile_reset_report.proto:77: RESET_REQUEST_ORIGIN_UNKNOWN = 4; How is it different from RESET_REQUEST_ORIGIN_UNSPECIFIED? https://codereview.chromium.org/2236663002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/reset_profile_settings_handler.cc (right): https://codereview.chromium.org/2236663002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:49: } else if (reset_request_origin == kOriginCct) { There shouldn't be "else" after the "return" statement (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md).
Addressed Vasilii's comments. https://codereview.chromium.org/2236663002/diff/20001/chrome/browser/profile_... File chrome/browser/profile_resetter/profile_reset_report.proto (right): https://codereview.chromium.org/2236663002/diff/20001/chrome/browser/profile_... chrome/browser/profile_resetter/profile_reset_report.proto:77: RESET_REQUEST_ORIGIN_UNKNOWN = 4; On 2016/08/11 16:33:38, vasilii wrote: > How is it different from RESET_REQUEST_ORIGIN_UNSPECIFIED? UNSPECIFIED is used as the default value in case we add new enum fields in the future (which we likely will). This follows Google's internal protobuf "Do's and Don'ts". A value of UNKNOWN is different because it explicitly excludes the other request origins. https://codereview.chromium.org/2236663002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/reset_profile_settings_handler.cc (right): https://codereview.chromium.org/2236663002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:49: } else if (reset_request_origin == kOriginCct) { On 2016/08/11 16:33:38, vasilii wrote: > There shouldn't be "else" after the "return" statement > (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md). Done.
profile_reset_report.proto LGTM
alito@chromium.org changed reviewers: + dbeam@chromium.org
Probably need more coffee... Seems I forgot to actually add dbeam@ as reviewer when I thought I had. Dan, feel free to suggest other reviewers if needed.
Friendly ping...
https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:28: /** @private {string} */ resetRequestOrigin_: '', https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:90: resetRequestOrigin = origin; can you just use a private instance member instead of a local static? i.e. this.resetRequestOrigin_ = origin; https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:534: #endif can you just put this in the #else block below and drop this #if? https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:550: #endif let's just make a bool outside of this #if and set it per-platform, it's getting a little hard to read https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/reset_profile_settings_handler.cc (right): https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:51: if (reset_request_origin == kOriginTriggeredReset) nit: curlies https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:58: << reset_request_origin << "'"; nit: I'd remove the string literal and just leave the variable
Addressed Dan's comments. Thanks for the review. PTAnL. https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:28: On 2016/08/19 02:06:41, Dan Beam wrote: > /** @private {string} */ > resetRequestOrigin_: '', Done. https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:90: resetRequestOrigin = origin; On 2016/08/19 02:06:41, Dan Beam wrote: > can you just use a private instance member instead of a local static? i.e. > > this.resetRequestOrigin_ = origin; Done. https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:534: #endif On 2016/08/19 02:06:41, Dan Beam wrote: > can you just put this in the #else block below and drop this #if? Done. https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:550: #endif On 2016/08/19 02:06:41, Dan Beam wrote: > let's just make a bool outside of this #if and set it per-platform, it's getting > a little hard to read Done. Also made the CCT-specific url windows-only since the CCT is only available on Windows. https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/reset_profile_settings_handler.cc (right): https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:51: if (reset_request_origin == kOriginTriggeredReset) On 2016/08/19 02:06:41, Dan Beam wrote: > nit: curlies Done. https://codereview.chromium.org/2236663002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:58: << reset_request_origin << "'"; On 2016/08/19 02:06:41, Dan Beam wrote: > nit: I'd remove the string literal and just leave the variable Done.
https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:42: ResetProfileSettingsOverlay.getInstance() this.resetRequestOrigin_ https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:44: }; }.bind(this); https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:58: didShowPage: function() { var hash = this.hash.slice(1).toLowerCase(); this.resetRequestOrigin_ = hash == 'cct' || hash == 'userclick' ? hash : ''; this.setHash(''); https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:91: * @param {string} origin Origin of the reset request. can you make this @protected? https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:94: this.resetRequestOrigin_ = origin; why not set this directly? https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:97: getResetRequestOrigin: function() { remove this getter https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:529: bool url_is_allowed_settings_page = false; this name doesn't make much sense to me https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:534: url.spec(), chrome::kChromeUISettingsURL, base::CompareCase::SENSITIVE); why not compare URL.GetOrigin() == GURL(chrome::kChromeUiSettingsURL).GetOrigin()? https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:539: base::StringPrintf("%s%s", chrome::kChromeUISettingsURL, why not just append these char* to reset_settings_url? or just call GURL::Resolve(). https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:545: url.spec() == reset_settings_url + "#CCT"; why is this uppercase?
Addressed comments. Dan, PTAnL. Thanks for being patient with a javascript newbie. https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:42: ResetProfileSettingsOverlay.getInstance() On 2016/08/23 05:48:23, Dan Beam wrote: > this.resetRequestOrigin_ Can't use 'this' here. See explanation in the comment below. https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:44: }; On 2016/08/23 05:48:22, Dan Beam wrote: > }.bind(this); This doesn't work on Windows. On Windows, TriggeredResetProfileSettingsOverlay is a subclass of ResetProfileSettingsOverlay. The InitializePage function is called twice, once for ResetProfileSettingsOverlay and once for TriggeredResetProfileSettingsOverlay (I think... Not a javascript expert...). So we can't rely on 'this' to always point to the ResetProfileSettingsOverlay object. https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:58: didShowPage: function() { On 2016/08/23 05:48:22, Dan Beam wrote: > var hash = this.hash.slice(1).toLowerCase(); > this.resetRequestOrigin_ = hash == 'cct' || hash == 'userclick' ? hash : ''; > this.setHash(''); Done. https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:91: * @param {string} origin Origin of the reset request. On 2016/08/23 05:48:22, Dan Beam wrote: > can you make this @protected? Removed the setter. Now setting resetRequestOrigin_ directly. https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:94: this.resetRequestOrigin_ = origin; On 2016/08/23 05:48:22, Dan Beam wrote: > why not set this directly? I assumed you meant that TriggeredResetProfileSettingsOverlay could set resetRequestOrigin_ directly, so removed the setter. https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:97: getResetRequestOrigin: function() { On 2016/08/23 05:48:23, Dan Beam wrote: > remove this getter Done. https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:529: bool url_is_allowed_settings_page = false; On 2016/08/23 05:48:23, Dan Beam wrote: > this name doesn't make much sense to me Changed to url_points_to_an_approved_settings_page. Is this better? https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:534: url.spec(), chrome::kChromeUISettingsURL, base::CompareCase::SENSITIVE); On 2016/08/23 05:48:23, Dan Beam wrote: > why not compare URL.GetOrigin() == > GURL(chrome::kChromeUiSettingsURL).GetOrigin()? Since I'm not able to test a ChromeOS build and the comparison was not written by me, I would rather leave this unchanged. https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:539: base::StringPrintf("%s%s", chrome::kChromeUISettingsURL, On 2016/08/23 05:48:23, Dan Beam wrote: > why not just append these char* to reset_settings_url? or just call > GURL::Resolve(). Changed to append to reset_settings_url. https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:545: url.spec() == reset_settings_url + "#CCT"; On 2016/08/23 05:48:23, Dan Beam wrote: > why is this uppercase? Changed to lowercase.
https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:44: }; On 2016/08/23 18:20:34, alito wrote: > On 2016/08/23 05:48:22, Dan Beam wrote: > > }.bind(this); > > This doesn't work on Windows. On Windows, TriggeredResetProfileSettingsOverlay > is a subclass of ResetProfileSettingsOverlay. The InitializePage function is > called twice, once for ResetProfileSettingsOverlay and once for > TriggeredResetProfileSettingsOverlay (I think... Not a javascript expert...). So > we can't rely on 'this' to always point to the ResetProfileSettingsOverlay > object. they're the same object with a different prototype chain...
https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:44: }; On 2016/08/23 22:46:53, Dan Beam wrote: > On 2016/08/23 18:20:34, alito wrote: > > On 2016/08/23 05:48:22, Dan Beam wrote: > > > }.bind(this); > > > > This doesn't work on Windows. On Windows, TriggeredResetProfileSettingsOverlay > > is a subclass of ResetProfileSettingsOverlay. The InitializePage function is > > called twice, once for ResetProfileSettingsOverlay and once for > > TriggeredResetProfileSettingsOverlay (I think... Not a javascript expert...). > So > > we can't rely on 'this' to always point to the ResetProfileSettingsOverlay > > object. > > they're the same object with a different prototype chain... Well, my explanation for why .bind(this) doesn't work was really just a guess on my part. I have tested this and using alert()s I've tried to follow the flow. When using .bind(this), after resetRequestOrigin_ has been set to a non-empty string in didShowPage(), the value of this.resetRequestOrigin_ from within this onclick event function call is empty. Any ideas why?
https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:44: }; On 2016/08/24 21:17:01, alito wrote: > On 2016/08/23 22:46:53, Dan Beam wrote: > > On 2016/08/23 18:20:34, alito wrote: > > > On 2016/08/23 05:48:22, Dan Beam wrote: > > > > }.bind(this); > > > > > > This doesn't work on Windows. On Windows, > TriggeredResetProfileSettingsOverlay > > > is a subclass of ResetProfileSettingsOverlay. The InitializePage function is > > > called twice, once for ResetProfileSettingsOverlay and once for > > > TriggeredResetProfileSettingsOverlay (I think... Not a javascript > expert...). > > So > > > we can't rely on 'this' to always point to the ResetProfileSettingsOverlay > > > object. > > > > they're the same object with a different prototype chain... > > Well, my explanation for why .bind(this) doesn't work was really just a guess on > my part. I have tested this and using alert()s I've tried to follow the flow. > When using .bind(this), after resetRequestOrigin_ has been set to a non-empty > string in didShowPage(), the value of this.resetRequestOrigin_ from within this > onclick event function call is empty. Any ideas why? > I've used the debugger and the console. From what I can see, ResetProfileSettingsOverlay and TriggeredResetProfileSettingsOverlay are two distinct objects that both get initialized with the same InitializePage() function. Since TriggeredResetProfileSettingsOverlay happens to be the last one for which InitializePage() is called, the onclick function will have 'this' bound to the TriggeredResetProfileSettingsOverlay object.
Dan, could you PTAnL?
https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:28: /** @private {string} */ this should be protected https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:42: ResetProfileSettingsOverlay.getInstance() if ResetProfileSettingsOverlay.getInstance() != this, we shouldn't be storing this variable in an instance. it should be on a static instead, i.e. ResetProfileSettingsOverlay.resetRequestOrigin_ = https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:43: .resetRequestOrigin_]); this.resetRequestOrigin_ https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:44: }; }.bind(this); https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/triggered_reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/triggered_reset_profile_settings_overlay.js:25: 'reset-profile-settings-overlay'); this constructor should be doing something like ResetProfileSettingsOverlay.call(this); to actually call the super https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/triggered_reset_profile_settings_overlay.js:36: 'triggeredreset'; this.resetRequestOrigin_ = 'triggeredreset'; https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:545: url.spec() == reset_settings_url + "#cct"; I think using GURL's == or .Resolve() (or maybe += does the same) are better than the string concatenation you're doing in some of this code. I don't really care what previous code does (you can leave it if you'd like). Propagating lends legitimacy.
also, can you add some tests to verify the code you're adding?
Dan, I have two questions for you in the comments below. Could you please take a look before I send you a new patch? /ali https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:42: ResetProfileSettingsOverlay.getInstance() On 2016/09/06 19:35:18, Dan Beam wrote: > if ResetProfileSettingsOverlay.getInstance() != this, we shouldn't be storing > this variable in an instance. The reason that ResetProfileSettingsOverlay.getInstance() != this every time is because initializePage() is not overridden by TriggeredResetProfileSettingsOverlay. So initializePage() gets called twice: once for ResetProfileSettingsOverlay and once for TriggeredResetProfileSettingsOverlay. See my suggestion 3.c below. > it should be on a static instead, i.e. ResetProfileSettingsOverlay.resetRequestOrigin_ = So I would create a non-const public static variable? I don't see any uses of non-const static variables in the options code base. Are you ok with that? My JS is weak, but my understanding is that we should avoid these (correct me if I'm wrong). The way I see it, I have five options for how to store and access resetRequestOrigin_: 1) Non-const public static variable The static variable would be initialized in the outermost function with ResetProfileSettingsOverlay.resetRequestOrigin_ = ... and would be accessed directly by both TriggeredResetProfileSettingsOverlay and the onclick function here. 2) Local static variable (ie defined in the same scope as the variable 'Page' at the top of the file) I could then define static setter/getter functions. The setter would be used by TriggeredResetProfileSettingsOverlay to set resetRequestOrigin. The getter would be used by the onclick function here (like ResetProfileSettingsOverlay.getResetRequestOrigin()). 3) a private instance variable 3.a) accessed by calling getInstance() This works because ResetProfileSettingsOverlay is a singleton (so I guess it is not that different from having a static variable?) and this is the approach in this patch set. 3.b) accessed via static getter/setter functions The static getter/setter functions would call getInstance() to access resetRequestOrigin_. An example of this usage: https://cs.chromium.org/chromium/src/chrome/browser/resources/options/clear_b... 3.c) accessed via 'this' To make this work, I would override initializePage() in TriggeredResetProfileSettingsOverlay and make that into essentially a no-op. That way, the onclick handlers would only be set once (instead of twice, which is the case now) and I could use 'this' to access resetRequestOrigin_. Which approach do you recommend? Feel free to suggest a better alternative I haven't considered. https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/triggered_reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/triggered_reset_profile_settings_overlay.js:25: 'reset-profile-settings-overlay'); On 2016/09/06 19:35:18, Dan Beam wrote: > this constructor should be doing something like > > ResetProfileSettingsOverlay.call(this); > > to actually call the super But wouldn't that result in Page's constructor being called with ResetProfileSettingsOverlay's name and title instead of the ones used here? I could instead add parameters to the constructor of ResetProfileSettingsOverlay so it would accept name and title to be passed to Page's constructor. Is that what you had in mind?
this code uses concrete inheritance and is deprecated. it's icky. we can fix some things about the silly inheritance it's doing, or we can use composition. https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:22: cr.addSingletonGetter(ResetProfileSettingsOverlay); remove this addSingletonGetter() call. it's not really needed and confusing you change this code to use new ResetProfileSettingsOverlay() https://cs.chromium.org/chromium/src/chrome/browser/resources/options/options... https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:28: /** @private {string} */ On 2016/09/06 19:35:18, Dan Beam wrote: > this should be protected do this https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:42: ResetProfileSettingsOverlay.getInstance() On 2016/09/13 02:02:00, alito wrote: > On 2016/09/06 19:35:18, Dan Beam wrote: > > if ResetProfileSettingsOverlay.getInstance() != this, we shouldn't be storing > > this variable in an instance. > > The reason that ResetProfileSettingsOverlay.getInstance() != this every time is > because initializePage() is not overridden by > TriggeredResetProfileSettingsOverlay. So initializePage() gets called twice: > once for ResetProfileSettingsOverlay and once for > TriggeredResetProfileSettingsOverlay. See my suggestion 3.c below. > > > it should be on a static instead, i.e. > ResetProfileSettingsOverlay.resetRequestOrigin_ = > > So I would create a non-const public static variable? I don't see any uses of > non-const static variables in the options code base. Are you ok with that? My JS > is weak, but my understanding is that we should avoid these (correct me if I'm > wrong). > > The way I see it, I have five options for how to store and access > resetRequestOrigin_: > > 1) Non-const public static variable > The static variable would be initialized in the outermost function with > ResetProfileSettingsOverlay.resetRequestOrigin_ = ... and would be accessed > directly by both TriggeredResetProfileSettingsOverlay and the onclick function > here. > > 2) Local static variable (ie defined in the same scope as the variable 'Page' at > the top of the file) > I could then define static setter/getter functions. The setter would be used > by TriggeredResetProfileSettingsOverlay to set resetRequestOrigin. The getter > would be used by the onclick function here (like > ResetProfileSettingsOverlay.getResetRequestOrigin()). > > 3) a private instance variable > > 3.a) accessed by calling getInstance() > This works because ResetProfileSettingsOverlay is a singleton (so I guess > it is not that different from having a static variable?) and this is the > approach in this patch set. > > 3.b) accessed via static getter/setter functions > The static getter/setter functions would call getInstance() to access > resetRequestOrigin_. An example of this usage: > https://cs.chromium.org/chromium/src/chrome/browser/resources/options/clear_b... > > 3.c) accessed via 'this' > To make this work, I would override initializePage() in > TriggeredResetProfileSettingsOverlay and make that into essentially a no-op. > That way, the onclick handlers would only be set once (instead of twice, which > is the case now) and I could use 'this' to access resetRequestOrigin_. > > Which approach do you recommend? Feel free to suggest a better alternative I > haven't considered. tl;dr https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:43: .resetRequestOrigin_]); On 2016/09/06 19:35:18, Dan Beam wrote: > this.resetRequestOrigin_ do this https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:44: }; On 2016/09/06 19:35:18, Dan Beam wrote: > }.bind(this); do this https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/triggered_reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/triggered_reset_profile_settings_overlay.js:23: Page.call(this, 'triggeredResetProfileSettings', do options.ResetProfileSettingsOverlay.call(this, ... name parameter ...) what an element @extends and what is .call()'d as a construction should match. this code is bad for not doing that. https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/triggered_reset_profile_settings_overlay.js:25: 'reset-profile-settings-overlay'); On 2016/09/13 02:02:00, alito wrote: > On 2016/09/06 19:35:18, Dan Beam wrote: > > this constructor should be doing something like > > > > ResetProfileSettingsOverlay.call(this); > > > > to actually call the super > > But wouldn't that result in Page's constructor being called with > ResetProfileSettingsOverlay's name and title instead of the ones used here? > > I could instead add parameters to the constructor of ResetProfileSettingsOverlay > so it would accept name and title to be passed to Page's constructor. Is that > what you had in mind? yes, add a name parameter https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/triggered_reset_profile_settings_overlay.js:36: 'triggeredreset'; On 2016/09/06 19:35:18, Dan Beam wrote: > this.resetRequestOrigin_ = 'triggeredreset'; do this
After discussing with Dan offline, I refactored the reset settings overlays here: https://codereview.chromium.org/2334003006/ I rebased to pick up that refactoring and I've addressed Dan's comments. Dan, could you PTAL? https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:22: cr.addSingletonGetter(ResetProfileSettingsOverlay); On 2016/09/13 18:42:09, Dan Beam wrote: > remove this addSingletonGetter() call. it's not really needed and confusing you > > change this code to use new ResetProfileSettingsOverlay() > > https://cs.chromium.org/chromium/src/chrome/browser/resources/options/options... Done. https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:28: /** @private {string} */ On 2016/09/13 18:42:09, Dan Beam wrote: > On 2016/09/06 19:35:18, Dan Beam wrote: > > this should be protected > > do this resetRequestOrigin_ is now a static variable. https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:43: .resetRequestOrigin_]); On 2016/09/13 18:42:09, Dan Beam wrote: > On 2016/09/06 19:35:18, Dan Beam wrote: > > this.resetRequestOrigin_ > > do this resetRequestOrigin_ is now a static variable. https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:44: }; On 2016/09/13 18:42:09, Dan Beam wrote: > On 2016/09/06 19:35:18, Dan Beam wrote: > > }.bind(this); > > do this No longer needed. https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/triggered_reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/triggered_reset_profile_settings_overlay.js:25: 'reset-profile-settings-overlay'); On 2016/09/13 18:42:10, Dan Beam wrote: > On 2016/09/13 02:02:00, alito wrote: > > On 2016/09/06 19:35:18, Dan Beam wrote: > > > this constructor should be doing something like > > > > > > ResetProfileSettingsOverlay.call(this); > > > > > > to actually call the super > > > > But wouldn't that result in Page's constructor being called with > > ResetProfileSettingsOverlay's name and title instead of the ones used here? > > > > I could instead add parameters to the constructor of > ResetProfileSettingsOverlay > > so it would accept name and title to be passed to Page's constructor. Is that > > what you had in mind? > > yes, add a name parameter This was parameterized in a different CL: https://codereview.chromium.org/2334003006/ https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/triggered_reset_profile_settings_overlay.js:36: 'triggeredreset'; On 2016/09/13 18:42:10, Dan Beam wrote: > On 2016/09/06 19:35:18, Dan Beam wrote: > > this.resetRequestOrigin_ = 'triggeredreset'; > > do this This is now taken care of by ResetProfileSettingsOverlay. https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2236663002/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:545: url.spec() == reset_settings_url + "#cct"; On 2016/09/06 19:35:18, Dan Beam wrote: > I think using GURL's == or .Resolve() (or maybe += does the same) are better > than the string concatenation you're doing in some of this code. I don't really > care what previous code does (you can leave it if you'd like). Propagating > lends legitimacy. Changed to use GURL for comparisons instead. Please take a careful look :-).
https://codereview.chromium.org/2236663002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/120001/chrome/browser/resourc... chrome/browser/resources/options/reset_profile_settings_overlay.js:49: if (!this.isTriggered_) { soooooo, it seems like it doesn't matter whether the dialog isTriggered_ or not whole adding these listeners. we just don't want to add them twice. because we still use this code for the triggered case. can we just do this instead? /** @private {boolean} */ ResetProfileSettingOverlay.listenersAdded_ = false; if (!ResetProfileSettingOverlay.listenersAdded_) { ... ResetProfileSettingOverlay.listenersAdded_ = true; } https://codereview.chromium.org/2236663002/diff/120001/chrome/browser/resourc... chrome/browser/resources/options/reset_profile_settings_overlay.js:91: var hash = this.hash.slice(1).toLowerCase(); do we need this hash variable if this.isTriggered_? https://codereview.chromium.org/2236663002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/reset_profile_settings_handler.cc (right): https://codereview.chromium.org/2236663002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:59: return reset_report::ChromeResetReport::RESET_REQUEST_ORIGIN_UNKNOWN; if (!reset_requeat_origin.empty()) NOTREACHED(); return RESET_REQUEST_ORIGIN_UNKNOWN;
Thanks for you comments Dan. Should all be addressed now. https://codereview.chromium.org/2236663002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/120001/chrome/browser/resourc... chrome/browser/resources/options/reset_profile_settings_overlay.js:49: if (!this.isTriggered_) { On 2016/09/16 06:29:41, Dan Beam wrote: > soooooo, it seems like it doesn't matter whether the dialog isTriggered_ or not > whole adding these listeners. we just don't want to add them twice. because we > still use this code for the triggered case. > > can we just do this instead? > > /** @private {boolean} */ > ResetProfileSettingOverlay.listenersAdded_ = false; > > if (!ResetProfileSettingOverlay.listenersAdded_) { > ... > ResetProfileSettingOverlay.listenersAdded_ = true; > } Done. https://codereview.chromium.org/2236663002/diff/120001/chrome/browser/resourc... chrome/browser/resources/options/reset_profile_settings_overlay.js:91: var hash = this.hash.slice(1).toLowerCase(); On 2016/09/16 06:29:41, Dan Beam wrote: > do we need this hash variable if this.isTriggered_? Made the triggered case simpler. https://codereview.chromium.org/2236663002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/reset_profile_settings_handler.cc (right): https://codereview.chromium.org/2236663002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:59: return reset_report::ChromeResetReport::RESET_REQUEST_ORIGIN_UNKNOWN; On 2016/09/16 06:29:41, Dan Beam wrote: > if (!reset_requeat_origin.empty()) > NOTREACHED(); > > return RESET_REQUEST_ORIGIN_UNKNOWN; Done.
lgtm thanks for bearing with me, sorry i misinterpreted this code at first (it's a little funky) https://codereview.chromium.org/2236663002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/140001/chrome/browser/resourc... chrome/browser/resources/options/reset_profile_settings_overlay.js:55: ResetProfileSettingsOverlay.resetRequestOrigin_]); nit: if you expect this to always be non-empty, wrap it in assert() chrome.send('performResetProfileSettings', [$('send-settings').checked, assert(ResetProfileSettingsOverlay.resetRequestOrigin_)]); https://codereview.chromium.org/2236663002/diff/140001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2236663002/diff/140001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:33: #include "base/strings/stringprintf.h" not needed any more https://codereview.chromium.org/2236663002/diff/140001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:531: url.GetOrigin() == settings_url.GetOrigin(); this also looks much better! yay!
alito@chromium.org changed reviewers: + sky@chromium.org
Adding sky for OWNER's approval for chrome/browser/ui/startup/startup_browser_creator.cc. Robert, any thoughts? https://codereview.chromium.org/2236663002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2236663002/diff/140001/chrome/browser/resourc... chrome/browser/resources/options/reset_profile_settings_overlay.js:55: ResetProfileSettingsOverlay.resetRequestOrigin_]); On 2016/09/16 19:48:29, Dan Beam wrote: > nit: if you expect this to always be non-empty, wrap it in assert() > > chrome.send('performResetProfileSettings', > [$('send-settings').checked, > assert(ResetProfileSettingsOverlay.resetRequestOrigin_)]); An empty string is ok to be passed here. It can happen, for example, if user navigates to chrome://settings/resetProfileSettings from the omnibox. https://codereview.chromium.org/2236663002/diff/140001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2236663002/diff/140001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:33: #include "base/strings/stringprintf.h" On 2016/09/16 19:48:29, Dan Beam wrote: > not needed any more Done. https://codereview.chromium.org/2236663002/diff/140001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:531: url.GetOrigin() == settings_url.GetOrigin(); On 2016/09/16 19:48:29, Dan Beam wrote: > this also looks much better! yay! Always a good feeling.
https://codereview.chromium.org/2236663002/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2236663002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:528: // line. See ExistingUserController::OnLoginSuccess. There is no ExistingUserController::OnLoginSuccess https://codereview.chromium.org/2236663002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:541: url == reset_settings_url.Resolve("#cct"); Would it make sense to define this hash some where? It's rather cryptic and ideally could be shared.
Thanks Scott. I've addressed your comments. Dan, I moved the CCT hash string to chrome/browser/ui/webui/settings_utils.h (rationale given in the comment below). /ali https://codereview.chromium.org/2236663002/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2236663002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:528: // line. See ExistingUserController::OnLoginSuccess. On 2016/09/16 21:53:05, sky wrote: > There is no ExistingUserController::OnLoginSuccess Well, it seems this comment was introduced in 2010 here: https://codereview.chromium.org/4980005/ where the code would "direct new accounts to chrome://settings/personal". The code has moved around significantly since then and I have no idea if there is any specific code or comment to point to. I've removed the reference to ExistingUserController::OnLoginSuccess. https://codereview.chromium.org/2236663002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:541: url == reset_settings_url.Resolve("#cct"); On 2016/09/16 21:53:05, sky wrote: > Would it make sense to define this hash some where? It's rather cryptic and > ideally could be shared. It was a bit hard to find it a nice home. I've defined it in chrome/browser/ui/webui/settings_utils.h since that seems to be shared by both the old options code as well as the new md-settings code (I'm going to need this hash also in md-settings in the next few CLs). Dan, Scott, if you have suggestions for a better home for the CCT hash string, please let me know.
https://codereview.chromium.org/2236663002/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/2236663002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:541: url == reset_settings_url.Resolve("#cct"); On 2016/09/17 20:23:31, alito wrote: > On 2016/09/16 21:53:05, sky wrote: > > Would it make sense to define this hash some where? It's rather cryptic and > > ideally could be shared. > > It was a bit hard to find it a nice home. I've defined it in > chrome/browser/ui/webui/settings_utils.h since that seems to be shared by both > the old options code as well as the new md-settings code (I'm going to need this > hash also in md-settings in the next few CLs). > > Dan, Scott, if you have suggestions for a better home for the CCT hash string, > please let me know. I wouldn't expect the has in a util file. Maybe Dan can suggest a better home.
Scott, I moved the definition of the hash to the resetter class in options. Based on the current code, I think that is as good a home as any. If I ever need to use it elsewhere, I can break it out of there then. PTAnL.
LGTM
The CQ bit was checked by alito@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2236663002/#ps210001 (title: "Move cct hash definition to options::ResetProfileSettingsHandler")
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.
Description was changed from ========== Add origin of settings reset request to feedback reports. BUG=636435 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add origin of settings reset request to feedback reports. BUG=636435 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #12 (id:210001)
Message was sent while issue was closed.
Description was changed from ========== Add origin of settings reset request to feedback reports. BUG=636435 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add origin of settings reset request to feedback reports. BUG=636435 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/241030d4936cb71e5acf0d4ef71c2ba95110209f Cr-Commit-Position: refs/heads/master@{#419807} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/241030d4936cb71e5acf0d4ef71c2ba95110209f Cr-Commit-Position: refs/heads/master@{#419807} |
