Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10453041:
Support for interactive set-chrome-as-default in Windows. (Closed)
|
Created:
8 years, 7 months ago by motek. Modified:
8 years, 6 months ago CC:
chromium-reviews, grt+watch_chromium.org, chrome-win8-eng_google.com Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThe CL adds to ShellUtil and ShellIntegration and adjusts invocations of the latter whenever necessary.
R=grt@chromium.org, sky@chromium.org
BUG=113326, 129232
TEST=N/A
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141172
Patch Set 1 #
Total comments: 37
Patch Set 2 : Cleaning up some random git mess + addressing reviewer's remarks. #Patch Set 3 : A style fix #
Total comments: 37
Patch Set 4 : Addressed reviewers' remarks and took care of a missed use case. #Patch Set 5 : Fixed for unittests + a typo or two. #
Total comments: 24
Patch Set 6 : Addressed reviewers' remarks. #
Total comments: 12
Patch Set 7 : Addressed reviewers' remarks. #Patch Set 8 : A minor style-oops. #
Total comments: 31
Patch Set 9 : Addressed reviewers' remarks. #Patch Set 10 : Updated a comment. #
Total comments: 4
Patch Set 11 : Updated a comment. #
Total comments: 3
Patch Set 12 : Addressed the reviewer's remark. #Messages
Total messages: 28 (0 generated)
partial review comments as per our discussion. http://codereview.chromium.org/10453041/diff/1/chrome/browser/custom_handlers... File chrome/browser/custom_handlers/protocol_handler_registry.cc (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/custom_handlers... chrome/browser/custom_handlers/protocol_handler_registry.cc:42: return ShellIntegration::CHANGE_DEFAULT_NOT_ALLOWED != although i personally like comparisons like this (SOME_CONSTANT != something_else), chromium doesn't. please flipe these around. http://codereview.chromium.org/10453041/diff/1/chrome/browser/custom_handlers... chrome/browser/custom_handlers/protocol_handler_registry.cc:43: ShellIntegration::CanSetAsDefaultProtocolClient(); nit: indent two more spaces http://codereview.chromium.org/10453041/diff/1/chrome/browser/first_run/first... File chrome/browser/first_run/first_run.cc (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/first_run/first... chrome/browser/first_run/first_run.cc:379: ShellIntegration::CHANGE_DEFAULT_UNATTENDED == flip these http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... File chrome/browser/shell_integration.h (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration.h:28: // initialized or if it is not supported (introduced for Windows8). Windows8 -> Windows 8 http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration.h:37: // forcing user into UI interaction when this should not be done. user -> the user http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration.h:44: // Returns true if the running browser can be set as the default browser. update comment. suggest something like: "Returns requirements for making the running browser the user's default." or something like that. http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration.h:47: // Returns true if the running browser can be set as the default client here, too http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... File chrome/browser/shell_integration_mac.mm (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration_mac.mm:47: if (CHANGE_DEFAULT_NOT_ALLOWED == CanSetAsDefaultProtocolClient()) why is line 27 != _UNATTENDED and this one == NOT_ALLOWED? (also, flip the expression) http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... File chrome/browser/shell_integration_win.cc (left): http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration_win.cc:449: nit: please add this newline back. it was my favorite. http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration_win.cc:394: if (!BrowserDistribution::GetDistribution()->CanSetAsDefault()) { no braces for these one-liners http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration_win.cc:398: if (base::win::GetVersion() >= base::win::VERSION_WIN8) { no braces for these http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration_win.cc:450: LOG(ERROR) << "Error getting app exe path"; i think NOTREACHED(); or LOG(DFATAL) << ...; would be good here, since this should never ever ever ever fail. http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/cocoa/first_... File chrome/browser/ui/cocoa/first_run_dialog.mm (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/cocoa/first_... chrome/browser/ui/cocoa/first_run_dialog.mm:162: makeDefaultBrowser_ = ShellIntegration::CHANGE_DEFAULT_NOT_ALLOWED != flip http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/startup/defa... File chrome/browser/ui/startup/default_browser_prompt.cc (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/startup/defa... chrome/browser/ui/startup/default_browser_prompt.cc:37: explicit DefaultBrowserInfoBarDelegate(InfoBarTabHelper* infobar_helper, remove "explicit" since there's more than one arg. http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/startup/defa... chrome/browser/ui/startup/default_browser_prompt.cc:55: static void SetChromeAsDefaultBrowser(bool interactive_flow); please separate this from the ConfirmInfoBarDelegate methods http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/startup/defa... chrome/browser/ui/startup/default_browser_prompt.cc:139: if (!ShellIntegration::StartSetAsDefaultBrowserInteractive()) { remove braces http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/startup/defa... chrome/browser/ui/startup/default_browser_prompt.cc:186: if (ShellIntegration::CHANGE_DEFAULT_NOT_ALLOWED == default_change_mode) i think this looks better without the return: if (default_change_mode != NOT_ALLOWED) PostTask... http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/webui/option... File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/webui/option... chrome/browser/ui/webui/options2/browser_options_handler2.cc:682: if (ShellIntegration::CHANGE_DEFAULT_NOT_ALLOWED == flip http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/webui/option... chrome/browser/ui/webui/options2/browser_options_handler2.cc:683: ShellIntegration::CanSetAsDefaultBrowser()) { indent four more spaces
Cleaned up git-related mess-up and addressed the first batch of remarks from grt@. PTAL. http://codereview.chromium.org/10453041/diff/1/chrome/browser/custom_handlers... File chrome/browser/custom_handlers/protocol_handler_registry.cc (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/custom_handlers... chrome/browser/custom_handlers/protocol_handler_registry.cc:42: return ShellIntegration::CHANGE_DEFAULT_NOT_ALLOWED != Yeah. I remember I had such a hard time adjusting to this rule first time around. And here we go again, over a year later. I'm sorry you are exposed to that. http://codereview.chromium.org/10453041/diff/1/chrome/browser/custom_handlers... chrome/browser/custom_handlers/protocol_handler_registry.cc:43: ShellIntegration::CanSetAsDefaultProtocolClient(); On 2012/05/25 20:27:38, grt wrote: > nit: indent two more spaces Done. http://codereview.chromium.org/10453041/diff/1/chrome/browser/first_run/first... File chrome/browser/first_run/first_run.cc (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/first_run/first... chrome/browser/first_run/first_run.cc:379: ShellIntegration::CHANGE_DEFAULT_UNATTENDED == On 2012/05/25 20:27:38, grt wrote: > flip these Done. http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... File chrome/browser/shell_integration.h (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration.h:28: // initialized or if it is not supported (introduced for Windows8). On 2012/05/25 20:27:38, grt wrote: > Windows8 -> Windows 8 Done. http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration.h:37: // forcing user into UI interaction when this should not be done. On 2012/05/25 20:27:38, grt wrote: > user -> the user Done. http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration.h:44: // Returns true if the running browser can be set as the default browser. On 2012/05/25 20:27:38, grt wrote: > update comment. suggest something like: "Returns requirements for making the > running browser the user's default." or something like that. Done. http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration.h:47: // Returns true if the running browser can be set as the default client On 2012/05/25 20:27:38, grt wrote: > here, too Done. http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... File chrome/browser/shell_integration_mac.mm (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration_mac.mm:47: if (CHANGE_DEFAULT_NOT_ALLOWED == CanSetAsDefaultProtocolClient()) On 2012/05/25 20:27:38, grt wrote: > why is line 27 != _UNATTENDED and this one == NOT_ALLOWED? (also, flip the > expression) It seemed natural to use a less constraining check. But it may be indeed confusing AND it shouldn't really matter, so I am changing it along your suggestion to reduce the surprise. http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... File chrome/browser/shell_integration_win.cc (left): http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration_win.cc:449: On 2012/05/25 20:27:38, grt wrote: > nit: please add this newline back. it was my favorite. Done. http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration_win.cc:394: if (!BrowserDistribution::GetDistribution()->CanSetAsDefault()) { On 2012/05/25 20:27:38, grt wrote: > no braces for these one-liners Done. http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration_win.cc:398: if (base::win::GetVersion() >= base::win::VERSION_WIN8) { On 2012/05/25 20:27:38, grt wrote: > no braces for these Done. http://codereview.chromium.org/10453041/diff/1/chrome/browser/shell_integrati... chrome/browser/shell_integration_win.cc:450: LOG(ERROR) << "Error getting app exe path"; On 2012/05/25 20:27:38, grt wrote: > i think NOTREACHED(); or LOG(DFATAL) << ...; would be good here, since this > should never ever ever ever fail. Fair enough. I just copied&pasted this without giving it much thought. Done. http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/cocoa/first_... File chrome/browser/ui/cocoa/first_run_dialog.mm (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/cocoa/first_... chrome/browser/ui/cocoa/first_run_dialog.mm:162: makeDefaultBrowser_ = ShellIntegration::CHANGE_DEFAULT_NOT_ALLOWED != On 2012/05/25 20:27:38, grt wrote: > flip Done. http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/startup/defa... File chrome/browser/ui/startup/default_browser_prompt.cc (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/startup/defa... chrome/browser/ui/startup/default_browser_prompt.cc:37: explicit DefaultBrowserInfoBarDelegate(InfoBarTabHelper* infobar_helper, On 2012/05/25 20:27:38, grt wrote: > remove "explicit" since there's more than one arg. Good point. Haven't noticed that. http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/startup/defa... chrome/browser/ui/startup/default_browser_prompt.cc:55: static void SetChromeAsDefaultBrowser(bool interactive_flow); On 2012/05/25 20:27:38, grt wrote: > please separate this from the ConfirmInfoBarDelegate methods Done. http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/startup/defa... chrome/browser/ui/startup/default_browser_prompt.cc:186: if (ShellIntegration::CHANGE_DEFAULT_NOT_ALLOWED == default_change_mode) On 2012/05/25 20:27:38, grt wrote: > i think this looks better without the return: > if (default_change_mode != NOT_ALLOWED) > PostTask... Done. http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/webui/option... File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right): http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/webui/option... chrome/browser/ui/webui/options2/browser_options_handler2.cc:682: if (ShellIntegration::CHANGE_DEFAULT_NOT_ALLOWED == On 2012/05/25 20:27:38, grt wrote: > flip Done. http://codereview.chromium.org/10453041/diff/1/chrome/browser/ui/webui/option... chrome/browser/ui/webui/options2/browser_options_handler2.cc:683: ShellIntegration::CanSetAsDefaultBrowser()) { On 2012/05/25 20:27:38, grt wrote: > indent four more spaces Done.
Round two! http://codereview.chromium.org/10453041/diff/11014/chrome/browser/shell_integ... File chrome/browser/shell_integration.h (right): http://codereview.chromium.org/10453041/diff/11014/chrome/browser/shell_integ... chrome/browser/shell_integration.h:29: static bool StartSetAsDefaultBrowserInteractive(); The name "StartBlaBlah" makes me think that this might kick off an async operation. is this the case, or is it a blocking operation? please document one way or the other. Should the comment really begin with "Initiates" rather than "Initializes"? If the function returns true, does the caller need to know anything about how the OS shell flow terminates? How about when the function returns false? Is that only due to an error? Must this only be called from a certain thread? http://codereview.chromium.org/10453041/diff/11014/chrome/browser/shell_integ... File chrome/browser/shell_integration_linux.cc (right): http://codereview.chromium.org/10453041/diff/11014/chrome/browser/shell_integ... chrome/browser/shell_integration_linux.cc:362: return CHANGE_DEFAULT_NOT_ALLOWED; shouldn't this be CHANGE_DEFAULT_UNATTENDED? http://codereview.chromium.org/10453041/diff/11014/chrome/browser/shell_integ... File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/10453041/diff/11014/chrome/browser/shell_integ... chrome/browser/shell_integration_win.cc:446: FilePath chrome_exe; should this enforce that it's being called on a particular thread? http://codereview.chromium.org/10453041/diff/11014/chrome/browser/ui/startup/... File chrome/browser/ui/startup/default_browser_prompt.cc (right): http://codereview.chromium.org/10453041/diff/11014/chrome/browser/ui/startup/... chrome/browser/ui/startup/default_browser_prompt.cc:35: // This requires IO access (registry) and may reasult in interaction with a reasult -> result http://codereview.chromium.org/10453041/diff/11014/chrome/browser/ui/startup/... chrome/browser/ui/startup/default_browser_prompt.cc:39: UMA_HISTOGRAM_COUNTS("DefaultBrowserWarning.SetAsDefaultUI", 1); do you have a corresponding change to histograms.xml for SetAsDefaultUI and SetAsDefaultUIFailed? http://codereview.chromium.org/10453041/diff/11014/chrome/browser/ui/webui/op... File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right): http://codereview.chromium.org/10453041/diff/11014/chrome/browser/ui/webui/op... chrome/browser/ui/webui/options2/browser_options_handler2.cc:715: UserMetricsAction("Options_ShowSetAsDefaultBrowser")); i don't know how these UserMetrics stuff work. do these new action names need to be registered somewhere else? http://codereview.chromium.org/10453041/diff/11014/chrome/browser/ui/webui/op... chrome/browser/ui/webui/options2/browser_options_handler2.cc:718: UserMetricsAction("Options_SetAsDefaultBrowserCancelled")); the comment for StartSetAsDefaultBrowserInteractive doesn't indicate that a return value of false means that the user cancelled it. is this metrics action correct, or should the comment be updated? http://codereview.chromium.org/10453041/diff/11014/chrome/browser/ui/webui/op... chrome/browser/ui/webui/options2/browser_options_handler2.cc:721: UpdateDefaultBrowserState(); hmm, this makes me think that StartSetAsDefaultBrowserInteractive is a blocking call (otherwise i wouldn't expect that it makes sense to update the state here). http://codereview.chromium.org/10453041/diff/11014/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10453041/diff/11014/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:617: bool LaunchApplicationAssociationDialog() { please update the comment and function name since this now brings up the "How do you want to open web pages dialog" rather than the advanced association UI. http://codereview.chromium.org/10453041/diff/11014/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:618: const wchar_t* protocol = L"http"; static const wchar_t protocol[] = L"http"; to be sure it's all in .rdata http://codereview.chromium.org/10453041/diff/11014/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:624: return SUCCEEDED(hr); how about adding this just before the return: DLOG_IF(WARNING, FAILED(hr)) << "Failed to set as default http handler; hr=0x" << std::hex << hr; http://codereview.chromium.org/10453041/diff/11014/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:953: return false; should there be a NOTREACHED() here? http://codereview.chromium.org/10453041/diff/11014/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1028: BrowserDistribution* dist, int shell_change, const string16& chrome_exe, put each parameters on its own line, indented four spaces http://codereview.chromium.org/10453041/diff/11014/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1039: // control panel. This action does not require elevation and we i suggest replacing the last few lines of this comment with: // control panel, which is a mess, or pop the concise "How you want to open // webpages"? dialog. We choose the latter. http://codereview.chromium.org/10453041/diff/11014/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10453041/diff/11014/chrome/installer/util/shel... chrome/installer/util/shell_util.h:240: static bool ShowMakeChromeDefaultSystemUI(BrowserDistribution* dist, please document (including whether or not this is a blocking call).
Drive-by comments for shell_util changes. http://codereview.chromium.org/10453041/diff/11014/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10453041/diff/11014/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:617: bool LaunchApplicationAssociationDialog() { On 2012/05/28 20:47:02, grt wrote: > please update the comment and function name since this now brings up the "How do > you want to open web pages dialog" rather than the advanced association UI. Also, can you leave the old call around? It will need to be invoked if we are to have a button in settings to pop this "all defaults" dialog. http://codereview.chromium.org/10453041/diff/11014/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1032: This is only for Win8+ right? If so, can you add a DCHECK here to enforce the contract? (and add it in the comment in the header) http://codereview.chromium.org/10453041/diff/11014/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1033: ShellUtil::RegisterChromeBrowser(dist, chrome_exe, L"", elevate_if_not_admin); Note to self (and reminder to my future reviewers!): once my UAC patch goes in (and provided this function is only for Win8+), elevate_if_not_admin can always be false and there is thus no need to have this parameter propagated through the call stack leading here. For now (as this will go in before my UAC patch) it makes sense to leave it here (so that we do get UAC on first run to be able to get this flow: as otherwise chrome's capabilities are not registered in the current registration flow...)
https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/browser/she... File chrome/browser/shell_integration.h (right): https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/browser/she... chrome/browser/shell_integration.h:29: static bool StartSetAsDefaultBrowserInteractive(); On 2012/05/28 20:47:02, grt wrote: > The name "StartBlaBlah" makes me think that this might kick off an async > operation. is this the case, or is it a blocking operation? please document > one way or the other. > > Should the comment really begin with "Initiates" rather than "Initializes"? If > the function returns true, does the caller need to know anything about how the > OS shell flow terminates? > > How about when the function returns false? Is that only due to an error? > > Must this only be called from a certain thread? Done. https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/browser/she... File chrome/browser/shell_integration_linux.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/browser/she... chrome/browser/shell_integration_linux.cc:362: return CHANGE_DEFAULT_NOT_ALLOWED; On 2012/05/28 20:47:02, grt wrote: > shouldn't this be CHANGE_DEFAULT_UNATTENDED? Oops. It should. Thanks. https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/browser/she... File chrome/browser/shell_integration_win.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/browser/she... chrome/browser/shell_integration_win.cc:446: FilePath chrome_exe; On 2012/05/28 20:47:02, grt wrote: > should this enforce that it's being called on a particular thread? I believed this has the same requirements as SetAs* functions above. So I thought this was pretty much in line with what's here. Do you think it would be helpful if I emphasized it here? https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/browser/ui/... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/browser/ui/... chrome/browser/ui/startup/default_browser_prompt.cc:35: // This requires IO access (registry) and may reasult in interaction with a On 2012/05/28 20:47:02, grt wrote: > reasult -> result Done. https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/browser/ui/... chrome/browser/ui/startup/default_browser_prompt.cc:39: UMA_HISTOGRAM_COUNTS("DefaultBrowserWarning.SetAsDefaultUI", 1); On 2012/05/28 20:47:02, grt wrote: > do you have a corresponding change to histograms.xml for SetAsDefaultUI and > SetAsDefaultUIFailed? Right. A good point. But the file is not here, git doesn't like it. Will I need a separate CL or something? https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/browser/ui/... File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/browser/ui/... chrome/browser/ui/webui/options2/browser_options_handler2.cc:715: UserMetricsAction("Options_ShowSetAsDefaultBrowser")); On 2012/05/28 20:47:02, grt wrote: > i don't know how these UserMetrics stuff work. do these new action names need > to be registered somewhere else? Ack. This is gone. Do you think I should somehow specifically gather metrics for that? https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/browser/ui/... chrome/browser/ui/webui/options2/browser_options_handler2.cc:718: UserMetricsAction("Options_SetAsDefaultBrowserCancelled")); On 2012/05/28 20:47:02, grt wrote: > the comment for StartSetAsDefaultBrowserInteractive doesn't indicate that a > return value of false means that the user cancelled it. is this metrics action > correct, or should the comment be updated? The comment in the other place was indeed incorrect and has been update. Still, I am not sure if we would even care about these stats... https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/browser/ui/... chrome/browser/ui/webui/options2/browser_options_handler2.cc:721: UpdateDefaultBrowserState(); On 2012/05/28 20:47:02, grt wrote: > hmm, this makes me think that StartSetAsDefaultBrowserInteractive is a blocking > call (otherwise i wouldn't expect that it makes sense to update the state here). You are correct. But there was another problem here and this code is now all gone, rolled into ShellIntegration. https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/installer/u... chrome/installer/util/shell_util.cc:617: bool LaunchApplicationAssociationDialog() { On 2012/05/28 20:47:02, grt wrote: > please update the comment and function name since this now brings up the "How do > you want to open web pages dialog" rather than the advanced association UI. Done. https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/installer/u... chrome/installer/util/shell_util.cc:617: bool LaunchApplicationAssociationDialog() { On 2012/05/29 16:05:19, gab wrote: > On 2012/05/28 20:47:02, grt wrote: > > please update the comment and function name since this now brings up the "How > do > > you want to open web pages dialog" rather than the advanced association UI. > > Also, can you leave the old call around? It will need to be invoked if we are to > have a button in settings to pop this "all defaults" dialog. Done. https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/installer/u... chrome/installer/util/shell_util.cc:618: const wchar_t* protocol = L"http"; On 2012/05/28 20:47:02, grt wrote: > static const wchar_t protocol[] = L"http"; > to be sure it's all in .rdata Done. https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/installer/u... chrome/installer/util/shell_util.cc:624: return SUCCEEDED(hr); On 2012/05/28 20:47:02, grt wrote: > how about adding this just before the return: > DLOG_IF(WARNING, FAILED(hr)) << "Failed to set as default http handler; hr=0x" > << std::hex << hr; Done. https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/installer/u... chrome/installer/util/shell_util.cc:953: return false; On 2012/05/28 20:47:02, grt wrote: > should there be a NOTREACHED() here? Done. https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/installer/u... chrome/installer/util/shell_util.cc:1028: BrowserDistribution* dist, int shell_change, const string16& chrome_exe, On 2012/05/28 20:47:02, grt wrote: > put each parameters on its own line, indented four spaces Done. https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/installer/u... chrome/installer/util/shell_util.cc:1032: On 2012/05/29 16:05:19, gab wrote: > This is only for Win8+ right? If so, can you add a DCHECK here to enforce the > contract? (and add it in the comment in the header) I don't know. It is not only Windows 8. This dialog functions in earlier versions and this is how I debug it for the most part, too. There is no good reason to use it, so I have done what you suggest, but I have my doubts... https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/installer/u... chrome/installer/util/shell_util.cc:1033: ShellUtil::RegisterChromeBrowser(dist, chrome_exe, L"", elevate_if_not_admin); On 2012/05/29 16:05:19, gab wrote: > Note to self (and reminder to my future reviewers!): once my UAC patch goes in > (and provided this function is only for Win8+), elevate_if_not_admin can always > be false and there is thus no need to have this parameter propagated through the > call stack leading here. > > For now (as this will go in before my UAC patch) it makes sense to leave it here > (so that we do get UAC on first run to be able to get this flow: as otherwise > chrome's capabilities are not registered in the current registration flow...) Well, OK. https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/installer/u... chrome/installer/util/shell_util.cc:1039: // control panel. This action does not require elevation and we On 2012/05/28 20:47:02, grt wrote: > i suggest replacing the last few lines of this comment with: > // control panel, which is a mess, or pop the concise "How you want to open > // webpages"? dialog. We choose the latter. Done. https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/installer/u... chrome/installer/util/shell_util.h:240: static bool ShowMakeChromeDefaultSystemUI(BrowserDistribution* dist, On 2012/05/28 20:47:02, grt wrote: > please document (including whether or not this is a blocking call). Done.
lg, see below. https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/11014/chrome/installer/u... chrome/installer/util/shell_util.cc:1032: On 2012/05/31 21:59:04, motek. wrote: > On 2012/05/29 16:05:19, gab wrote: > > This is only for Win8+ right? If so, can you add a DCHECK here to enforce the > > contract? (and add it in the comment in the header) > I don't know. It is not only Windows 8. This dialog functions in earlier > versions and this is how I debug it for the most part, too. There is no good > reason to use it, so I have done what you suggest, but I have my doubts... Thanks, my point is that it will never be used on Win7 and forcing Win8+ will allow me to remove unnecessary extra parameters (i.e. elevate_if_not_admin) once my HKCU only patch goes in. I wouldn't like to have to keep parameters around just for the sake of being compatible on an OS we will never use it on. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.h:242: // on Windows 7, too. This is a blocking call. Please change "This is intended for Windows 8, but is known to work on Windows 7, too." to "This is intended for Windows 8 and above only." So that the contract in the comment matches the implementation.
lg https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/browser/she... File chrome/browser/shell_integration_mac.mm (right): https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/browser/she... chrome/browser/shell_integration_mac.mm:47: if (CanSetAsDefaultBrowser() != CHANGE_DEFAULT_UNATTENDED) CanSetAsDefaultBrowser -> CanSetAsDefaultProtocolClient https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/browser/ui/... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/browser/ui/... chrome/browser/ui/startup/default_browser_prompt.cc:41: UMA_HISTOGRAM_COUNTS("DefaultBrowserWarning.SetAsDefaultUIFailed", 1); please make sure that the description for this in histograms.xml indicates that it means failure or cancelled by user (if that is, indeed, the case). https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.cc:617: bool LaunchSelectDefaultHttpHandlerDialog() { since this exact same thing will be needed for the rph case, please change it so that it takes a const wchar_t* protocol as a parameter, and pass L"http" down in ShowMakeChromeDefaultSystemUI(). https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.cc:1044: int shell_change, indeed, looks like this is never used. please remove it. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.cc:1048: << "This routine is intended for Windows 8 and newer only"; remove the text here since it's obvious. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.cc:1060: // http://msdn.microsoft.com/en-us/library/cc144154(VS.85).aspx i don't think this link is relevant. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.h:242: // on Windows 7, too. This is a blocking call. On 2012/05/31 22:17:20, gab wrote: > Please change "This is intended for Windows 8, but is known to work on Windows > 7, too." > to "This is intended for Windows 8 and above only." > > So that the contract in the comment matches the implementation. Agreed. If you'd like to leave a note for posterity indicating that it technically does work in Win7 as well, please put it in the implementation since it's not relevant to consumers of this function. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.h:246: int shell_change, is this parameter needed? my understanding is that this function is purely a per-user operation.
See more comments below. Cheers, Gab https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.cc:1046: bool elevate_if_not_admin) { |elevate_if_not_admin| is only false when we register chrome without the intention of making it default. In this case you are about to make it default and if chrome is not registered you won't be able to. Thus |elevate_if_not_admin| should always be true. Please remove this parameter and hard-code "true" in the call below. This will pop a UAC for now, but my upcoming patch doesn't pop UAC if Win8+ even if |elevate_if_not_admin|. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.cc:1052: ShellUtil::RegisterChromeBrowser(dist, chrome_exe, L"", elevate_if_not_admin); This call should be preceeded by !IsChromeRegistered (i.e. like http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/installer/util/shel...) imho IsChromeRegistered() should be the first check in RegisterChromeBrowser(), but it isn't for now so you'll need this to avoid registering Chrome when it already is.
https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/browser/she... File chrome/browser/shell_integration_mac.mm (right): https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/browser/she... chrome/browser/shell_integration_mac.mm:47: if (CanSetAsDefaultBrowser() != CHANGE_DEFAULT_UNATTENDED) On 2012/06/01 14:36:39, grt wrote: > CanSetAsDefaultBrowser -> CanSetAsDefaultProtocolClient Done. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/browser/ui/... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/browser/ui/... chrome/browser/ui/startup/default_browser_prompt.cc:41: UMA_HISTOGRAM_COUNTS("DefaultBrowserWarning.SetAsDefaultUIFailed", 1); On 2012/06/01 14:36:39, grt wrote: > please make sure that the description for this in histograms.xml indicates that > it means failure or cancelled by user (if that is, indeed, the case). Ack. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.cc:617: bool LaunchSelectDefaultHttpHandlerDialog() { On 2012/06/01 14:36:39, grt wrote: > since this exact same thing will be needed for the rph case, please change it so > that it takes a const wchar_t* protocol as a parameter, and pass L"http" down in > ShowMakeChromeDefaultSystemUI(). Done. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.cc:1044: int shell_change, On 2012/06/01 14:36:39, grt wrote: > indeed, looks like this is never used. please remove it. Done. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.cc:1046: bool elevate_if_not_admin) { On 2012/06/01 15:27:08, gab wrote: > |elevate_if_not_admin| is only false when we register chrome without the > intention of making it default. > > In this case you are about to make it default and if chrome is not registered > you won't be able to. > > Thus |elevate_if_not_admin| should always be true. > > Please remove this parameter and hard-code "true" in the call below. > > This will pop a UAC for now, but my upcoming patch doesn't pop UAC if Win8+ even > if |elevate_if_not_admin|. Done. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.cc:1048: << "This routine is intended for Windows 8 and newer only"; On 2012/06/01 14:36:39, grt wrote: > remove the text here since it's obvious. Done. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.cc:1052: ShellUtil::RegisterChromeBrowser(dist, chrome_exe, L"", elevate_if_not_admin); On 2012/06/01 15:27:08, gab wrote: > This call should be preceeded by !IsChromeRegistered (i.e. like > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/installer/util/shel...) > > imho IsChromeRegistered() should be the first check in RegisterChromeBrowser(), > but it isn't for now so you'll need this to avoid registering Chrome when it > already is. Man, none of that existed when I wrote this first time. Done. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.cc:1060: // http://msdn.microsoft.com/en-us/library/cc144154(VS.85).aspx On 2012/06/01 14:36:39, grt wrote: > i don't think this link is relevant. Done. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.h:242: // on Windows 7, too. This is a blocking call. On 2012/06/01 14:36:39, grt wrote: > On 2012/05/31 22:17:20, gab wrote: > > Please change "This is intended for Windows 8, but is known to work on Windows > > 7, too." > > to "This is intended for Windows 8 and above only." > > > > So that the contract in the comment matches the implementation. > > Agreed. If you'd like to leave a note for posterity indicating that it > technically does work in Win7 as well, please put it in the implementation since > it's not relevant to consumers of this function. Done. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.h:242: // on Windows 7, too. This is a blocking call. On 2012/05/31 22:17:20, gab wrote: > Please change "This is intended for Windows 8, but is known to work on Windows > 7, too." > to "This is intended for Windows 8 and above only." > > So that the contract in the comment matches the implementation. Ack. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.h:246: int shell_change, On 2012/06/01 14:36:39, grt wrote: > is this parameter needed? my understanding is that this function is purely a > per-user operation. Done.
looks great. down to just some nits. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.cc:1052: ShellUtil::RegisterChromeBrowser(dist, chrome_exe, L"", elevate_if_not_admin); On 2012/06/01 20:50:17, motek. wrote: > On 2012/06/01 15:27:08, gab wrote: > > This call should be preceeded by !IsChromeRegistered (i.e. like > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/installer/util/shel...) > > > > imho IsChromeRegistered() should be the first check in > RegisterChromeBrowser(), > > but it isn't for now so you'll need this to avoid registering Chrome when it > > already is. > Man, none of that existed when I wrote this first time. Done. gab: are you sure this is needed? It looks to me like RegisterChromeBrowser does call IsChromeRegistered before doing work. https://chromiumcodereview.appspot.com/10453041/diff/16001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/16001/chrome/installer/u... chrome/installer/util/shell_util.cc:617: bool LaunchSelectDefaultHttpHandlerDialog(const wchar_t* protocol) { Replace "Http" with "Protocol" in the function name, and update the comment accordingly (although may as well leave in a note to the fact that "https" is implied when "http" is passed in on Windows 8). https://chromiumcodereview.appspot.com/10453041/diff/16001/chrome/installer/u... chrome/installer/util/shell_util.cc:618: OPENASINFO open_as_info = {}; DCHECK(protocol); just before this https://chromiumcodereview.appspot.com/10453041/diff/16001/chrome/installer/u... chrome/installer/util/shell_util.cc:1045: DCHECK(base::win::GetVersion() >= base::win::VERSION_WIN8); nit: DCHECK_GE(base::win::GetVersion(), base::win::VERSION_WIN8); (sorry i didn't mention it before. it just popped out at me now.) https://chromiumcodereview.appspot.com/10453041/diff/16001/chrome/installer/u... chrome/installer/util/shell_util.cc:1050: !RegisterChromeBrowser(dist, chrome_exe, L"", true)) { ubernit: L"" -> string()
See comments below, sorry for partly sending you in the wrong direction with IsChromeRegistered. https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u... chrome/installer/util/shell_util.cc:1052: ShellUtil::RegisterChromeBrowser(dist, chrome_exe, L"", elevate_if_not_admin); On 2012/06/04 02:41:14, grt wrote: > On 2012/06/01 20:50:17, motek. wrote: > > On 2012/06/01 15:27:08, gab wrote: > > > This call should be preceeded by !IsChromeRegistered (i.e. like > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/installer/util/shel...) > > > > > > imho IsChromeRegistered() should be the first check in > > RegisterChromeBrowser(), > > > but it isn't for now so you'll need this to avoid registering Chrome when it > > > already is. > > Man, none of that existed when I wrote this first time. Done. > > gab: are you sure this is needed? It looks to me like RegisterChromeBrowser > does call IsChromeRegistered before doing work. Arg, my bad, yes indeed RegisterChromeBrowser does check for IsChromeRegistered. The other call site somehow induced me into thinking otherwise... if (!RegisterChromeBrowser(dist, chrome_exe, string(), true)) return false; is sufficient. I just fixed the confusing definition and usage of RegisterChromeBrowser myself: https://chromiumcodereview.appspot.com/10512026 https://chromiumcodereview.appspot.com/10453041/diff/16001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/16001/chrome/installer/u... chrome/installer/util/shell_util.cc:1044: static const wchar_t* protocol = L"http"; nit: static strings in the Chrome codebase should not be wide strings, instead make this a char string and use ASCIIToUTF16(protocol) below at run-time. https://chromiumcodereview.appspot.com/10453041/diff/16001/chrome/installer/u... chrome/installer/util/shell_util.cc:1044: static const wchar_t* protocol = L"http"; nit: How about "protocol" -->"http_protocol" or hardcoding the string right before its use? From a reader's perspective I thought |protocol| below was a variable until I looked up to the top of this function to realize it was the static string "http".
ptal https://chromiumcodereview.appspot.com/10453041/diff/16001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/16001/chrome/installer/u... chrome/installer/util/shell_util.cc:617: bool LaunchSelectDefaultHttpHandlerDialog(const wchar_t* protocol) { On 2012/06/04 02:41:14, grt wrote: > Replace "Http" with "Protocol" in the function name, and update the comment > accordingly (although may as well leave in a note to the fact that "https" is > implied when "http" is passed in on Windows 8). Done. https://chromiumcodereview.appspot.com/10453041/diff/16001/chrome/installer/u... chrome/installer/util/shell_util.cc:618: OPENASINFO open_as_info = {}; On 2012/06/04 02:41:14, grt wrote: > DCHECK(protocol); just before this Done. https://chromiumcodereview.appspot.com/10453041/diff/16001/chrome/installer/u... chrome/installer/util/shell_util.cc:1044: static const wchar_t* protocol = L"http"; On 2012/06/05 01:48:48, gab wrote: > nit: static strings in the Chrome codebase should not be wide strings, instead > make this a char string and use ASCIIToUTF16(protocol) below at run-time. In-lined as discussed. https://chromiumcodereview.appspot.com/10453041/diff/16001/chrome/installer/u... chrome/installer/util/shell_util.cc:1044: static const wchar_t* protocol = L"http"; On 2012/06/05 01:48:48, gab wrote: > nit: How about "protocol" -->"http_protocol" or hardcoding the string right > before its use? > From a reader's perspective I thought |protocol| below was a variable until I > looked up to the top of this function to realize it was the static string > "http". Ack. https://chromiumcodereview.appspot.com/10453041/diff/16001/chrome/installer/u... chrome/installer/util/shell_util.cc:1045: DCHECK(base::win::GetVersion() >= base::win::VERSION_WIN8); On 2012/06/04 02:41:14, grt wrote: > nit: DCHECK_GE(base::win::GetVersion(), base::win::VERSION_WIN8); > (sorry i didn't mention it before. it just popped out at me now.) Done. https://chromiumcodereview.appspot.com/10453041/diff/16001/chrome/installer/u... chrome/installer/util/shell_util.cc:1050: !RegisterChromeBrowser(dist, chrome_exe, L"", true)) { On 2012/06/04 02:41:14, grt wrote: > ubernit: L"" -> string() string16() ;-)? Done.
Looks good (I'm excited about this!), see comments below. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... File chrome/browser/shell_integration.h (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... chrome/browser/shell_integration.h:40: enum DefaultSettingsChangePermission { This name "DefaultSettingsChangePermission" is very long and I don't feel it conveys the advantage of a long name (i.e. its not really clear what it is from the name anyways). What about something like "MakeDefaultOption" or "MakeDefaultMode"? and then change the names below to MAKE_DEFAULT_* as the intent is to *make* chrome default not *change* the default to a random browser. I'm open to replacing 'make' with set' in my above suggestions too (i.e. we have SetDefault functions and MakeDefault functions in the code, but no ChangeDefault functions). https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... File chrome/browser/shell_integration_mac.mm (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... chrome/browser/shell_integration_mac.mm:20: return CHANGE_DEFAULT_NOT_ALLOWED; optional: I would personally prefer: const bool is_canary = (chrome::VersionInfo::GetChannel() == chrome::VersionInfo::CHANNEL_CANARY); return is_canary ? CHANGE_DEFAULT_NOT_ALLOWED : CHANGE_DEFAULT_UNATTENDED; I feel this conveys the intention more (on top of being more compact). Note: The indentation might not be correct for the ternary operator in my example (I like things aligned, but Chromium sometimes thinks otherwise...). https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... File chrome/browser/shell_integration_win.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... chrome/browser/shell_integration_win.cc:490: LOG(ERROR) << "Failed to launch the set-default-browser Windows UI."; LOG(DFATAL) is preferable here (as it will DCHECK on debug builds instead of silently logging an error, but will still log an error on release builds). https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:618: bool LaunchSelectDefaultProtocolHandlerDialog(const wchar_t* protocol) { This method should either: - RegisterChromeForProtocol with elevate_if_not_admin = true (note that this also calls RegisterChromeBrowser so that no caller would have to worry about registration :)... given this function is in ShellUtil this is a perfect place to do registration imo) or - Enforce in its contract (i.e. comments) the |protocol| needs to already be registered. and enforce it with DCHECK(ShellUtil::IsChromeRegisteredForProtocol(...)) otherwise we risk popping the dialog and not have Chrome listed in it =(! https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:625: DLOG_IF(WARNING, FAILED(hr)) << "Failed to set as default http handler; hr=0x" "http" --> << protocol << https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:626: << std::hex << hr; I'm not exactly sure which osstream is used for DLOGS, but be warned that << std::hex << changes the output format for ALL future integers passed in this stream! So if the DLOG stream is some kind of global output stream you might be messing up logs all over the place... https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:1058: // webpages"? dialog. We choose the latter. The '?' goes inside the quotes right? https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:1058: // webpages"? dialog. We choose the latter. choose --> chose (i.e. chose is the past tense of choose). https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.h:244: // Parameters as MakeChromeDefault. "Same parameters as"?
lgtm w/ one comment tweak. i'll leave the other things for you and gab to iron out. thanks for bearing with my review delays. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... File chrome/browser/shell_integration_win.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... chrome/browser/shell_integration_win.cc:490: LOG(ERROR) << "Failed to launch the set-default-browser Windows UI."; On 2012/06/06 22:47:21, gab wrote: > LOG(DFATAL) is preferable here (as it will DCHECK on debug builds instead of > silently logging an error, but will still log an error on release builds). I'm not so sure about this. Ideally, an integration test that checks to see if the dialog appears when it's expected should be our way of detecting breakage here rather than a DCHECK, which puts a burden on all Windows developers. I say this as a developer who has several DCHECKs disabled/removed in my local build because they're well known and they get in my way. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:618: bool LaunchSelectDefaultProtocolHandlerDialog(const wchar_t* protocol) { On 2012/06/06 22:47:21, gab wrote: > This method should either: > - RegisterChromeForProtocol with elevate_if_not_admin = true > (note that this also calls RegisterChromeBrowser so that no caller would have to > worry about registration :)... given this function is in ShellUtil this is a > perfect place to do registration imo) Don't do this. This is a small, private, easy to understand helper function with a clear purpose. Please keep it that way. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:626: << std::hex << hr; On 2012/06/06 22:47:21, gab wrote: > I'm not exactly sure which osstream is used for DLOGS, but be warned that << > std::hex << changes the output format for ALL future integers passed in this > stream! > > So if the DLOG stream is some kind of global output stream you might be messing > up logs all over the place... The stream's lifetime is bound to the log message itself, which dies at the semicolon. This use of std::hex is safe. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:1058: // webpages"? dialog. We choose the latter. On 2012/06/06 22:47:21, gab wrote: > choose --> chose (i.e. chose is the past tense of choose). Dunno. It sounds fine to me as-is. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.h:244: // Parameters as MakeChromeDefault. On 2012/06/06 22:47:21, gab wrote: > "Same parameters as"? Good catch. Since it no longer has the same parameters as MakeChromeDefault, please document the params (copy-n-paste if you wish) rather than refer to another function.
Reply to Greg's comment. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:618: bool LaunchSelectDefaultProtocolHandlerDialog(const wchar_t* protocol) { On 2012/06/07 02:50:36, grt wrote: > On 2012/06/06 22:47:21, gab wrote: > > This method should either: > > - RegisterChromeForProtocol with elevate_if_not_admin = true > > (note that this also calls RegisterChromeBrowser so that no caller would have > to > > worry about registration :)... given this function is in ShellUtil this is a > > perfect place to do registration imo) > > Don't do this. This is a small, private, easy to understand helper function > with a clear purpose. Please keep it that way. Ok, but do add it to the contract then, right?
Addressed remarks as discussed. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... File chrome/browser/shell_integration.h (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... chrome/browser/shell_integration.h:40: enum DefaultSettingsChangePermission { On 2012/06/06 22:47:21, gab wrote: > This name "DefaultSettingsChangePermission" is very long and I don't feel it > conveys the advantage of a long name (i.e. its not really clear what it is from > the name anyways). > > What about something like "MakeDefaultOption" or "MakeDefaultMode"? Nah. This type expresses neither mode nor defines an option. It defines a response to 'can I make this default', so 'permission' seems to be the best word here. > > and then change the names below to > MAKE_DEFAULT_* > as the intent is to *make* chrome default not *change* the default to a random > browser. > > I'm open to replacing 'make' with set' in my above suggestions too (i.e. we have > SetDefault functions and MakeDefault functions in the code, but no ChangeDefault > functions). DefaultWebClientSetPermission would not be much shorter, but perhaps more illuminating than the current state or either of proposed alternatives. I'll change to that. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... File chrome/browser/shell_integration_mac.mm (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... chrome/browser/shell_integration_mac.mm:20: return CHANGE_DEFAULT_NOT_ALLOWED; On 2012/06/06 22:47:21, gab wrote: > optional: I would personally prefer: > > const bool is_canary = (chrome::VersionInfo::GetChannel() == > chrome::VersionInfo::CHANNEL_CANARY); > return is_canary ? CHANGE_DEFAULT_NOT_ALLOWED : > CHANGE_DEFAULT_UNATTENDED; > > I feel this conveys the intention more (on top of being more compact). > > Note: The indentation might not be correct for the ternary operator in my > example (I like things aligned, but Chromium sometimes thinks otherwise...). I think current version is in keeping with the style around (just like what you propose would be) and I would rather that it stays. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... File chrome/browser/shell_integration_win.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... chrome/browser/shell_integration_win.cc:490: LOG(ERROR) << "Failed to launch the set-default-browser Windows UI."; On 2012/06/06 22:47:21, gab wrote: > LOG(DFATAL) is preferable here (as it will DCHECK on debug builds instead of > silently logging an error, but will still log an error on release builds). No code around here appears to do it in the same situation. Could you please explain what makes this particular situation different? Intuitively, there is nothing 'FATAL' to failing this operation, just like in any other case here. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... chrome/browser/shell_integration_win.cc:490: LOG(ERROR) << "Failed to launch the set-default-browser Windows UI."; On 2012/06/07 02:50:36, grt wrote: > On 2012/06/06 22:47:21, gab wrote: > > LOG(DFATAL) is preferable here (as it will DCHECK on debug builds instead of > > silently logging an error, but will still log an error on release builds). > > I'm not so sure about this. Ideally, an integration test that checks to see if > the dialog appears when it's expected should be our way of detecting breakage > here rather than a DCHECK, which puts a burden on all Windows developers. I say > this as a developer who has several DCHECKs disabled/removed in my local build > because they're well known and they get in my way. Ack. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:618: bool LaunchSelectDefaultProtocolHandlerDialog(const wchar_t* protocol) { On 2012/06/07 02:50:36, grt wrote: > On 2012/06/06 22:47:21, gab wrote: > > This method should either: > > - RegisterChromeForProtocol with elevate_if_not_admin = true > > (note that this also calls RegisterChromeBrowser so that no caller would have > to > > worry about registration :)... given this function is in ShellUtil this is a > > perfect place to do registration imo) > > Don't do this. This is a small, private, easy to understand helper function > with a clear purpose. Please keep it that way. Ack. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:618: bool LaunchSelectDefaultProtocolHandlerDialog(const wchar_t* protocol) { On 2012/06/06 22:47:21, gab wrote: > This method should either: > - RegisterChromeForProtocol with elevate_if_not_admin = true > (note that this also calls RegisterChromeBrowser so that no caller would have to > worry about registration :)... given this function is in ShellUtil this is a > perfect place to do registration imo) > > or > - Enforce in its contract (i.e. comments) the |protocol| needs to already be > registered. > and enforce it with DCHECK(ShellUtil::IsChromeRegisteredForProtocol(...)) > > otherwise we risk popping the dialog and not have Chrome listed in it =(! Ack. But see below. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:618: bool LaunchSelectDefaultProtocolHandlerDialog(const wchar_t* protocol) { On 2012/06/07 14:01:51, gab wrote: > On 2012/06/07 02:50:36, grt wrote: > > On 2012/06/06 22:47:21, gab wrote: > > > This method should either: > > > - RegisterChromeForProtocol with elevate_if_not_admin = true > > > (note that this also calls RegisterChromeBrowser so that no caller would > have > > to > > > worry about registration :)... given this function is in ShellUtil this is a > > > perfect place to do registration imo) > > > > Don't do this. This is a small, private, easy to understand helper function > > with a clear purpose. Please keep it that way. > > Ok, but do add it to the contract then, right? Well, perhaps to comments. Enforcement through DCHECK seems spurious and would require passing extra parameters JUST to make that DCHECK. Updated the comment. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:625: DLOG_IF(WARNING, FAILED(hr)) << "Failed to set as default http handler; hr=0x" On 2012/06/06 22:47:21, gab wrote: > "http" --> << protocol << Done. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:1058: // webpages"? dialog. We choose the latter. On 2012/06/06 22:47:21, gab wrote: > The '?' goes inside the quotes right? Done. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:1058: // webpages"? dialog. We choose the latter. On 2012/06/06 22:47:21, gab wrote: > choose --> chose (i.e. chose is the past tense of choose). This is as intended. I find the present tense to be perfectly natural in this context. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:1058: // webpages"? dialog. We choose the latter. On 2012/06/07 02:50:36, grt wrote: > On 2012/06/06 22:47:21, gab wrote: > > choose --> chose (i.e. chose is the past tense of choose). > > Dunno. It sounds fine to me as-is. Ack. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.h:244: // Parameters as MakeChromeDefault. On 2012/06/06 22:47:21, gab wrote: > "Same parameters as"? Not anymore. Done. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.h:244: // Parameters as MakeChromeDefault. On 2012/06/07 02:50:36, grt wrote: > On 2012/06/06 22:47:21, gab wrote: > > "Same parameters as"? > > Good catch. Since it no longer has the same parameters as MakeChromeDefault, > please document the params (copy-n-paste if you wish) rather than refer to > another function. Ack.
LGTM, some opinions and a single nit below. Cheers! Gab https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... File chrome/browser/shell_integration_win.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/browser/she... chrome/browser/shell_integration_win.cc:490: LOG(ERROR) << "Failed to launch the set-default-browser Windows UI."; On 2012/06/07 14:58:50, motek. wrote: > On 2012/06/06 22:47:21, gab wrote: > > LOG(DFATAL) is preferable here (as it will DCHECK on debug builds instead of > > silently logging an error, but will still log an error on release builds). > No code around here appears to do it in the same situation. Could you please > explain what makes this particular situation different? Intuitively, there is > nothing 'FATAL' to failing this operation, just like in any other case here. My thinking is that it's not 'FATAL' (and that's why we don't want to crash on Release builds), but it's nice to know on Debug builds (say when Windows 9 comes out) if we can't pop this anymore, i.e. we would hit a DCHECK as opposed to having to dig to find the root of some weird bug. On the other hand it might be more handy when Win9 comes out to just look at the 'ERROR's in the log and know all that we have to fix instead of fixing a DCHECK only to fall on the next DCHECK. I don't feel strongly about it, it's up to you. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:618: bool LaunchSelectDefaultProtocolHandlerDialog(const wchar_t* protocol) { On 2012/06/07 14:58:50, motek. wrote: > On 2012/06/07 14:01:51, gab wrote: > > On 2012/06/07 02:50:36, grt wrote: > > > On 2012/06/06 22:47:21, gab wrote: > > > > This method should either: > > > > - RegisterChromeForProtocol with elevate_if_not_admin = true > > > > (note that this also calls RegisterChromeBrowser so that no caller would > > have > > > to > > > > worry about registration :)... given this function is in ShellUtil this is > a > > > > perfect place to do registration imo) > > > > > > Don't do this. This is a small, private, easy to understand helper function > > > with a clear purpose. Please keep it that way. > > > > Ok, but do add it to the contract then, right? > Well, perhaps to comments. Enforcement through DCHECK seems spurious and would > require passing extra parameters JUST to make that DCHECK. Updated the comment. Arg, indeed, for now, because you need to know the suffix... I'm adding code as we speak to have a method to do this, ShellUtil::GetCurrentInstallationSuffix(): http://codereview.chromium.org/10451074/ I'm fine with no DCHECK for now then :(. https://chromiumcodereview.appspot.com/10453041/diff/30001/chrome/installer/u... chrome/installer/util/shell_util.cc:626: << std::hex << hr; On 2012/06/07 02:50:36, grt wrote: > On 2012/06/06 22:47:21, gab wrote: > > I'm not exactly sure which osstream is used for DLOGS, but be warned that << > > std::hex << changes the output format for ALL future integers passed in this > > stream! > > > > So if the DLOG stream is some kind of global output stream you might be > messing > > up logs all over the place... > > The stream's lifetime is bound to the log message itself, which dies at the > semicolon. This use of std::hex is safe. Awesome, good to know :). https://chromiumcodereview.appspot.com/10453041/diff/32003/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/10453041/diff/32003/chrome/installer/u... chrome/installer/util/shell_util.h:245: // chrome_exe: The chrome.exe path to register as default browser. nit: Variables should be englobed in '|', i.e.: |dist|: The type of browser distribution currently in use. |chrome_exe|: The path to the chrome exe to register as default browser.
lgtm++ https://chromiumcodereview.appspot.com/10453041/diff/32003/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/10453041/diff/32003/chrome/installer/u... chrome/installer/util/shell_util.h:245: // chrome_exe: The chrome.exe path to register as default browser. On 2012/06/07 15:18:54, gab wrote: > nit: Variables should be englobed in '|', i.e.: > > |dist|: The type of browser distribution currently in use. > |chrome_exe|: The path to the chrome exe to register as default browser. gab's right, but for the sake of staying with the style present in the file, i think it's fine to leave it as-is. your call. (land this thing!)
https://chromiumcodereview.appspot.com/10453041/diff/32003/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/10453041/diff/32003/chrome/installer/u... chrome/installer/util/shell_util.h:245: // chrome_exe: The chrome.exe path to register as default browser. On 2012/06/07 15:18:54, gab wrote: > nit: Variables should be englobed in '|', i.e.: > > |dist|: The type of browser distribution currently in use. > |chrome_exe|: The path to the chrome exe to register as default browser. Done. https://chromiumcodereview.appspot.com/10453041/diff/32003/chrome/installer/u... chrome/installer/util/shell_util.h:245: // chrome_exe: The chrome.exe path to register as default browser. On 2012/06/07 15:30:32, grt wrote: > On 2012/06/07 15:18:54, gab wrote: > > nit: Variables should be englobed in '|', i.e.: > > > > |dist|: The type of browser distribution currently in use. > > |chrome_exe|: The path to the chrome exe to register as default browser. > > gab's right, but for the sake of staying with the style present in the file, i > think it's fine to leave it as-is. your call. (land this thing!) Ack.
I only looked at chrome/browser/ui. LGTM there, assuming ShellIntegration can be accessed from the file thread. https://chromiumcodereview.appspot.com/10453041/diff/33004/chrome/browser/ui/... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/33004/chrome/browser/ui/... chrome/browser/ui/startup/default_browser_prompt.cc:37: void SetChromeAsDefaultBrowser(bool interactive_flow) { I'm not familiar with shell integration. Is it thread safe? https://chromiumcodereview.appspot.com/10453041/diff/33004/chrome/browser/ui/... chrome/browser/ui/startup/default_browser_prompt.cc:81: bool interactive_flow_required_; const
Thanks. About to submit. https://chromiumcodereview.appspot.com/10453041/diff/33004/chrome/browser/ui/... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://chromiumcodereview.appspot.com/10453041/diff/33004/chrome/browser/ui/... chrome/browser/ui/startup/default_browser_prompt.cc:37: void SetChromeAsDefaultBrowser(bool interactive_flow) { On 2012/06/07 17:52:20, sky wrote: > I'm not familiar with shell integration. Is it thread safe? I believe it is. This is an equivalent to what used to be called from DefaultBrowserInfoBarDelegate::Accept (spun to FILE thread).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/motek@chromium.org/10453041/37021
Try job failure for 10453041-37021 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/motek@chromium.org/10453041/37021
Try job failure for 10453041-37021 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/motek@chromium.org/10453041/37021
Change committed as 141172 |
