Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1182513007:
Reset browser.check_default_browser to true when Chrome is the user's default browser (Closed)
|
Created:
5 years, 6 months ago by grt (UTC plus 2) Modified:
5 years, 6 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, hodie, jschuh Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReset browser.check_default_browser to true when Chrome is the user's default browser
BUG=492734
Committed: https://crrev.com/b0e8c81ed1be3462355c5d5543d2636545029d63
Cr-Commit-Position: refs/heads/master@{#334725}
Patch Set 1 #
Total comments: 8
Patch Set 2 : msw comments; fix thread bounce #
Total comments: 4
Patch Set 3 : find Profile* by path #
Messages
Total messages: 19 (6 generated)
grt@chromium.org changed reviewers: + msw@chromium.org, stevenjb@chromium.org
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1182513007/1
Please take a look: msw: chrome/browser/ui/startup/default_browser_prompt.cc stevenjb: chrome/browser/ui/webui/options/browser_options_handler.cc Thanks.
OWNER lgtm for browser_options_handler.cc
https://codereview.chromium.org/1182513007/diff/1/chrome/browser/ui/startup/d... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://codereview.chromium.org/1182513007/diff/1/chrome/browser/ui/startup/d... chrome/browser/ui/startup/default_browser_prompt.cc:205: PrefService* prefs = ProfileManager::GetLastUsedProfile()->GetPrefs(); It seems odd to change the last used profile, maybe use |profile| from ShowDefaultBrowserPrompt? (and avoid the profile_manager.h include) https://codereview.chromium.org/1182513007/diff/1/chrome/browser/ui/startup/d... chrome/browser/ui/startup/default_browser_prompt.cc:253: const std::string disable_version_string = nit: maybe avoid this whole check if show_prompt is already false? https://codereview.chromium.org/1182513007/diff/1/chrome/browser/ui/startup/d... chrome/browser/ui/startup/default_browser_prompt.cc:261: show_prompt = false; nit: show_prompt &= disable_version.Equals(Version(version_info.Version()));
https://codereview.chromium.org/1182513007/diff/1/chrome/browser/ui/startup/d... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://codereview.chromium.org/1182513007/diff/1/chrome/browser/ui/startup/d... chrome/browser/ui/startup/default_browser_prompt.cc:205: PrefService* prefs = ProfileManager::GetLastUsedProfile()->GetPrefs(); On 2015/06/16 19:23:01, msw wrote: > It seems odd to change the last used profile, maybe use |profile| from > ShowDefaultBrowserPrompt? (and avoid the profile_manager.h include) Ah, what I've done here may be unsafe for several reasons since this function is run on the FILE thread, so it probably shouldn't touch the Profile* or its prefs. Is it safe for CheckDefaultBrowserCallback to take the Profile* from ShowDefaultBrowserPrompt and bounce it back to the UI thread to update the pref? Could the Profile* have gone away during the bounce to/from the FILE thread? This is why I involved the ProfileManager: in the normal case it will be the same Profile*. If not, it will at least be some sensible Profile*. https://codereview.chromium.org/1182513007/diff/1/chrome/browser/ui/startup/d... chrome/browser/ui/startup/default_browser_prompt.cc:253: const std::string disable_version_string = On 2015/06/16 19:23:01, msw wrote: > nit: maybe avoid this whole check if show_prompt is already false? Done. https://codereview.chromium.org/1182513007/diff/1/chrome/browser/ui/startup/d... chrome/browser/ui/startup/default_browser_prompt.cc:261: show_prompt = false; On 2015/06/16 19:23:01, msw wrote: > nit: show_prompt &= disable_version.Equals(Version(version_info.Version())); I don't like that pattern since it's using a bitwise-AND on bools. It's a bit subtle (it works because of bool->int->bool conversions).
https://codereview.chromium.org/1182513007/diff/1/chrome/browser/ui/startup/d... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://codereview.chromium.org/1182513007/diff/1/chrome/browser/ui/startup/d... chrome/browser/ui/startup/default_browser_prompt.cc:205: PrefService* prefs = ProfileManager::GetLastUsedProfile()->GetPrefs(); On 2015/06/16 19:31:53, grt wrote: > On 2015/06/16 19:23:01, msw wrote: > > It seems odd to change the last used profile, maybe use |profile| from > > ShowDefaultBrowserPrompt? (and avoid the profile_manager.h include) > > Ah, what I've done here may be unsafe for several reasons since this function is > run on the FILE thread, so it probably shouldn't touch the Profile* or its > prefs. Is it safe for CheckDefaultBrowserCallback to take the Profile* from > ShowDefaultBrowserPrompt and bounce it back to the UI thread to update the pref? > Could the Profile* have gone away during the bounce to/from the FILE thread? > This is why I involved the ProfileManager: in the normal case it will be the > same Profile*. If not, it will at least be some sensible Profile*. Maybe use profile->GetPath(), like ProfileSizeTask and NukeProfileFromDisk? https://codereview.chromium.org/1182513007/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://codereview.chromium.org/1182513007/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt.cc:198: void ResetCheckDefaultBrowserPrefOnUIThread() { nit: maybe DCHECK_CURRENTLY_ON(BrowserThread::UI); ? https://codereview.chromium.org/1182513007/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt.cc:203: void CheckDefaultBrowserOnFileThread(bool show_prompt, nit: maybe DCHECK_CURRENTLY_ON(BrowserThread::FILE); ?
The CQ bit was checked by grt@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1182513007/#ps40001 (title: "find Profile* by path")
PTAL, and feel at liberty to check the CQ box if this looks good to you. Thanks for the quick review. https://codereview.chromium.org/1182513007/diff/1/chrome/browser/ui/startup/d... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://codereview.chromium.org/1182513007/diff/1/chrome/browser/ui/startup/d... chrome/browser/ui/startup/default_browser_prompt.cc:205: PrefService* prefs = ProfileManager::GetLastUsedProfile()->GetPrefs(); On 2015/06/16 20:29:49, msw wrote: > On 2015/06/16 19:31:53, grt wrote: > > On 2015/06/16 19:23:01, msw wrote: > > > It seems odd to change the last used profile, maybe use |profile| from > > > ShowDefaultBrowserPrompt? (and avoid the profile_manager.h include) > > > > Ah, what I've done here may be unsafe for several reasons since this function > is > > run on the FILE thread, so it probably shouldn't touch the Profile* or its > > prefs. Is it safe for CheckDefaultBrowserCallback to take the Profile* from > > ShowDefaultBrowserPrompt and bounce it back to the UI thread to update the > pref? > > Could the Profile* have gone away during the bounce to/from the FILE thread? > > This is why I involved the ProfileManager: in the normal case it will be the > > same Profile*. If not, it will at least be some sensible Profile*. > > Maybe use profile->GetPath(), like ProfileSizeTask and NukeProfileFromDisk? Done. https://codereview.chromium.org/1182513007/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://codereview.chromium.org/1182513007/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt.cc:198: void ResetCheckDefaultBrowserPrefOnUIThread() { On 2015/06/16 20:29:49, msw wrote: > nit: maybe DCHECK_CURRENTLY_ON(BrowserThread::UI); ? Done. https://codereview.chromium.org/1182513007/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt.cc:203: void CheckDefaultBrowserOnFileThread(bool show_prompt, On 2015/06/16 20:29:49, msw wrote: > nit: maybe DCHECK_CURRENTLY_ON(BrowserThread::FILE); ? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1182513007/40001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by grt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1182513007/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b0e8c81ed1be3462355c5d5543d2636545029d63 Cr-Commit-Position: refs/heads/master@{#334725} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
