Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1575163002:
New experiment without registry deletion (Closed)
|
Created:
4 years, 11 months ago by Patrick Monette Modified:
4 years, 11 months ago CC:
chromium-reviews, jschuh Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNew experiment without registry deletion
Now requires the user to check "Always user this app".
BUG=576490
Committed: https://crrev.com/3d24f096b250b2364f75e6247e8a6d8ac7ec811b
Cr-Commit-Position: refs/heads/master@{#368774}
Patch Set 1 #Patch Set 2 : Add experiment #
Total comments: 6
Patch Set 3 : Comments #Patch Set 4 : Comments #2 #
Total comments: 7
Patch Set 5 : Last comments #
Total comments: 2
Patch Set 6 : Small nit #Messages
Total messages: 26 (12 generated)
pmonette@chromium.org changed reviewers: + gab@chromium.org
Hey Gab, PTAL!
Description was changed from ========== Remove registry deletion BUG=531649 ========== to ========== Remove registry deletion Now requires the user to check "Always user this app". BUG=531649 ==========
Description was changed from ========== Remove registry deletion Now requires the user to check "Always user this app". BUG=531649 ========== to ========== New experiment without registry deletion Now requires the user to check "Always user this app". BUG=531649 ==========
So IIUC all this CL does is introduce a new group which triggers the "Enabled" logic but blocks the registry deletion part of it? https://codereview.chromium.org/1575163002/diff/20001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1575163002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:288: "EnabledNoRegistry"; Use StartsWith() instead of == for comparing group names (more flexible server side after) https://codereview.chromium.org/1575163002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:292: void ResetDefaultBrowser() { What's the reason to move the method? It appears unchanged but makes this diff bigger..?
https://codereview.chromium.org/1575163002/diff/20001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1575163002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:722: bool ShellIntegration::DefaultBrowserWorker::SetAsDefaultBrowserAsynchronous() { It's weird to have a group ("EnablednoRegistry") trigger some things as part of things triggered by a substring of it ("Enabled"). How about instead having an enum which indicates what to do and passing it to this method based on specific group name rather than nesting group name meaning.
Take another look! https://codereview.chromium.org/1575163002/diff/20001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1575163002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:288: "EnabledNoRegistry"; On 2016/01/11 23:41:00, gab wrote: > Use StartsWith() instead of == for comparing group names (more flexible server > side after) Done. https://codereview.chromium.org/1575163002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:292: void ResetDefaultBrowser() { On 2016/01/11 23:41:00, gab wrote: > What's the reason to move the method? It appears unchanged but makes this diff > bigger..? Just to make order of definition = order of use. https://codereview.chromium.org/1575163002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:722: bool ShellIntegration::DefaultBrowserWorker::SetAsDefaultBrowserAsynchronous() { On 2016/01/11 23:43:28, gab wrote: > It's weird to have a group ("EnablednoRegistry") trigger some things as part of > things triggered by a substring of it ("Enabled"). > > How about instead having an enum which indicates what to do and passing it to > this method based on specific group name rather than nesting group name meaning. This won't change the fact that there is 2 meaning behind the group name. We're juggling with 3 possible groups where 2 are very similar. I don't think it will be easy to make only one call to FindFullName() without making the rest of the plumbing convoluted. There is 2 different significations encoded in the group name and the code makes it explicit. I don't fully understand your idea though. I probably missed something :S
pmonette@chromium.org changed reviewers: + asvitkine@chromium.org
Responded to offline comments.
lgtm w/ comments. https://codereview.chromium.org/1575163002/diff/60001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1575163002/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:61: // is "EnabledFull" where the default browser choice is reset. // One of the group names for the AsyncSetAsDefault experiment. Unlike other // "Enabled" groups, this group doesn't reset the current default browser choice // in the registry. (i.e. don't explicitly mention a group name that is server-side only and cut a bit of the rest to make it more succinct, WDYT?) https://codereview.chromium.org/1575163002/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:62: const char kEnabledNoRegistryGroupName[] = "EnabledNoRegistry"; kAsyncSetAsDefaultExperimentEnabledNoRegistryGroupName ? Man this is long but I feel these constants should all have an experiment specific prefix. https://codereview.chromium.org/1575163002/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:62: const char kEnabledNoRegistryGroupName[] = "EnabledNoRegistry"; Also add // A prefix shared by multiple groups that kicks off the generic set-as-default experiment. const char kAsyncSetAsDefaultExperimentEnabledGroupPrefix[] = "Enabled"; (i.e. the advantage of specifying "EnabledNoRegistry" as a constant is only there when all other variants are alongside it) https://codereview.chromium.org/1575163002/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:309: bool ShouldResetDefaultBrowser() { Put this below IsAsyncSetAsDefaultEnabled() to keep related methods together?
Description was changed from ========== New experiment without registry deletion Now requires the user to check "Always user this app". BUG=531649 ========== to ========== New experiment without registry deletion Now requires the user to check "Always user this app". BUG=576490 ==========
https://codereview.chromium.org/1575163002/diff/60001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1575163002/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:61: // is "EnabledFull" where the default browser choice is reset. On 2016/01/12 01:09:04, gab wrote: > // One of the group names for the AsyncSetAsDefault experiment. Unlike other > // "Enabled" groups, this group doesn't reset the current default browser choice > // in the registry. > > (i.e. don't explicitly mention a group name that is server-side only and cut a > bit of the rest to make it more succinct, WDYT?) I like it. https://codereview.chromium.org/1575163002/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:62: const char kEnabledNoRegistryGroupName[] = "EnabledNoRegistry"; On 2016/01/12 01:09:04, gab wrote: > kAsyncSetAsDefaultExperimentEnabledNoRegistryGroupName ? > > Man this is long but I feel these constants should all have an experiment > specific prefix. Done. https://codereview.chromium.org/1575163002/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:309: bool ShouldResetDefaultBrowser() { On 2016/01/12 01:09:04, gab wrote: > Put this below IsAsyncSetAsDefaultEnabled() to keep related methods together? Done.
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/1575163002/#ps80001 (title: "Last comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1575163002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1575163002/80001
Patch set 5 lgtm w/ comment nit.. https://codereview.chromium.org/1575163002/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1575163002/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:59: // A prefix shared by multiple groups that kicks off the generic set-as-default nit: I guess for consistency s/set-as-default/AsyncSetAsDefault/ (sorry, I know I wrote this suggestion, but seeing it in context now brought this to my eye...)
The CQ bit was unchecked by pmonette@chromium.org
https://codereview.chromium.org/1575163002/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1575163002/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:59: // A prefix shared by multiple groups that kicks off the generic set-as-default On 2016/01/12 01:26:46, gab wrote: > nit: I guess for consistency s/set-as-default/AsyncSetAsDefault/ > > (sorry, I know I wrote this suggestion, but seeing it in context now brought > this to my eye...) Done.
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/1575163002/#ps100001 (title: "Small nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1575163002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1575163002/100001
Message was sent while issue was closed.
Description was changed from ========== New experiment without registry deletion Now requires the user to check "Always user this app". BUG=576490 ========== to ========== New experiment without registry deletion Now requires the user to check "Always user this app". BUG=576490 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== New experiment without registry deletion Now requires the user to check "Always user this app". BUG=576490 ========== to ========== New experiment without registry deletion Now requires the user to check "Always user this app". BUG=576490 Committed: https://crrev.com/3d24f096b250b2364f75e6247e8a6d8ac7ec811b Cr-Commit-Position: refs/heads/master@{#368774} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3d24f096b250b2364f75e6247e8a6d8ac7ec811b Cr-Commit-Position: refs/heads/master@{#368774}
Message was sent while issue was closed.
lgtm |
