Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1349163008:
Setting chrome as the default browser is now fixed on Windows 10 (Closed)
|
Created:
5 years, 3 months ago by Patrick Monette Modified:
5 years, 1 month ago Reviewers:
nikunjb, rkaplow, sky, Alexei Svitkine (slow), Peter Kasting, brettw, ananta, grt (UTC plus 2) CC:
chromium-reviews, vabr+watchlist_chromium.org, asvitkine+watch_chromium.org, hodie Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSetting chrome as the default browser is now fixed on Windows 10
BUG=531649
Committed: https://crrev.com/f89ac7c739d5b0ba039af8cabac76a74ff3b2b54
Cr-Commit-Position: refs/heads/master@{#352526}
Patch Set 1 #
Total comments: 29
Patch Set 2 : asvitkine comments #1 #
Total comments: 15
Patch Set 3 : Now using OpenWith.exe to open the http url #
Total comments: 25
Patch Set 4 : grt comments #1 #
Total comments: 53
Patch Set 5 : Rebasing #Patch Set 6 : Test for default browser callback + comments #
Total comments: 87
Patch Set 7 : Responding to comments #
Total comments: 2
Patch Set 8 : Report early success when already default browser #
Total comments: 10
Patch Set 9 : Added detailed metrics #
Total comments: 12
Patch Set 10 : More comments #Patch Set 11 : Rebasing #
Total comments: 2
Patch Set 12 : Removed unused header #
Total comments: 2
Patch Set 13 : Modified a comment #Patch Set 14 : Fixed compilation for non-Windows build #Patch Set 15 : Fix compilation again #Patch Set 16 : Fix compilation again #
Total comments: 2
Messages
Total messages: 74 (24 generated)
pmonette@chromium.org changed reviewers: + asvitkine@chromium.org, pkasting@chromium.org, sky@chromium.org
asvitkine@chromium.org: Please review changes in histograms.xml pkasting@chromium.org: Please review changes in startup_browser_creator.h .cc sky@chromium.org: Please review changes in shell_integration.h .cc _win.cc and the small changes in the unit tests. Thanks.
https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:696: if (succeeded) { Nit: No {}'s for 1-line bodies. Same for code in the function above. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:697: UMA_HISTOGRAM_COUNTS("AsyncSetAsDefault.Success", 1); For, for a boolean histogram, you should use UMA_HISTOGRAM_BOOLEAN(). Second, it's useful to log both the true and false value, so you can see what the ratio is. Otherwise, just a count is not very meaningful (e.g. what does a number like 32434 say? is that a lot? a little?) https://codereview.chromium.org/1349163008/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1349163008/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:1813: +<histogram name="AsyncSetAsDefault.Duration" units="milliseconds"> The AsyncSetAsDefault top-level prefix doesn't sound very meaningful to me. Suggest nesting things under the existing DefaultBrowser.* prefix.
sky@chromium.org changed reviewers: + ananta@chromium.org
+ananta
Responded to Alexei. Please take another look. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:696: if (succeeded) { On 2015/09/22 15:20:43, Alexei Svitkine (slow) wrote: > Nit: No {}'s for 1-line bodies. Same for code in the function above. Done. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:697: UMA_HISTOGRAM_COUNTS("AsyncSetAsDefault.Success", 1); On 2015/09/22 15:20:43, Alexei Svitkine (slow) wrote: > For, for a boolean histogram, you should use UMA_HISTOGRAM_BOOLEAN(). > > Second, it's useful to log both the true and false value, so you can see what > the ratio is. Otherwise, just a count is not very meaningful (e.g. what does a > number like 32434 say? is that a lot? a little?) Done. The reasoning was that we already have the total count with AsyncSetAsDefault.Duration. https://codereview.chromium.org/1349163008/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1349163008/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:1813: +<histogram name="AsyncSetAsDefault.Duration" units="milliseconds"> On 2015/09/22 15:20:43, Alexei Svitkine (slow) wrote: > The AsyncSetAsDefault top-level prefix doesn't sound very meaningful to me. > Suggest nesting things under the existing DefaultBrowser.* prefix. Done.
Responded to Alexei. Please take another look. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:696: if (succeeded) { On 2015/09/22 15:20:43, Alexei Svitkine (slow) wrote: > Nit: No {}'s for 1-line bodies. Same for code in the function above. Done. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:697: UMA_HISTOGRAM_COUNTS("AsyncSetAsDefault.Success", 1); On 2015/09/22 15:20:43, Alexei Svitkine (slow) wrote: > For, for a boolean histogram, you should use UMA_HISTOGRAM_BOOLEAN(). > > Second, it's useful to log both the true and false value, so you can see what > the ratio is. Otherwise, just a count is not very meaningful (e.g. what does a > number like 32434 say? is that a lot? a little?) Done. The reasoning was that we already have the total count with AsyncSetAsDefault.Duration. https://codereview.chromium.org/1349163008/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1349163008/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:1813: +<histogram name="AsyncSetAsDefault.Duration" units="milliseconds"> On 2015/09/22 15:20:43, Alexei Svitkine (slow) wrote: > The AsyncSetAsDefault top-level prefix doesn't sound very meaningful to me. > Suggest nesting things under the existing DefaultBrowser.* prefix. Done.
grt@chromium.org changed reviewers: + grt@chromium.org
fyi to OWNERS: i am doing a top-to-bottom review of this. feedback coming later today. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration.cc:246: // Only notify observer is |UninitializeSetAsDefault| returns true. This is to is -> if https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration.cc:312: } this should run CompleteSetAsDefault with result=false, no? if (interactive_permitted) result = ShellIntegration::SetAsDefaultBrowserInteractive(); BrowserThread::PostTask(...); https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration.cc:318: default: while you're here, please change this from "default:" to "case SET_DEFAULT_NOT_ALLOWED:" so that the compiler will spew errors if/when new values are added to the enum. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration.h:64: // In Windows 8 a browser can be made default-in-metro only in an interactive please update comment with a note about Win10 using an asynchronous flow. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration.h:224: // On windows 10+, there is no official way to prompt the user to set a this seems like an implementation detail that consumers don't need to know about. please move into the .cc file. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration.h:264: #if defined(OS_WIN) move these into the private: block at the bottom of the class (just above DISALLOW_COPY_AND_ASSIGN); see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O.... https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:256: static const wchar_t* kUrlAssociationKeyFormats[] = { static const wchar_t* const https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:261: static const wchar_t* kProtocols[] = {L"http", L"https"}; static const wchar_t* const https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:267: KEY_WRITE); KEY_WRITE -> KEY_SET_VALUE https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:296: DCHECK(base::win::GetVersion() >= base::win::VERSION_WIN10); DCHECK_GE https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:299: ShellExecute(0, 0, kSetDefaultBrowserHelpUrl, 0, 0, SW_SHOW); does something like this work? CommandLine cmd(L"openwith.exe"); cmd.AppendArg(kSetDefaultBrowserHelpUrl); base::LaunchProcess(cmd); https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:655: StartupBrowserCreator::AddUrlFilter( this seems like a good place to put the comment explaining how the filter and timeout work. https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:670: timer_duration = base::TimeDelta::FromSeconds(std::atoi(value.c_str())); atoi -> base::StringToInt. it has interesting failure modes, so maybe something like: int seconds = 0; if (!value.empty()) base::StringToInt(value, &seconds); if (seconds) timer_duration = base::TimeDelta::FromSeconds(seconds); https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:457: if (it != filters->end()) { nit to reduce braces: if (it == filters->end()) return false; it->second.Run(); return true; https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:474: GetUrlFilters()->insert({url, callback}); how about: (*GetUrlFilters())[url] = callback; https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:482: filters->erase(filters->find(url)); remove the DCHECK above and change this to: size_t num_erased = filters->erase(url); DCHECK_NE(0, num_erased); https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:690: // Filters out specific urls. nit: Filters -> Filter https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:879: static UrlFilters* url_filters_ = new UrlFilters(); in the vast majority of Chrome runs, there will never be any filters. how about avoiding the alloc of it until it's actually needed? as it is, HasUrlFilters will cause the instance to be created to check to see if it's empty. alternatively, you could make url_filters_ a private static class member so that the filtering code could be: if (url_filters_) { ... } https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.h:35: typedef std::map<GURL, base::Callback<void(void)>> UrlFilters; base::Callback<void(void)> -> base::Closure https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.h:108: // All the filters functions should be called on the UI thread. comment suggestion: // Adds a filter that will execute |closure| on the UI thread when |url| is // opened via the command-line. https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.h:112: const base::Callback<void(void)>& callback); base::Callback<void(void)> -> base::Closure https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.h:117: // Returns true if at least one url filter exists. this doesn't look like it needs to be part of the public api, does it? https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.h:122: static bool ExecuteFilterCallback(const GURL& url); does this need to be public? https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.h:173: // Returns the map of all the url filters. This will creatE the filter if it creatE -> create https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:49: return CanSetAsDefaultBrowser(); is this correct? should this map _ASYNCHRONOUS to _INTERACTIVE? https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:316: break; this also needs to post a call to CompleteSetAsDefault(false) for the else case, no? https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:345: BrowserThread::PostTask( all codepaths here must call PostTask. i suggest you move it down to the bottom of the function and just set |result| as appropriate in this switch statement. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:367: } case ShellIntegration::SET_DEFAULT_ASYNCHRONOUS: NOTREACHED(); break; https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.h:276: // Function that sets Chrome as the default web client. please add to the comment that implementations MUST post a task to run CompleteSetAsDefault on the UI thread with the result of the operation.
On 2015/09/22 18:21:48, grt wrote: > fyi to OWNERS: i am doing a top-to-bottom review of this. feedback coming later > today. well, there's some of the feedback. :-) more to come.
https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration.h:264: #if defined(OS_WIN) On 2015/09/22 18:21:47, grt wrote: > move these into the private: block at the bottom of the class (just above > DISALLOW_COPY_AND_ASSIGN); see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O.... actually, it looks like they can be among DefaultBrowserWorker's private members. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:150: void ShellIntegration::DefaultBrowserWorker::InitializeSetAsDefault() {} nit: move these down immediately after ShellIntegration::DefaultBrowserWorker::SetAsDefault to preserve the same order as the header as per http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O.... https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:194: InitializeSetAsDefault(); there's no need to register the filter or run the timer if there's no observer_, is there? https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:206: observer_ = NULL; likewise, the timer/filter can be cancelled if the observer goes away. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:245: // Only notify observer is |UninitializeSetAsDefault| returns true. This is to i think it's cleaner for DefaultWebClientWorker to have its own state (e.g., a bool) that it uses to protect against multiple calls to CompleteSetAsDefault. also, please add a note to CompleteSetAsDefault's doc comment explaining that it is safe for subclasses to call it multiple times, and that only the first call is processed. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:316: break; On 2015/09/22 18:21:48, grt wrote: > this also needs to post a call to CompleteSetAsDefault(false) for the else case, > no? maybe it's cleaner to have one PostTask at the bottom of the function, with the one exception (SET_DEFAULT_ASYNCHRONOUS) doing an early return. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.h:279: // Executed before calling |ExecuteSetAsDefault|. comment suggestion: // Invoked on the UI thread prior to starting a SetAsDefault operation. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.h:284: virtual bool UninitializeSetAsDefault(bool succeeded) { return true; } suggestion: // Invoked on the UI thread following a SetAsDefault operation. virtual void FinalizeSetAsDefault(bool succeeded) {} see comment in .cc file about not returning bool. another possibility for these methods: PreSetAsDefault PostSetAsDefault
https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.h:266: scoped_ptr<base::OneShotTimer<DefaultWebClientWorker>> async_timer_; FYI: it looks like OneShotTimer is getting a bit of a tweak in https://codereview.chromium.org/1355063004/.
Why add this whole concept of a generic URL-to-callback map so we can maybe add one URL from one callsite? At first glance this seems overdesigned. Especially if you make grt's suggested change to remove the need to "uninitialize", can we just get by with a simple command-line flag to trigger setting as default, or the like?
Patchset #4 (id:60001) has been deleted
Addressed grt's comments. Please take another look. pkasting@: I removed the general Url to callback filters. PTAL. anante@ or sky@: Can you look at changes in shell_integration.h .cc _win.cc https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration.cc:246: // Only notify observer is |UninitializeSetAsDefault| returns true. This is to On 2015/09/22 18:21:47, grt wrote: > is -> if Comment removed. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration.cc:312: } On 2015/09/22 18:21:47, grt wrote: > this should run CompleteSetAsDefault with result=false, no? > if (interactive_permitted) > result = ShellIntegration::SetAsDefaultBrowserInteractive(); > BrowserThread::PostTask(...); CompleteSetAsDefault is not called at the end with the Asynchronous case returning early. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration.cc:318: default: On 2015/09/22 18:21:47, grt wrote: > while you're here, please change this from "default:" to "case > SET_DEFAULT_NOT_ALLOWED:" so that the compiler will spew errors if/when new > values are added to the enum. Done. Moved the cases around so they are in the same order as they are defined. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration.h:64: // In Windows 8 a browser can be made default-in-metro only in an interactive On 2015/09/22 18:21:47, grt wrote: > please update comment with a note about Win10 using an asynchronous flow. Done. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration.h:224: // On windows 10+, there is no official way to prompt the user to set a On 2015/09/22 18:21:47, grt wrote: > this seems like an implementation detail that consumers don't need to know > about. please move into the .cc file. Done. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration.h:264: #if defined(OS_WIN) On 2015/09/22 19:17:48, grt wrote: > On 2015/09/22 18:21:47, grt wrote: > > move these into the private: block at the bottom of the class (just above > > DISALLOW_COPY_AND_ASSIGN); see > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O.... > > actually, it looks like they can be among DefaultBrowserWorker's private > members. Done. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:256: static const wchar_t* kUrlAssociationKeyFormats[] = { On 2015/09/22 18:21:47, grt wrote: > static const wchar_t* const Done. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:261: static const wchar_t* kProtocols[] = {L"http", L"https"}; On 2015/09/22 18:21:47, grt wrote: > static const wchar_t* const Done. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:267: KEY_WRITE); On 2015/09/22 18:21:47, grt wrote: > KEY_WRITE -> KEY_SET_VALUE Done. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:296: DCHECK(base::win::GetVersion() >= base::win::VERSION_WIN10); On 2015/09/22 18:21:47, grt wrote: > DCHECK_GE Done. https://codereview.chromium.org/1349163008/diff/1/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:299: ShellExecute(0, 0, kSetDefaultBrowserHelpUrl, 0, 0, SW_SHOW); On 2015/09/22 18:21:47, grt wrote: > does something like this work? > CommandLine cmd(L"openwith.exe"); > cmd.AppendArg(kSetDefaultBrowserHelpUrl); > base::LaunchProcess(cmd); Done. https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:655: StartupBrowserCreator::AddUrlFilter( On 2015/09/22 18:21:47, grt wrote: > this seems like a good place to put the comment explaining how the filter and > timeout work. Done. https://codereview.chromium.org/1349163008/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:670: timer_duration = base::TimeDelta::FromSeconds(std::atoi(value.c_str())); On 2015/09/22 18:21:47, grt wrote: > atoi -> base::StringToInt. it has interesting failure modes, so maybe something > like: > > int seconds = 0; > if (!value.empty()) > base::StringToInt(value, &seconds); > if (seconds) > timer_duration = base::TimeDelta::FromSeconds(seconds); Done. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:49: return CanSetAsDefaultBrowser(); On 2015/09/22 18:21:48, grt wrote: > is this correct? should this map _ASYNCHRONOUS to _INTERACTIVE? Done. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:150: void ShellIntegration::DefaultBrowserWorker::InitializeSetAsDefault() {} On 2015/09/22 19:17:48, grt wrote: > nit: move these down immediately after > ShellIntegration::DefaultBrowserWorker::SetAsDefault to preserve the same order > as the header as per > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O.... Done. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:194: InitializeSetAsDefault(); On 2015/09/22 19:17:48, grt wrote: > there's no need to register the filter or run the timer if there's no observer_, > is there? Done. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:206: observer_ = NULL; On 2015/09/22 19:17:48, grt wrote: > likewise, the timer/filter can be cancelled if the observer goes away. Done. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:245: // Only notify observer is |UninitializeSetAsDefault| returns true. This is to On 2015/09/22 19:17:48, grt wrote: > i think it's cleaner for DefaultWebClientWorker to have its own state (e.g., a > bool) that it uses to protect against multiple calls to CompleteSetAsDefault. > > also, please add a note to CompleteSetAsDefault's doc comment explaining that it > is safe for subclasses to call it multiple times, and that only the first call > is processed. Done. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:316: break; On 2015/09/22 19:17:48, grt wrote: > On 2015/09/22 18:21:48, grt wrote: > > this also needs to post a call to CompleteSetAsDefault(false) for the else > case, > > no? > > maybe it's cleaner to have one PostTask at the bottom of the function, with the > one exception (SET_DEFAULT_ASYNCHRONOUS) doing an early return. I like this suggestion. Done. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:345: BrowserThread::PostTask( On 2015/09/22 18:21:48, grt wrote: > all codepaths here must call PostTask. i suggest you move it down to the bottom > of the function and just set |result| as appropriate in this switch statement. Done. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:367: } On 2015/09/22 18:21:48, grt wrote: > case ShellIntegration::SET_DEFAULT_ASYNCHRONOUS: > NOTREACHED(); > break; Done. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.h:266: scoped_ptr<base::OneShotTimer<DefaultWebClientWorker>> async_timer_; On 2015/09/22 19:19:56, grt wrote: > FYI: it looks like OneShotTimer is getting a bit of a tweak in > https://codereview.chromium.org/1355063004/. Acknowledged. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.h:276: // Function that sets Chrome as the default web client. On 2015/09/22 18:21:48, grt wrote: > please add to the comment that implementations MUST post a task to run > CompleteSetAsDefault on the UI thread with the result of the operation. Done. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.h:279: // Executed before calling |ExecuteSetAsDefault|. On 2015/09/22 19:17:48, grt wrote: > comment suggestion: > // Invoked on the UI thread prior to starting a SetAsDefault operation. Done. https://codereview.chromium.org/1349163008/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration.h:284: virtual bool UninitializeSetAsDefault(bool succeeded) { return true; } On 2015/09/22 19:17:48, grt wrote: > suggestion: > // Invoked on the UI thread following a SetAsDefault operation. > virtual void FinalizeSetAsDefault(bool succeeded) {} > > see comment in .cc file about not returning bool. > > another possibility for these methods: > PreSetAsDefault > PostSetAsDefault Done.
https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:113: base::CancelableClosure async_set_as_default_filter_callback; The Google style guide bans the use of global or static objects of non-POD type. You could use a pointer to a closure and set it to null when it's canceled. That avoids the complexity of CancelableClosure anyway, which you don't actually need; just make sure not to leak memory. You could also just store a DefaultBrowserWorker* and call CompleteSetAsDefault() on it directly, which I find noticeably simpler and clearer, at the cost of a conceptual dependency that to me seems acceptable to introduce, since this code already is understanding that this has to do with handling being set as the default browser. Either one of these would probably simplify your implementation enough to inline the accessors in the header. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:681: GURL url(arg); Nit: I wonder if it wouldn't be simpler to just string-compare, which would avoid the overhead (and the code) of creating a canonical URL out of these. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:683: url == GURL(L"https://support.google.com/chrome?p=default_browser")) { Why are you not using the constant here? https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.h:112: static void SetAsyncSetAsDefaultFilter(base::Closure callback); I find the consistent use of the phrase "AsyncSetAsDefault" everywhere confusing, since I can barely parse it, it leads to multiple verbs in a row ("set async set"), and it doesn't say "default what". Something like this: SetDefaultBrowserCallback(); GetDefaultBrowserURL(); ...might be clearer. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.h:121: // All the filters functions should be called on the UI thread. Looks like all this should have been removed?
looking good. more comments. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:51: // Set as default asynchronous is not supported for all protocols. suggestion: "not supported for all protocols" -> "only supported for default web browser" https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:53: permission = SET_DEFAULT_INTERACTIVE; nit: if (blah) return SET_DEFAULT_INTERACTIVE; return permission; or rename "permission" to "perm" and: return (perm == SET_DEFAULT_ASYNCHRONOUS) ? SET_DEFAULT_INTERACTIVE : perm; https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:185: set_as_default_completed_ = false; should this handle the case where a previous StartSetAsDefault hasn't yet completed? maybe FinalizeSetAsDefault(false) so that the old filter callback is removed? or have InitializeSetAsDefault() be a noop if the timer already exists? https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:206: // up ressources. nit: resources https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:229: if (!set_as_default_completed_) { wdyt of set_as_default_in_progress_ instead of set_as_default_completed_ so that it's false to begin with and true when something is happening? it makes this conditional more clear, to boot. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:236: set_as_default_completed_ = true; nit: move this up into the body of the if() https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:288: if (interactive_permitted) { nit: omit braces https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:295: // asynchronously through a filter in startup_browser_creator.h. suggestion: // asynchronously via a filter established in InitializeSetAsDefault(). https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:45: // The caller should call SetAsyncSetAsDefaultFilterCallback to intercept the this comment isn't necessary. i think it's better to say that this call provides no indication as to whether or not the operation succeeded, and that the reader is more likely to want to use DefaultBrowserWorker, which will invoke its observer as appropriate. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:67: // The browser control policy disallow setting the default browser. even i didn't know what this meant. :-) i think a better comment would be // The browser distribution is not permitted to be made default. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:72: // On Windows 8, a browser can be made fault only in an interactive flow. fault -> default https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:264: // Function that performs the check. Subclasses are responsible for calling nit: mention that this is run on the FILE thread. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:288: bool set_as_default_completed_; bool set_as_default_completed_ = true; and remove this member from the ctor's initializer list in the .cc file https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:316: scoped_ptr<base::OneShotTimer<DefaultBrowserWorker>> async_timer_; the OneShotTimer tweak landed at r350496; please rebase. i think you can move the #include of timer.h from here to the .cc file and forward declare OneShotTimer now. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:301: } suggestion: it would be good to know if this fails. how about base::Process process(base::LaunchProcess(cmdline, base::LaunchOptions())); UMA_HISTOGRAM_BOOLEAN("DefaultBrowser.AsyncSetAsDefault.Launched", process.IsValid()); https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:312: else if (IsSetAsDefaultAsynchronous()) nit: remove these "else"es: if (foo) return blah; if (bar) return blorf; return spam; https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:695: if (IsSetAsDefaultAsynchronous()) { how about: if (async_timer_) { so that there are no surprises if the user's field trial config somehow changes while the timer is running. this also naturally protects from multiple calls. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:113: base::CancelableClosure async_set_as_default_filter_callback; i think CancelableClosure is overkill here since you never need to cancel a callback that is in-flight (posted to a message loop). i think it would suffice to have: base::Closure* g_async_set_as_default_callback = nullptr; and: SetAsyncSetAsDefaultFilter(const base::Closure& callback) { DCHECK_CURRENTLY_ON(BrowserThread::UI); if (callback.is_null()) { delete g_async_set_as_default_callback; g_async_set_as_default_callback = nullptr; return; } if (!g_async_set_as_default_callback) g_async_set_as_default_callback = new base::Closure(callback); else *g_async_set_as_default_callback = callback; } and get rid of ClearAsyncSetAsDefaultFilter (callers can pass an empty callback to clear). one thing to keep in mind is that it's pretty easy to get into a situation where a second worker is started while a first worker is still waiting for the timeout. have you thought about how that will be handled? https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.h:8: #include <map> unused https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.h:112: static void SetAsyncSetAsDefaultFilter(base::Closure callback); nit: pass callbacks by constref
pkasting: question for you. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:110: L"https://support.google.com/chrome?p=default_browser"; PK: it seems like this should be brand-specific. do you think this should come from google_chrome_strings.grd with some other value in chromium_strings.grd, or just put this in a #if defined(GOOGLE_CHROME_BUILD) in some way?
https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:110: L"https://support.google.com/chrome?p=default_browser"; On 2015/09/24 18:47:06, grt wrote: > PK: it seems like this should be brand-specific. do you think this should come > from google_chrome_strings.grd with some other value in chromium_strings.grd, or > just put this in a #if defined(GOOGLE_CHROME_BUILD) in some way? Do we have any support pages for Chromium (not Chrome)? What do we do for other help/support links in the codebase? https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:113: base::CancelableClosure async_set_as_default_filter_callback; On 2015/09/24 18:24:12, grt wrote: > i think CancelableClosure is overkill here since you never need to cancel a > callback that is in-flight (posted to a message loop). i think it would suffice > to have: > > base::Closure* g_async_set_as_default_callback = nullptr; > > and: > > SetAsyncSetAsDefaultFilter(const base::Closure& callback) { > DCHECK_CURRENTLY_ON(BrowserThread::UI); > if (callback.is_null()) { > delete g_async_set_as_default_callback; > g_async_set_as_default_callback = nullptr; > return; > } > if (!g_async_set_as_default_callback) > g_async_set_as_default_callback = new base::Closure(callback); > else > *g_async_set_as_default_callback = callback; > } > > and get rid of ClearAsyncSetAsDefaultFilter (callers can pass an empty callback > to clear). One downside to this is it's prone to leaks. It's not clear who's supposed to delete this if no one calls the API with a null pointer (e.g. we shut down first). One reason I liked my DefaultBrowserWorker* suggestion is that not only do I think it's simpler and clearer, there aren't lifetime problems, because this would be a non-owning pointer, so it can't leak.
https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:113: base::CancelableClosure async_set_as_default_filter_callback; On 2015/09/24 20:34:10, Peter Kasting wrote: > On 2015/09/24 18:24:12, grt wrote: > > i think CancelableClosure is overkill here since you never need to cancel a > > callback that is in-flight (posted to a message loop). i think it would > suffice > > to have: > > > > base::Closure* g_async_set_as_default_callback = nullptr; > > > > and: > > > > SetAsyncSetAsDefaultFilter(const base::Closure& callback) { > > DCHECK_CURRENTLY_ON(BrowserThread::UI); > > if (callback.is_null()) { > > delete g_async_set_as_default_callback; > > g_async_set_as_default_callback = nullptr; > > return; > > } > > if (!g_async_set_as_default_callback) > > g_async_set_as_default_callback = new base::Closure(callback); > > else > > *g_async_set_as_default_callback = callback; > > } > > > > and get rid of ClearAsyncSetAsDefaultFilter (callers can pass an empty > callback > > to clear). > > One downside to this is it's prone to leaks. It's not clear who's supposed to > delete this if no one calls the API with a null pointer (e.g. we shut down > first). > > One reason I liked my DefaultBrowserWorker* suggestion is that not only do I > think it's simpler and clearer, there aren't lifetime problems, because this > would be a non-owning pointer, so it can't leak. On the flipside, it means that there's a risk that the Worker goes away without clearing the pointer here, leaving a dangling pointer. Then you'll get a crash on a subsequent navigation to the magic URL. I'm less concerned about leaking since there's no risk of leaking more than one instance, and we have a policy of leaking global-ish objects at shutdown.
On 2015/09/24 20:39:24, grt wrote: > https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... > File chrome/browser/ui/startup/startup_browser_creator.cc (right): > > https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... > chrome/browser/ui/startup/startup_browser_creator.cc:113: > base::CancelableClosure async_set_as_default_filter_callback; > On 2015/09/24 20:34:10, Peter Kasting wrote: > > On 2015/09/24 18:24:12, grt wrote: > > > i think CancelableClosure is overkill here since you never need to cancel a > > > callback that is in-flight (posted to a message loop). i think it would > > suffice > > > to have: > > > > > > base::Closure* g_async_set_as_default_callback = nullptr; > > > > > > and: > > > > > > SetAsyncSetAsDefaultFilter(const base::Closure& callback) { > > > DCHECK_CURRENTLY_ON(BrowserThread::UI); > > > if (callback.is_null()) { > > > delete g_async_set_as_default_callback; > > > g_async_set_as_default_callback = nullptr; > > > return; > > > } > > > if (!g_async_set_as_default_callback) > > > g_async_set_as_default_callback = new base::Closure(callback); > > > else > > > *g_async_set_as_default_callback = callback; > > > } > > > > > > and get rid of ClearAsyncSetAsDefaultFilter (callers can pass an empty > > callback > > > to clear). > > > > One downside to this is it's prone to leaks. It's not clear who's supposed to > > delete this if no one calls the API with a null pointer (e.g. we shut down > > first). > > > > One reason I liked my DefaultBrowserWorker* suggestion is that not only do I > > think it's simpler and clearer, there aren't lifetime problems, because this > > would be a non-owning pointer, so it can't leak. > > On the flipside, it means that there's a risk that the Worker goes away without > clearing the pointer here, leaving a dangling pointer. Then you'll get a crash > on a subsequent navigation to the magic URL. That one's easy; the worker can safely clear this pointer in its destructor, at least as long as all accesses to that pointer are on the same thread, which I assumed they were. (And if all accesses _are_ on the same thread, maybe we can make the worker object non-refcounted too.) > I'm less concerned about leaking since there's no risk of leaking more than one > instance, and we have a policy of leaking global-ish objects at shutdown. Leaking at shutdown should always be done via the marking types we use to indicate things are leakable, not by raw pointers. Instrumentation code will (rightfully) complain if we leak something that's not marked to be leaked.
On 2015/09/24 20:53:22, Peter Kasting wrote: > On 2015/09/24 20:39:24, grt wrote: > > > https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... > > File chrome/browser/ui/startup/startup_browser_creator.cc (right): > > > > > https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... > > chrome/browser/ui/startup/startup_browser_creator.cc:113: > > base::CancelableClosure async_set_as_default_filter_callback; > > On 2015/09/24 20:34:10, Peter Kasting wrote: > > > On 2015/09/24 18:24:12, grt wrote: > > > > i think CancelableClosure is overkill here since you never need to cancel > a > > > > callback that is in-flight (posted to a message loop). i think it would > > > suffice > > > > to have: > > > > > > > > base::Closure* g_async_set_as_default_callback = nullptr; > > > > > > > > and: > > > > > > > > SetAsyncSetAsDefaultFilter(const base::Closure& callback) { > > > > DCHECK_CURRENTLY_ON(BrowserThread::UI); > > > > if (callback.is_null()) { > > > > delete g_async_set_as_default_callback; > > > > g_async_set_as_default_callback = nullptr; > > > > return; > > > > } > > > > if (!g_async_set_as_default_callback) > > > > g_async_set_as_default_callback = new base::Closure(callback); > > > > else > > > > *g_async_set_as_default_callback = callback; > > > > } > > > > > > > > and get rid of ClearAsyncSetAsDefaultFilter (callers can pass an empty > > > callback > > > > to clear). > > > > > > One downside to this is it's prone to leaks. It's not clear who's supposed > to > > > delete this if no one calls the API with a null pointer (e.g. we shut down > > > first). > > > > > > One reason I liked my DefaultBrowserWorker* suggestion is that not only do I > > > think it's simpler and clearer, there aren't lifetime problems, because this > > > would be a non-owning pointer, so it can't leak. > > > > On the flipside, it means that there's a risk that the Worker goes away > without > > clearing the pointer here, leaving a dangling pointer. Then you'll get a crash > > on a subsequent navigation to the magic URL. > > That one's easy; the worker can safely clear this pointer in its destructor, at > least as long as all accesses to that pointer are on the same thread, which I > assumed they were. > > (And if all accesses _are_ on the same thread, maybe we can make the worker > object non-refcounted too.) The refcounting makes the Worker easy to use in a fire-and-forget setting, since its posted tasks keep it alive as it bounces from thread to thread. Maybe that can be changed, but I propose considering that a separate cleanup item. > > I'm less concerned about leaking since there's no risk of leaking more than > one > > instance, and we have a policy of leaking global-ish objects at shutdown. > > Leaking at shutdown should always be done via the marking types we use to > indicate things are leakable, not by raw pointers. Instrumentation code will > (rightfully) complain if we leak something that's not marked to be leaked. Ah, leaking the right way for tooling sgtm. Another reason I like the callback (aside from the decoupling) is that it will make a unit/browser test easier to write since the test won't need to mock out the Worker class just to see that the StartupBrowserCreator obeys its contract.
Patchset #5 (id:100001) has been deleted
Patchset #6 (id:140001) has been deleted
pkasting@: There should be no leak of g_default_browser_callback as its always cleared in the worker's destructor. grt@ and pkasting@: Please review last version with a new browser test. Thanks! https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:51: // Set as default asynchronous is not supported for all protocols. On 2015/09/24 18:24:11, grt wrote: > suggestion: > "not supported for all protocols" -> "only supported for default web browser" Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:53: permission = SET_DEFAULT_INTERACTIVE; On 2015/09/24 18:24:11, grt wrote: > nit: > if (blah) > return SET_DEFAULT_INTERACTIVE; > return permission; > or rename "permission" to "perm" and: > return (perm == SET_DEFAULT_ASYNCHRONOUS) ? SET_DEFAULT_INTERACTIVE : perm; Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:185: set_as_default_completed_ = false; On 2015/09/24 18:24:11, grt wrote: > should this handle the case where a previous StartSetAsDefault hasn't yet > completed? maybe FinalizeSetAsDefault(false) so that the old filter callback is > removed? or have InitializeSetAsDefault() be a noop if the timer already exists? Calling FinalizeSetAsDefault is better imo because we want to try again if the user repeatedly click Set default browser. The intent picker might have disappeared and we are waiting on the timer to finish. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:206: // up ressources. On 2015/09/24 18:24:11, grt wrote: > nit: resources Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:229: if (!set_as_default_completed_) { On 2015/09/24 18:24:11, grt wrote: > wdyt of set_as_default_in_progress_ instead of set_as_default_completed_ so that > it's false to begin with and true when something is happening? it makes this > conditional more clear, to boot. Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:236: set_as_default_completed_ = true; On 2015/09/24 18:24:11, grt wrote: > nit: move this up into the body of the if() Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:288: if (interactive_permitted) { On 2015/09/24 18:24:11, grt wrote: > nit: omit braces Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:295: // asynchronously through a filter in startup_browser_creator.h. On 2015/09/24 18:24:11, grt wrote: > suggestion: > // asynchronously via a filter established in InitializeSetAsDefault(). Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:45: // The caller should call SetAsyncSetAsDefaultFilterCallback to intercept the On 2015/09/24 18:24:11, grt wrote: > this comment isn't necessary. i think it's better to say that this call provides > no indication as to whether or not the operation succeeded, and that the reader > is more likely to want to use DefaultBrowserWorker, which will invoke its > observer as appropriate. Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:67: // The browser control policy disallow setting the default browser. On 2015/09/24 18:24:11, grt wrote: > even i didn't know what this meant. :-) i think a better comment would be > // The browser distribution is not permitted to be made default. Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:72: // On Windows 8, a browser can be made fault only in an interactive flow. On 2015/09/24 18:24:11, grt wrote: > fault -> default Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:264: // Function that performs the check. Subclasses are responsible for calling On 2015/09/24 18:24:11, grt wrote: > nit: mention that this is run on the FILE thread. Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:288: bool set_as_default_completed_; On 2015/09/24 18:24:11, grt wrote: > bool set_as_default_completed_ = true; > and remove this member from the ctor's initializer list in the .cc file Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:316: scoped_ptr<base::OneShotTimer<DefaultBrowserWorker>> async_timer_; On 2015/09/24 18:24:11, grt wrote: > the OneShotTimer tweak landed at r350496; please rebase. i think you can move > the #include of timer.h from here to the .cc file and forward declare > OneShotTimer now. Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:301: } On 2015/09/24 18:24:12, grt wrote: > suggestion: it would be good to know if this fails. how about > base::Process process(base::LaunchProcess(cmdline, base::LaunchOptions())); > UMA_HISTOGRAM_BOOLEAN("DefaultBrowser.AsyncSetAsDefault.Launched", > process.IsValid()); Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:312: else if (IsSetAsDefaultAsynchronous()) On 2015/09/24 18:24:11, grt wrote: > nit: remove these "else"es: > if (foo) > return blah; > if (bar) > return blorf; > return spam; Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:695: if (IsSetAsDefaultAsynchronous()) { On 2015/09/24 18:24:12, grt wrote: > how about: > if (async_timer_) { > so that there are no surprises if the user's field trial config somehow changes > while the timer is running. this also naturally protects from multiple calls. Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:113: base::CancelableClosure async_set_as_default_filter_callback; On 2015/09/24 20:39:24, grt wrote: > On 2015/09/24 20:34:10, Peter Kasting wrote: > > On 2015/09/24 18:24:12, grt wrote: > > > i think CancelableClosure is overkill here since you never need to cancel a > > > callback that is in-flight (posted to a message loop). i think it would > > suffice > > > to have: > > > > > > base::Closure* g_async_set_as_default_callback = nullptr; > > > > > > and: > > > > > > SetAsyncSetAsDefaultFilter(const base::Closure& callback) { > > > DCHECK_CURRENTLY_ON(BrowserThread::UI); > > > if (callback.is_null()) { > > > delete g_async_set_as_default_callback; > > > g_async_set_as_default_callback = nullptr; > > > return; > > > } > > > if (!g_async_set_as_default_callback) > > > g_async_set_as_default_callback = new base::Closure(callback); > > > else > > > *g_async_set_as_default_callback = callback; > > > } > > > > > > and get rid of ClearAsyncSetAsDefaultFilter (callers can pass an empty > > callback > > > to clear). > > > > One downside to this is it's prone to leaks. It's not clear who's supposed to > > delete this if no one calls the API with a null pointer (e.g. we shut down > > first). > > > > One reason I liked my DefaultBrowserWorker* suggestion is that not only do I > > think it's simpler and clearer, there aren't lifetime problems, because this > > would be a non-owning pointer, so it can't leak. > > On the flipside, it means that there's a risk that the Worker goes away without > clearing the pointer here, leaving a dangling pointer. Then you'll get a crash > on a subsequent navigation to the magic URL. > > I'm less concerned about leaking since there's no risk of leaking more than one > instance, and we have a policy of leaking global-ish objects at shutdown. Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:681: GURL url(arg); On 2015/09/23 23:01:39, Peter Kasting wrote: > Nit: I wonder if it wouldn't be simpler to just string-compare, which would > avoid the overhead (and the code) of creating a canonical URL out of these. Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:683: url == GURL(L"https://support.google.com/chrome?p=default_browser")) { On 2015/09/23 23:01:39, Peter Kasting wrote: > Why are you not using the constant here? Using it now. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.h:8: #include <map> On 2015/09/24 18:24:12, grt wrote: > unused Removed. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.h:112: static void SetAsyncSetAsDefaultFilter(base::Closure callback); On 2015/09/23 23:01:39, Peter Kasting wrote: > I find the consistent use of the phrase "AsyncSetAsDefault" everywhere > confusing, since I can barely parse it, it leads to multiple verbs in a row > ("set async set"), and it doesn't say "default what". Something like this: > > SetDefaultBrowserCallback(); > GetDefaultBrowserURL(); > > ...might be clearer. Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.h:112: static void SetAsyncSetAsDefaultFilter(base::Closure callback); On 2015/09/24 18:24:12, grt wrote: > nit: pass callbacks by constref Done. https://codereview.chromium.org/1349163008/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.h:121: // All the filters functions should be called on the UI thread. On 2015/09/23 23:01:39, Peter Kasting wrote: > Looks like all this should have been removed? Yes. Removed.
LGTM with a raft of nits, mostly comment changes https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/custom_... File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/custom_... chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:230: ShellIntegration::DefaultWebClientState state = Nit: Are the class name qualifiers really needed here? https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/custom_... chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:232: if (force_failure_) { Nit: {} unnecessary https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:32: #if !defined(OS_WIN) Nit: Define these in the same place in the .cc file as they're listed in the .h file. (This file looks like it already suffers from being out of order, might be nice to fix the existing code order first as a separate CL.) https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:49: auto permission = CanSetAsDefaultBrowser(); Nit: I think in this case using the real type is slightly more helpful: DefaultWebClientSetPermission permission = CanSetAsDefaultBrowser(); https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:52: if (permission == SET_DEFAULT_ASYNCHRONOUS) Nit: Shorter: return (permission == SET_DEFAULT_ASYNCHRONOUS) ? SET_DEFAULT_INTERACTIVE : permission; https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:208: // If set as default is currently running, CompleteSetAsDefault is invoked as Nit: Avoid passive voice ("If an attempt to set the default browser is already in progress, its result won't be posted to any observers. Manually invoke CompleteSetAsDefault() to ensure we free _______ [name of resource, or whatever].") https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:235: // FinalizeSetAsDefault will remove the last reference and delete the worker Nit: will -> would otherwise; the worker -> us https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:236: // in the middle of the function. Nit: the -> this https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:245: // Set as default completed, check again to make sure it stuck... Nit: Can you elaborate? When would the operation "not stick"? https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:308: if (interactive_permitted) { Nit: If you reverse this conditional, you can omit the {} around the now-one-line body (and it reads a little more clearly to me to not have "break;" after a "return;"). https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:368: if (interactive_permitted) { Nit: If you reverse this conditional and return; instead of break;, and also change the break; in the ASYNC case below to a return;, you can then replace all the copied PostTask() calls in this function with a single call below the switch. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:40: // Prompt the user to select the default browser by trying to open Nit: Prompts https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:42: // set Chrome as your default browser" help page. Only call this if Nit: Don't give the actual URL in this comment; just the description of it is sufficient, and more future-proof. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:63: // Windows 8 and Windows 10 introduced differents way to set the default Nit: differents way -> different ways (or "a different way"?) https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:66: // The browser distribution is not permitted to be made default. Nit: Maybe give an example of why? https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:74: // is it also asynchronous. Nit: is it -> it is Although this might be better: "On Windows 10+, the set default browser flow is both interactive and asynchronous." https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:251: // CompleteSetAsDefault, this should not be called multiple times. Nit: Here and everywhere else in the CL you give a function name in a comment, suffix with "()" to make it clear it's a function and not a type. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:260: void CompleteSetAsDefault(bool succeeded); Nit: The names "CompleteSetAsDefault()" and "FinalizeSetAsDefault()" are rather confusingly similar. Consider clearer and more distinct names, e.g. "OnSetAsDefaultAttemptComplete()" and "CleanupAfterSetAsDefaultAttempt()". https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:267: // Function that performs the check. Always called on the FILE thread. Nit: Remove "Function that"; change "Performs the check" to either "Checks whether the browser is default", or, if that's ambiguous with another function, something like "Helper function for XXX() that implements the actual default browser check". https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:272: // Sets Chrome as the default web client. Always called on the FILE thread. Nit: web client -> browser? https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:273: // |interactive_permitted| will make SetAsDefault fails if it requires Nit: fails -> fail https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:279: virtual void InitializeSetAsDefault() {} Nit: Don't give even no-op implementations of virtual functions inline; define these in the .cc file. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:307: // On windows 10+, adds the default browser callback and starts the timer Nit: Capitalize Windows (here and below) https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:314: #if defined(OS_WIN) Nit: You can move this #if up to include the two functions above, which will remove the need to define the no-op overrides in shell_integration.cc. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:256: L"%ls\\UserChoice", Nit: Can we replace the "ls" params in these with "s" and use char* for the protocols? https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:275: const std::string group_name = Nit: Init this just above its first use. At that point I might just inline this into the statement that uses it. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:314: if (IsSetAsDefaultAsynchronous()) Nit: Blank line above this, or else remove all the blank lines in this function (no obvious rule on how these blanks are being used). Shorter: return IsSetAsDefaultAsynchronous() ? SET_DEFAULT_ASYNCHRONOUS : SET_DEFAULT_INTERACTIVE; https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:657: if (IsSetAsDefaultAsynchronous()) { Nit: Reverse this + early return to avoid indenting the entire function body https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:658: // On windows 10+, there is no official way to prompt the user to set a Nit: Capitalize Windows https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:662: // 3. Windows prompt the user with "How would you link to open this?". Nit: prompt -> will prompt https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:664: // CompleteSetAsDefault is called with succeeded equals to true. Nit: Avoid passive voice, plus grammar issues. How about "If Chrome is selected, we intercept the attempt to open the URL and instead call CompleteSetAsDefault(), passing true to indicate success." https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:682: async_timer_.reset(new base::OneShotTimer()); Nit: Because this doesn't actually start the timer, and the comment above is meant to refer to all the code from here on, remove the two blank lines below this. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:684: base::TimeDelta timer_duration = base::TimeDelta::FromMinutes(2); Nit: If you move this down lower you can init as: base::TimeDelta timer_duration = seconds ? base::TimeDelta::FromSeconds(seconds) : base::TimeDelta::FromMinutes(2); Another way to do this: if (!seconds) seconds = 120; async_timer_->Start( FROM_HERE, base::TimeDelta::FromSeconds(seconds), base::Bind(&DefaultBrowserWorker::CompleteSetAsDefault, this, false)); https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:704: // Destroy timer. Nit: This comment and the next one both simply restate the code here; remove them. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:459: g_default_browser_callback = new base::Closure(callback); Nit: I might add a comment here on why this can't leak. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:110: for (int i = 0; i < tab_strip->count(); i++) { Tiny nit: There's no formal rule requiring this but I tend to prefer preincrement whenever you don't actually need a postincrement.
https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:14: #include "base/timer/timer.h" nit: remove this and instead forward decl OneShotTimer on line 20 https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:279: virtual void InitializeSetAsDefault() {} On 2015/09/25 20:52:27, Peter Kasting wrote: > Nit: Don't give even no-op implementations of virtual functions inline; define > these in the .cc file. Just curious: I see inline empty inlines for virutal methods like this extensively in content/public. Are those all incorrect, or are is it sometimes okay to do so and other times not? What's the rule? https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:676: CompleteSetAsDefault(false); i don't think this is right. when InitializeSetAsDefault() returns, StartSetAsDefault() will post a task to the FILE thread that calls SetAsDefault. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:468: if (g_default_browser_callback) { delete null is well-defined in C++, so you could remove this if() and unconditionally do the delete and assignment. otherwise, one can imagine that this code (which is only expected to be called when g_default_browser_callback isn't null) will actually do two null checks on every call. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:111: static bool SetDefaultBrowserCallback(const base::Closure& callback); please document the return value. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:117: void ProcessCommandLineAlreadyRunningDefaultProfile(base::CommandLine cmdline) { nit: pass by constref
histograms lgtm
https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:279: virtual void InitializeSetAsDefault() {} On 2015/09/28 14:31:04, grt wrote: > On 2015/09/25 20:52:27, Peter Kasting wrote: > > Nit: Don't give even no-op implementations of virtual functions inline; define > > these in the .cc file. > > Just curious: I see inline empty inlines for virutal methods like this > extensively in content/public. Are those all incorrect, or are is it sometimes > okay to do so and other times not? What's the rule? To me we shouldn't be doing that there, and it's inconsistent with the Dos And Don'ts rules we have. I know Blink explicitly and intentionally does inline many empty virtual base methods, though, as that difference came up in discussion last week.
Responding to comments. Greg take another look please. ananta@ Can you review changes in shell_integration.h, shell_integration.cc, shell_integration_win.cc, protocol_handler_registry_unittest.cc and external_protocol_handler_unittest.cc please. Thanks! https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/custom_... File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/custom_... chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:230: ShellIntegration::DefaultWebClientState state = On 2015/09/25 20:52:25, Peter Kasting wrote: > Nit: Are the class name qualifiers really needed here? Yes https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/custom_... chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:232: if (force_failure_) { On 2015/09/25 20:52:25, Peter Kasting wrote: > Nit: {} unnecessary Removed. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:32: #if !defined(OS_WIN) On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: Define these in the same place in the .cc file as they're listed in the .h > file. > > (This file looks like it already suffers from being out of order, might be nice > to fix the existing code order first as a separate CL.) Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:49: auto permission = CanSetAsDefaultBrowser(); On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: I think in this case using the real type is slightly more helpful: > > DefaultWebClientSetPermission permission = CanSetAsDefaultBrowser(); Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:52: if (permission == SET_DEFAULT_ASYNCHRONOUS) On 2015/09/25 20:52:25, Peter Kasting wrote: > Nit: Shorter: > > return (permission == SET_DEFAULT_ASYNCHRONOUS) ? > SET_DEFAULT_INTERACTIVE : permission; Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:208: // If set as default is currently running, CompleteSetAsDefault is invoked as On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: Avoid passive voice ("If an attempt to set the default browser is already > in progress, its result won't be posted to any observers. Manually invoke > CompleteSetAsDefault() to ensure we free _______ [name of resource, or > whatever].") Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:235: // FinalizeSetAsDefault will remove the last reference and delete the worker On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: will -> would otherwise; the worker -> us Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:236: // in the middle of the function. On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: the -> this Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:245: // Set as default completed, check again to make sure it stuck... On 2015/09/25 20:52:25, Peter Kasting wrote: > Nit: Can you elaborate? When would the operation "not stick"? Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:308: if (interactive_permitted) { On 2015/09/25 20:52:25, Peter Kasting wrote: > Nit: If you reverse this conditional, you can omit the {} around the > now-one-line body (and it reads a little more clearly to me to not have "break;" > after a "return;"). Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:368: if (interactive_permitted) { On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: If you reverse this conditional and return; instead of break;, and also > change the break; in the ASYNC case below to a return;, you can then replace all > the copied PostTask() calls in this function with a single call below the > switch. Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:14: #include "base/timer/timer.h" On 2015/09/28 14:31:04, grt wrote: > nit: remove this and instead forward decl OneShotTimer on line 20 Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:40: // Prompt the user to select the default browser by trying to open On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: Prompts Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:42: // set Chrome as your default browser" help page. Only call this if On 2015/09/25 20:52:27, Peter Kasting wrote: > Nit: Don't give the actual URL in this comment; just the description of it is > sufficient, and more future-proof. Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:63: // Windows 8 and Windows 10 introduced differents way to set the default On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: differents way -> different ways (or "a different way"?) Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:74: // is it also asynchronous. On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: is it -> it is > > Although this might be better: "On Windows 10+, the set default browser flow is > both interactive and asynchronous." Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:251: // CompleteSetAsDefault, this should not be called multiple times. On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: Here and everywhere else in the CL you give a function name in a comment, > suffix with "()" to make it clear it's a function and not a type. Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:260: void CompleteSetAsDefault(bool succeeded); On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: The names "CompleteSetAsDefault()" and "FinalizeSetAsDefault()" are rather > confusingly similar. Consider clearer and more distinct names, e.g. > "OnSetAsDefaultAttemptComplete()" and "CleanupAfterSetAsDefaultAttempt()". Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:267: // Function that performs the check. Always called on the FILE thread. On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: Remove "Function that"; change "Performs the check" to either "Checks > whether the browser is default", or, if that's ambiguous with another function, > something like "Helper function for XXX() that implements the actual default > browser check". Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:272: // Sets Chrome as the default web client. Always called on the FILE thread. On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: web client -> browser? Browser is related to http and https protocols and web client is used more generally in the current context. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:273: // |interactive_permitted| will make SetAsDefault fails if it requires On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: fails -> fail Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:279: virtual void InitializeSetAsDefault() {} On 2015/09/25 20:52:27, Peter Kasting wrote: > Nit: Don't give even no-op implementations of virtual functions inline; define > these in the .cc file. Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:307: // On windows 10+, adds the default browser callback and starts the timer On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: Capitalize Windows (here and below) Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:314: #if defined(OS_WIN) On 2015/09/25 20:52:26, Peter Kasting wrote: > Nit: You can move this #if up to include the two functions above, which will > remove the need to define the no-op overrides in shell_integration.cc. Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:256: L"%ls\\UserChoice", On 2015/09/25 20:52:27, Peter Kasting wrote: > Nit: Can we replace the "ls" params in these with "s" and use char* for the > protocols? No, the types must match in StringPrintf https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:275: const std::string group_name = On 2015/09/25 20:52:27, Peter Kasting wrote: > Nit: Init this just above its first use. > > At that point I might just inline this into the statement that uses it. There is a comment about that in the finch101 guide (go/finch101). I'll add it here. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:314: if (IsSetAsDefaultAsynchronous()) On 2015/09/25 20:52:27, Peter Kasting wrote: > Nit: Blank line above this, or else remove all the blank lines in this function > (no obvious rule on how these blanks are being used). > > Shorter: > > return IsSetAsDefaultAsynchronous() ? > SET_DEFAULT_ASYNCHRONOUS : SET_DEFAULT_INTERACTIVE; Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:657: if (IsSetAsDefaultAsynchronous()) { On 2015/09/25 20:52:27, Peter Kasting wrote: > Nit: Reverse this + early return to avoid indenting the entire function body Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:658: // On windows 10+, there is no official way to prompt the user to set a On 2015/09/25 20:52:27, Peter Kasting wrote: > Nit: Capitalize Windows Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:662: // 3. Windows prompt the user with "How would you link to open this?". On 2015/09/25 20:52:27, Peter Kasting wrote: > Nit: prompt -> will prompt Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:664: // CompleteSetAsDefault is called with succeeded equals to true. On 2015/09/25 20:52:27, Peter Kasting wrote: > Nit: Avoid passive voice, plus grammar issues. How about "If Chrome is > selected, we intercept the attempt to open the URL and instead call > CompleteSetAsDefault(), passing true to indicate success." Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:676: CompleteSetAsDefault(false); On 2015/09/28 14:31:04, grt wrote: > i don't think this is right. when InitializeSetAsDefault() returns, > StartSetAsDefault() will post a task to the FILE thread that calls SetAsDefault. I could set add if (set_as_default_in_progress_) before posting a task for SetAsDefault. One potentially bad side-effect of that is that the following flow won't work : 1. chrome://settings 2. Click Make Chrome the default browser. 3. (The timer starts, the callback is set, the popup is opened) 4. Dismiss the popup (the timer still runs and the callback is set) 5. Click Set as default using the butter bar at the top. Or I leave it like that and the preceding flow will work. I just need to add a comment to explain that it is intended. What do you think? https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:682: async_timer_.reset(new base::OneShotTimer()); On 2015/09/25 20:52:27, Peter Kasting wrote: > Nit: Because this doesn't actually start the timer, and the comment above is > meant to refer to all the code from here on, remove the two blank lines below > this. Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:684: base::TimeDelta timer_duration = base::TimeDelta::FromMinutes(2); On 2015/09/25 20:52:27, Peter Kasting wrote: > Nit: If you move this down lower you can init as: > > base::TimeDelta timer_duration = seconds ? > base::TimeDelta::FromSeconds(seconds) : base::TimeDelta::FromMinutes(2); > > Another way to do this: > > if (!seconds) > seconds = 120; > async_timer_->Start( > FROM_HERE, base::TimeDelta::FromSeconds(seconds), > base::Bind(&DefaultBrowserWorker::CompleteSetAsDefault, this, false)); Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:704: // Destroy timer. On 2015/09/25 20:52:27, Peter Kasting wrote: > Nit: This comment and the next one both simply restate the code here; remove > them. Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:459: g_default_browser_callback = new base::Closure(callback); On 2015/09/25 20:52:27, Peter Kasting wrote: > Nit: I might add a comment here on why this can't leak. Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.cc:468: if (g_default_browser_callback) { On 2015/09/28 14:31:04, grt wrote: > delete null is well-defined in C++, so you could remove this if() and > unconditionally do the delete and assignment. otherwise, one can imagine that > this code (which is only expected to be called when g_default_browser_callback > isn't null) will actually do two null checks on every call. Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:111: static bool SetDefaultBrowserCallback(const base::Closure& callback); On 2015/09/28 14:31:05, grt wrote: > please document the return value. Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:110: for (int i = 0; i < tab_strip->count(); i++) { On 2015/09/25 20:52:27, Peter Kasting wrote: > Tiny nit: There's no formal rule requiring this but I tend to prefer > preincrement whenever you don't actually need a postincrement. Done. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:117: void ProcessCommandLineAlreadyRunningDefaultProfile(base::CommandLine cmdline) { On 2015/09/28 14:31:05, grt wrote: > nit: pass by constref Done.
Patchset #7 (id:180001) has been deleted
https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:272: // Sets Chrome as the default web client. Always called on the FILE thread. On 2015/09/28 23:46:37, Patrick Monette wrote: > On 2015/09/25 20:52:26, Peter Kasting wrote: > > Nit: web client -> browser? > > Browser is related to http and https protocols and web client is used more > generally in the current context. There's certainly a ton of usage of "browser" in this file and the others here. I've never heard the restriction that "browser" only refers to http[s] -- not sure what you call it then when we use Chrome to look at file:, ftp:, etc. URLs. I've never heard the term "web client" outside your use. In other words, I'm not sure I'm knowledgeable enough to overrule you, but certainly none of my observation or experience supports a contention that "web client" is clearer or more consistent than "browser" here. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:256: L"%ls\\UserChoice", On 2015/09/28 23:46:37, Patrick Monette wrote: > On 2015/09/25 20:52:27, Peter Kasting wrote: > > Nit: Can we replace the "ls" params in these with "s" and use char* for the > > protocols? > > No, the types must match in StringPrintf I'm confused. I am suggesting that the types match -- that these params specify narrow strings and the protocols provided for those params be narrow strings. Are you saying the wchar_t*-format string StringPrintf() cannot accept %s arguments at all? https://codereview.chromium.org/1349163008/diff/200001/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/200001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:277: // UMA reports the correct group. Is this what we actually want? If we're letting the command line control state, then the user's field trial status is not in fact controlling behavior. It seems like calling this first would corrupt our metrics. I realize the doc you linked to shows this as an example, but at least to my layman's eyes, that code looks wrong.
rkaplow@chromium.org changed reviewers: + rkaplow@chromium.org
https://codereview.chromium.org/1349163008/diff/200001/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/200001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:277: // UMA reports the correct group. On 2015/09/28 23:57:49, Peter Kasting wrote: > Is this what we actually want? If we're letting the command line control state, > then the user's field trial status is not in fact controlling behavior. It > seems like calling this first would corrupt our metrics. I realize the doc you > linked to shows this as an example, but at least to my layman's eyes, that code > looks wrong. This is still correct although it's a bit subtle. The group that the user will get put in is a specific forcing group which is tracked separately from the usual probabalistic groups.
https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration.h:272: // Sets Chrome as the default web client. Always called on the FILE thread. On 2015/09/28 23:57:49, Peter Kasting wrote: > On 2015/09/28 23:46:37, Patrick Monette wrote: > > On 2015/09/25 20:52:26, Peter Kasting wrote: > > > Nit: web client -> browser? > > > > Browser is related to http and https protocols and web client is used more > > generally in the current context. > > There's certainly a ton of usage of "browser" in this file and the others here. > I've never heard the restriction that "browser" only refers to http[s] -- not > sure what you call it then when we use Chrome to look at file:, ftp:, etc. URLs. > I've never heard the term "web client" outside your use. > > In other words, I'm not sure I'm knowledgeable enough to overrule you, but > certainly none of my observation or experience supports a contention that "web > client" is clearer or more consistent than "browser" here. Well to me it seems like the person who designed these worker classes in this file adopted the convention of "web client" meaning the general term for supporting a certain protocol since DefaultWebClientWorker is the abstract base class for the 2 workers. DefaultBrowserWorker: Specialization for http/https protocols. DefaultProtocolClientWorker: Supports any other protocol. It is consistent in the context of this file only. https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:256: L"%ls\\UserChoice", On 2015/09/28 23:57:49, Peter Kasting wrote: > On 2015/09/28 23:46:37, Patrick Monette wrote: > > On 2015/09/25 20:52:27, Peter Kasting wrote: > > > Nit: Can we replace the "ls" params in these with "s" and use char* for the > > > protocols? > > > > No, the types must match in StringPrintf > > I'm confused. I am suggesting that the types match -- that these params specify > narrow strings and the protocols provided for those params be narrow strings. > > Are you saying the wchar_t*-format string StringPrintf() cannot accept %s > arguments at all? Yes. Apparently it is because the %s specifier is non-portable between platforms. Even if this is Windows-specific code, a DCHECK fails at runtime on IsWprintfFormatPortable() even if it compiles with no warnings. More info: https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...
Patchset #8 (id:220001) has been deleted
Thanks for the details, LGTM (again)
https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:180: if (set_as_default_in_progress_) in looking at how set_as_default_in_progress_ is used, it seems to be used for two distinct things: 1) ignore successive calls to OnSetAsDefaultAttemptComplete() after the initial one, and 2) ensure that FinalizeSetAsDefault is called when there's an outstanding call to InitializeSetAsDefault. for this latter case ("did something happen in InitializeSetAsDefault that needs to be cleaned up"), i think a few small changes would improve the interaction between DefaultWebClientWorker and DefaultBrowserWorker since, for these cases, the followon work done by OnSetAsDefaultAttemptComplete to check for defaultness isn't appropriate. 1) InitializeSetAsDefault() should return a bool indicating "i did work that needs a subsequent call to FinalizeSetAsDefault()". DefaultBrowserWorker::InitializeSetAsDefault would return false for all of its early-exit codepaths. 2) a new bool set_as_default_initialized_ should be set on line 189 based on the return value of InitializeSetAsDefault(). 3) here, ObserverDestroyed(), and ~DefaultBrowserWorker() should be changed to "if (set_as_default_initialized_) { set_as_default_initialized_ = false; FinalizeSetAsDefault(false); }" 4) OnSetAsDefaultAttemptComplete should do similarly. The idea here is to ensure that FinalizeSetAsDefault is called exactly once for every successful call to InitializeSetAsDefault. Feel free to push back if I'm missing something, or you think this is going in the wrong direction. Is there a need to clear set_as_default_in_progress_ in ObserverDestroyed()? https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:204: // OnSetAsDefaultAttemptComplete() to ensure we free the default browser nit: "free" -> "clear"? https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:287: OnSetAsDefaultAttemptComplete(false); i don't think this could ever happen. the object can't be destroyed while either async_timer_ is alive or the callback in SBC is set since each of those holds a reference to the worker. i think this is reasonable: // The worker cannot be destroyed while an operation is in progress since all // tasks, timers, and callbacks hold a reference to it. DCHECK(!set_as_default_in_progress_); wdyt? am i missing another possible destruction path? https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... chrome/browser/shell_integration.h:309: void SetAsDefaultBrowserAsynchronous(); static? https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:666: OnSetAsDefaultAttemptComplete(false); won't this result in DefaultWebClientWorker::CheckIsDefault being posted to the FILE thread before DefaultWebClientWorker::SetAsDefault? this doesn't seem desirable. how about posting a task to run OnSetAsDefaultAttemptComplete(false) on this thread instead of calling it directly to keep the ordering consistent.
Patchset #9 (id:260001) has been deleted
grt@ Please take another look! https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:180: if (set_as_default_in_progress_) On 2015/09/30 17:22:32, grt wrote: > in looking at how set_as_default_in_progress_ is used, it seems to be used for > two distinct things: 1) ignore successive calls to > OnSetAsDefaultAttemptComplete() after the initial one, and 2) ensure that > FinalizeSetAsDefault is called when there's an outstanding call to > InitializeSetAsDefault. for this latter case ("did something happen in > InitializeSetAsDefault that needs to be cleaned up"), i think a few small > changes would improve the interaction between DefaultWebClientWorker and > DefaultBrowserWorker since, for these cases, the followon work done by > OnSetAsDefaultAttemptComplete to check for defaultness isn't appropriate. > > 1) InitializeSetAsDefault() should return a bool indicating "i did work that > needs a subsequent call to FinalizeSetAsDefault()". > DefaultBrowserWorker::InitializeSetAsDefault would return false for all of its > early-exit codepaths. > > 2) a new bool set_as_default_initialized_ should be set on line 189 based on the > return value of InitializeSetAsDefault(). > > 3) here, ObserverDestroyed(), and ~DefaultBrowserWorker() should be changed to > "if (set_as_default_initialized_) { set_as_default_initialized_ = false; > FinalizeSetAsDefault(false); }" > > 4) OnSetAsDefaultAttemptComplete should do similarly. > > The idea here is to ensure that FinalizeSetAsDefault is called exactly once for > every successful call to InitializeSetAsDefault. Feel free to push back if I'm > missing something, or you think this is going in the wrong direction. > > Is there a need to clear set_as_default_in_progress_ in ObserverDestroyed()? Done. https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:204: // OnSetAsDefaultAttemptComplete() to ensure we free the default browser On 2015/09/30 17:22:32, grt wrote: > nit: "free" -> "clear"? Done. https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:287: OnSetAsDefaultAttemptComplete(false); On 2015/09/30 17:22:32, grt wrote: > i don't think this could ever happen. the object can't be destroyed while either > async_timer_ is alive or the callback in SBC is set since each of those holds a > reference to the worker. i think this is reasonable: > > // The worker cannot be destroyed while an operation is in progress since all > // tasks, timers, and callbacks hold a reference to it. > DCHECK(!set_as_default_in_progress_); > > wdyt? am i missing another possible destruction path? SGTM. Done. https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... chrome/browser/shell_integration.h:309: void SetAsDefaultBrowserAsynchronous(); On 2015/09/30 17:22:32, grt wrote: > static? Done. https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/240001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:666: OnSetAsDefaultAttemptComplete(false); On 2015/09/30 17:22:32, grt wrote: > won't this result in DefaultWebClientWorker::CheckIsDefault being posted to the > FILE thread before DefaultWebClientWorker::SetAsDefault? this doesn't seem > desirable. how about posting a task to run OnSetAsDefaultAttemptComplete(false) > on this thread instead of calling it directly to keep the ordering consistent. Yeah makes sense. Done.
final comments (i swear) https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:183: FinalizeSetAsDefault(); set_as_default_initialized_ = false; so that the variable represents the true state at all times https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:223: if (set_as_default_in_progress_ && ShouldReportAttemptResults()) since this represents the observer walking away from the worker, i think it makes sense to move this into ObserverDestroyed(). wdyt? https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:275: DCHECK(ShouldReportAttemptResults()); how about: if (!ShouldReportAttemptResults()) return; here instead of checking at each callsite? i believe modern compilers should still be able to optimize it away on non-Win platforms since the function will become "if (true) return;" https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:280: base::TimeTicks::Now() - start_time_); the times we care about are: - time to success (interaction was good) - time to abandonment (interaction failed and user turned away) - retry (interaction failed and user tried again) please make these different histograms since we'll want to compare them with one another. https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... chrome/browser/shell_integration.h:284: bool set_as_default_initialized_ = false; nit: make this a protected accessor to a private member var: bool set_as_default_initialized() const { return set_as_default_initialized_; } https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:712: base::Process process(base::LaunchProcess(cmdline, base::LaunchOptions())); nit: return base::LaunchProcess(cmdline, base::LaunchOptions()).IsValid();
Patchset #10 (id:300001) has been deleted
https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:183: FinalizeSetAsDefault(); On 2015/10/02 17:39:18, grt (very slow until Oct 12) wrote: > set_as_default_initialized_ = false; > so that the variable represents the true state at all times Done. https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:223: if (set_as_default_in_progress_ && ShouldReportAttemptResults()) On 2015/10/02 17:39:18, grt (very slow until Oct 12) wrote: > since this represents the observer walking away from the worker, i think it > makes sense to move this into ObserverDestroyed(). wdyt? Done. https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:275: DCHECK(ShouldReportAttemptResults()); On 2015/10/02 17:39:18, grt (very slow until Oct 12) wrote: > how about: > if (!ShouldReportAttemptResults()) > return; > here instead of checking at each callsite? i believe modern compilers should > still be able to optimize it away on non-Win platforms since the function will > become "if (true) return;" Done. https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:280: base::TimeTicks::Now() - start_time_); On 2015/10/02 17:39:18, grt (very slow until Oct 12) wrote: > the times we care about are: > - time to success (interaction was good) > - time to abandonment (interaction failed and user turned away) > - retry (interaction failed and user tried again) > please make these different histograms since we'll want to compare them with one > another. Done. https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... chrome/browser/shell_integration.h:284: bool set_as_default_initialized_ = false; On 2015/10/02 17:39:18, grt (very slow until Oct 12) wrote: > nit: make this a protected accessor to a private member var: > bool set_as_default_initialized() const { return > set_as_default_initialized_; } Done. https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1349163008/diff/280001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:712: base::Process process(base::LaunchProcess(cmdline, base::LaunchOptions())); On 2015/10/02 17:39:18, grt (very slow until Oct 12) wrote: > nit: > return base::LaunchProcess(cmdline, base::LaunchOptions()).IsValid(); Done.
lgtm https://codereview.chromium.org/1349163008/diff/340001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1349163008/diff/340001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:13: #include "base/strings/stringprintf.h" unused?
On 2015/09/28 18:23:41, Alexei Svitkine wrote: > histograms lgtm PTAL to changes to histograms since you last looked. Thanks.
lgtm
pmonette@chromium.org changed reviewers: + brettw@chromium.org
brettw@. I need owners lgtm, can you take a look please? Thanks! https://codereview.chromium.org/1349163008/diff/340001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1349163008/diff/340001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:13: #include "base/strings/stringprintf.h" On 2015/10/02 23:13:56, grt (very slow until Oct 12) wrote: > unused? Done.
lgtm https://codereview.chromium.org/1349163008/diff/360001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/360001/chrome/browser/shell_i... chrome/browser/shell_integration.h:59: // This is the most common case where no special permission or interaction This comment is weird to me. This doesn't cover Windows 8-10+ so it's weird to see it described as the "most common". Maybe just say Windows 7 and before?
Also CL description: I assume you mean "New prompts..."
I changed the CL title to add clarity https://codereview.chromium.org/1349163008/diff/360001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1349163008/diff/360001/chrome/browser/shell_i... chrome/browser/shell_integration.h:59: // This is the most common case where no special permission or interaction On 2015/10/05 19:58:02, brettw wrote: > This comment is weird to me. This doesn't cover Windows 8-10+ so it's weird to > see it described as the "most common". Maybe just say Windows 7 and before? Yeah but it also includes Linux and Mac. I updated the comment.
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, grt@chromium.org, asvitkine@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1349163008/#ps380001 (title: "Modified a comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349163008/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349163008/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, pkasting@chromium.org, brettw@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1349163008/#ps400001 (title: "Fixed compilation for non-Windows build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349163008/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349163008/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, pkasting@chromium.org, brettw@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1349163008/#ps420001 (title: "Fix compilation again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349163008/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349163008/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, pkasting@chromium.org, brettw@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1349163008/#ps440001 (title: "Fix compilation again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349163008/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349163008/440001
Message was sent while issue was closed.
Committed patchset #16 (id:440001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/f89ac7c739d5b0ba039af8cabac76a74ff3b2b54 Cr-Commit-Position: refs/heads/master@{#352526}
Message was sent while issue was closed.
nikunjb@google.com changed reviewers: + nikunjb@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/1349163008/diff/440001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1349163008/diff/440001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:74529: + <affected-histogram name="DefaultBrowser.AsyncSetAsDefault.Duration"/> include Other worker and LaunchFailure is skipped. Expected?
Message was sent while issue was closed.
https://codereview.chromium.org/1349163008/diff/440001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1349163008/diff/440001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:74529: + <affected-histogram name="DefaultBrowser.AsyncSetAsDefault.Duration"/> On 2015/10/26 17:45:30, nikunjb wrote: > include Other worker and LaunchFailure is skipped. Expected? Yes because the duration for those error codes are not useful. If they even happen, it will be at the beginning of setting the default browser and the duration will always be 0. |
