Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10702097:
Adds browser preferences to configure first-run flow on Windows 8. (Closed)
|
Created:
8 years, 5 months ago by motek. Modified:
8 years, 4 months ago CC:
chromium-reviews, santhoshba_google.com Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdds browser preferences to configure first-run flow on Windows 8.
- master_prefefrencs/installerdata file may now contain
{ "distribution": {
"suppress_first_run_default_browser_prompt": true
} }
to suppress the Win8+ first-run dialog that walks the
user through making Chrome the default browser.
If the dialog is suppressed, the default browser
butterbar/prompt appears on first run instead.
- master_preferences/installerdata file may now contain
{ "browser": {
"suppress_switch_to_metro_mode_on_set_default": true
} }
to suppress switching to metro mode on Win8+ immediately
after Chrome is made the default browser via the first-run
dialog.
This CL also removes the non-dialog mode of
SetAsDefaultBrowserUI and fixes a problem where the dialog
would be shown only when master preferences were not given.
BUG=135255, 135256, 135257
TEST=Verify that modifiers described above act as
advertised in setup procedure on Win8. If both prefs are
absent, the program should show a modal set-default dialog
and upon successful change restart Chrome *into the
normal first-run flow* in Metro. Note that butter-bar set
default is not expected to restart into metro.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149881
Patch Set 1 #
Total comments: 18
Patch Set 2 : Addressed reviewer's remarks. #Patch Set 3 : Merged suppress-dialog and check-default-on-first run prefs. #
Total comments: 21
Patch Set 4 : A comment fix. #
Total comments: 12
Patch Set 5 : Addressed grt@'s remarks. #
Total comments: 4
Patch Set 6 : Addressed owner's remarks. #Patch Set 7 : Changed flag name along with gab@'s suggestion. #Patch Set 8 : Reverted the most recent name change. #
Total comments: 12
Patch Set 9 : Addressed owner's remarks. #
Total comments: 4
Patch Set 10 : Renamed the new field in SBC. #Messages
Total messages: 38 (0 generated)
Hi, a minor review waiting for you guys...
i think the kDefaultBrowserFlowDialogEnabled preference should be removed and replaced with a "distribution" parameter named "suppress_first_run_default_browser_prompt" (it's long, but it's consistent with the name of the function that is suppressed) that is parsed in first_run.cc's SetupMasterPrefsFromInstallPrefs into a new member of the first_run::MasterPrefs struct and defaults to false. this localizes it to the first-run code and circumvents the need to register the preference. https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:1647: chrome::ShowFirstRunDefaultBrowserPrompt(profile_); if my suggestion about kDefaultBrowserFlowDialogEnabled is taken, this becomes: if (!master_prefs_->suppress_default_browser_prompt) chrome::ShowFirstRunDefaultBrowserPrompt(profile_); https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/first_r... File chrome/browser/first_run/first_run_win.cc (left): https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/first_r... chrome/browser/first_run/first_run_win.cc:553: chrome::ShowFirstRunDefaultBrowserPrompt(profile); nice. https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/ui/brow... File chrome/browser/ui/browser_ui_prefs.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/ui/brow... chrome/browser/ui/browser_ui_prefs.cc:98: prefs->RegisterBooleanPref(prefs::kDefaultBrowserFlowDialogEnabled, if my suggestion about kDefaultBrowserFlowDialogEnabled is taken, this goes away. https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/ui/star... File chrome/browser/ui/startup/default_browser_prompt_win.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/ui/star... chrome/browser/ui/startup/default_browser_prompt_win.cc:77: profile->GetPrefs()->GetBoolean( if my suggestion about kDefaultBrowserFlowDialogEnabled is taken, this is removed. https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/ui/webu... File chrome/browser/ui/webui/set_as_default_browser_ui.cc (left): https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/ui/webu... chrome/browser/ui/webui/set_as_default_browser_ui.cc:171: if (!delegate->IsPopupOrPanel(contents)) { was this for the in-tab variant of the dialog? https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/common/pref_nam... chrome/common/pref_names.cc:835: const char kCheckDefaultBrowserOnFirstRun[] = please make the names of the constants for these new prefs match the values. for example, kFirstRunCheckDefaultBrowser or browser.check_default_browser_on_first_run for this one. https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/common/pref_nam... chrome/common/pref_names.cc:844: // By default, setting Chrome as deafult during first run on Windows 8 will deafult -> default https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/common/pref_nam... chrome/common/pref_names.cc:846: // Chrome. This boolean flag supresses this behaviour. since "flag" has special meaning in Chrome, i suggest using a different noun here ("preference"?). https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/common/pref_nam... File chrome/common/pref_names.h (right): https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/common/pref_nam... chrome/common/pref_names.h:313: extern const char kSuppressSwitchToMetroModeOnSetDefault[]; is there a more precise definition for the desired feature than "Need to create a flag that allows Chrome Win8 to launch in desktop mode on first run, even if Chrome is selected as default browser." (issue 135255)? i find it easy to interpret that to be more broad than this one flag (for which i would have written "Need to create a flag that suppresses the transition to metro mode when Chrome is made the default browser during first run on Win8." in the feature request).
https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:1647: chrome::ShowFirstRunDefaultBrowserPrompt(profile_); On 2012/07/05 17:16:09, grt wrote: > if my suggestion about kDefaultBrowserFlowDialogEnabled is taken, this becomes: > if (!master_prefs_->suppress_default_browser_prompt) > chrome::ShowFirstRunDefaultBrowserPrompt(profile_); Done. https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/first_r... File chrome/browser/first_run/first_run_win.cc (left): https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/first_r... chrome/browser/first_run/first_run_win.cc:553: chrome::ShowFirstRunDefaultBrowserPrompt(profile); On 2012/07/05 17:16:09, grt wrote: > nice. Thank you. https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/ui/brow... File chrome/browser/ui/browser_ui_prefs.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/ui/brow... chrome/browser/ui/browser_ui_prefs.cc:98: prefs->RegisterBooleanPref(prefs::kDefaultBrowserFlowDialogEnabled, On 2012/07/05 17:16:09, grt wrote: > if my suggestion about kDefaultBrowserFlowDialogEnabled is taken, this goes > away. Done. https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/ui/star... File chrome/browser/ui/startup/default_browser_prompt_win.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/ui/star... chrome/browser/ui/startup/default_browser_prompt_win.cc:77: profile->GetPrefs()->GetBoolean( On 2012/07/05 17:16:09, grt wrote: > if my suggestion about kDefaultBrowserFlowDialogEnabled is taken, this is > removed. Done. https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/ui/webu... File chrome/browser/ui/webui/set_as_default_browser_ui.cc (left): https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/browser/ui/webu... chrome/browser/ui/webui/set_as_default_browser_ui.cc:171: if (!delegate->IsPopupOrPanel(contents)) { On 2012/07/05 17:16:09, grt wrote: > was this for the in-tab variant of the dialog? Yes. When it was a page, we would have navigated to the sync promo before closing. https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/common/pref_nam... chrome/common/pref_names.cc:835: const char kCheckDefaultBrowserOnFirstRun[] = On 2012/07/05 17:16:09, grt wrote: > please make the names of the constants for these new prefs match the values. > for example, kFirstRunCheckDefaultBrowser or > browser.check_default_browser_on_first_run for this one. Done. https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/common/pref_nam... chrome/common/pref_names.cc:844: // By default, setting Chrome as deafult during first run on Windows 8 will On 2012/07/05 17:16:09, grt wrote: > deafult -> default This has been moved to master_preferences_constants, https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/common/pref_nam... chrome/common/pref_names.cc:846: // Chrome. This boolean flag supresses this behaviour. On 2012/07/05 17:16:09, grt wrote: > since "flag" has special meaning in Chrome, i suggest using a different noun > here ("preference"?). Done. https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/common/pref_nam... File chrome/common/pref_names.h (right): https://chromiumcodereview.appspot.com/10702097/diff/1/chrome/common/pref_nam... chrome/common/pref_names.h:313: extern const char kSuppressSwitchToMetroModeOnSetDefault[]; On 2012/07/05 17:16:09, grt wrote: > is there a more precise definition for the desired feature than "Need to create > a flag that allows Chrome Win8 to launch in desktop mode on first run, even if > Chrome is selected as default browser." (issue 135255)? i find it easy to > interpret that to be more broad than this one flag (for which i would have > written "Need to create a flag that suppresses the transition to metro mode when > Chrome is made the default browser during first run on Win8." in the feature > request). I can only second what you wrote. At first, I believed that this was an idea equivalent to that setting in IE10, where you can have the browser launch in Desktop, even if it is a default. I asked the person that entered the bug specifically about that and I got a clarification, which resulted in what I coded. This has to do with some partners' downloads, where they would rather not have their software disappear from the view before the user had a chance to interact with it.
On top of previous changes (as requested by the reviewer), I have removed one of prefs (kFirstRunCheckDefaultBrowser), merging its functionality into the masterpref which controls showing the first-run dialog.
On 2012/07/24 12:38:18, motek. wrote: > On top of previous changes (as requested by the reviewer), I have removed one of > prefs (kFirstRunCheckDefaultBrowser), merging its functionality into the > masterpref which controls showing the first-run dialog. As a side-effect you can remove BUG 135257 from the list above.
Initial comments. http://codereview.chromium.org/10702097/diff/12001/chrome/browser/ui/browser_... File chrome/browser/ui/browser_ui_prefs.cc (right): http://codereview.chromium.org/10702097/diff/12001/chrome/browser/ui/browser_... chrome/browser/ui/browser_ui_prefs.cc:95: prefs->RegisterBooleanPref(prefs::kSuppressSwitchToMetroModeOnSetDefault, Double-negation in settings usually leads to confusion. I would prefer: kSwitchToMetroModeOnsetDefault and have it default to true. http://codereview.chromium.org/10702097/diff/12001/chrome/browser/ui/webui/se... File chrome/browser/ui/webui/set_as_default_browser_ui.cc (left): http://codereview.chromium.org/10702097/diff/12001/chrome/browser/ui/webui/se... chrome/browser/ui/webui/set_as_default_browser_ui.cc:171: if (!delegate->IsPopupOrPanel(contents)) { Just to make sure, we are removing this code to prevent the sync promo from showing up after the first run default browser prompt completes right? Are we sure we want to hide the sync promo in all cases? http://codereview.chromium.org/10702097/diff/12001/chrome/browser/ui/webui/se... File chrome/browser/ui/webui/set_as_default_browser_ui.cc (right): http://codereview.chromium.org/10702097/diff/12001/chrome/browser/ui/webui/se... chrome/browser/ui/webui/set_as_default_browser_ui.cc:138: if (!Profile::FromWebUI(web_ui())->GetPrefs()->GetBoolean( Remove '!' after applying suggested changes in browser_ui_prefs.cc http://codereview.chromium.org/10702097/diff/12001/chrome/browser/ui/webui/se... chrome/browser/ui/webui/set_as_default_browser_ui.cc:317: new SetAsDefaultBrowserDialogImpl(profile, browser); Use scoped_ptr, otherwise you are leaking this object. http://codereview.chromium.org/10702097/diff/12001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): http://codereview.chromium.org/10702097/diff/12001/chrome/common/pref_names.c... chrome/common/pref_names.cc:845: "browser.suppress_switch_to_metro_mode_on_set_default"; Adjust this to reflect suggested change in browser_ui_prefs. http://codereview.chromium.org/10702097/diff/12001/chrome/installer/util/mast... File chrome/installer/util/master_preferences_constants.cc (right): http://codereview.chromium.org/10702097/diff/12001/chrome/installer/util/mast... chrome/installer/util/master_preferences_constants.cc:35: "suppress_first_run_default_browser_prompt"; I think something more generic like non_interactive_first_run is better as OEMs that don't want this now won't want other types of interactive first runs we can come up with in the future and having one pref to disable them all is easier imo (and renaming a pref later is a pain as it'll be hardcoded all over the enterprise guys' scripts). http://codereview.chromium.org/10702097/diff/12001/chrome/installer/util/mast... File chrome/installer/util/master_preferences_constants.h (right): http://codereview.chromium.org/10702097/diff/12001/chrome/installer/util/mast... chrome/installer/util/master_preferences_constants.h:53: extern const char kDistroSkipFirstRunPref[]; There already seems to be a skip first run pref, I don't see why we need a new one. OEMs that don't want first run dialogs just don't want them, I can't see a case where they'd want some, but not default browser one... http://codereview.chromium.org/10702097/diff/12001/chrome/installer/util/mast... chrome/installer/util/master_preferences_constants.h:74: // shown. Relevant in Windows 8 context only. In general it's preferable to refer to Windows 8+ (instead of just Windows 8) so that comments are still valid when future versions of Windows come out.
Unfortunately, I disagree with gab@'s main suggestions. Let's discuss. https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... File chrome/browser/ui/browser_ui_prefs.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... chrome/browser/ui/browser_ui_prefs.cc:95: prefs->RegisterBooleanPref(prefs::kSuppressSwitchToMetroModeOnSetDefault, While I agree with the principle, I have to disagree with this suggestion in this specific context. This is about some default behavior which itself is triggered in some unusual circumstances (as in: most of the time nobody cares). We want to alter this behaviour for some reason, this must be a deliberate action. Thus, the default of 'change the default behaviour' would have to be true. I feel this would be as confusing (or as understandable) as what I propose. In fact, personally I feel better about 'suppress' than about 'switch to metro' set to true because it applies in very specific circumstances. On 2012/07/24 17:43:16, gab wrote: > Double-negation in settings usually leads to confusion. > > I would prefer: kSwitchToMetroModeOnsetDefault and have it default to true. https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... File chrome/browser/ui/webui/set_as_default_browser_ui.cc (left): https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... chrome/browser/ui/webui/set_as_default_browser_ui.cc:171: if (!delegate->IsPopupOrPanel(contents)) { On 2012/07/24 17:43:16, gab wrote: > Just to make sure, we are removing this code to prevent the sync promo from > showing up after the first run default browser prompt completes right? > > Are we sure we want to hide the sync promo in all cases? We are removing this code because !delegate->IsPopupOrPanel(contents) will never be true. The code for in-tab presentation has been removed and thus it is always a popup. In this situation, main window's tabs are already assembled as designed and there is no point to navigate anywhere. https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... File chrome/browser/ui/webui/set_as_default_browser_ui.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... chrome/browser/ui/webui/set_as_default_browser_ui.cc:138: if (!Profile::FromWebUI(web_ui())->GetPrefs()->GetBoolean( On 2012/07/24 17:43:16, gab wrote: > Remove '!' after applying suggested changes in browser_ui_prefs.cc As pointed out there, I'd rather keep it. https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... chrome/browser/ui/webui/set_as_default_browser_ui.cc:317: new SetAsDefaultBrowserDialogImpl(profile, browser); On 2012/07/24 17:43:16, gab wrote: > Use scoped_ptr, otherwise you are leaking this object. I don't think I am leaking it. This instance is necessary for the duration of the dialog's lifetime and is deleted in SetAsDefaultBrowserDialogImpl::OnDialogClosed (line 287), in keeping with instructions given with ui::WebDialogDelegate. https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/common/pref... File chrome/common/pref_names.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/common/pref... chrome/common/pref_names.cc:845: "browser.suppress_switch_to_metro_mode_on_set_default"; On 2012/07/24 17:43:16, gab wrote: > Adjust this to reflect suggested change in browser_ui_prefs. As explained earlier, I believe it is better the way it is. https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/installer/u... File chrome/installer/util/master_preferences_constants.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/installer/u... chrome/installer/util/master_preferences_constants.cc:35: "suppress_first_run_default_browser_prompt"; On 2012/07/24 17:43:16, gab wrote: > I think something more generic like non_interactive_first_run is better as OEMs > that don't want this now won't want other types of interactive first runs we can > come up with in the future and having one pref to disable them all is easier imo > (and renaming a pref later is a pain as it'll be hardcoded all over the > enterprise guys' scripts). This one is quite specific now (after merging it with that other flag) and influences indirectly showing (or not) of the butter bar on first run. I don't feel strongly about naming, but I am afraid it may cause extra confusion. Besides, what if someone comes up with another modal flow that will absolutely need to be controlled separately? Then we will have a pref that does this one dialog but purports to control everything... https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/installer/u... File chrome/installer/util/master_preferences_constants.h (right): https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/installer/u... chrome/installer/util/master_preferences_constants.h:53: extern const char kDistroSkipFirstRunPref[]; On 2012/07/24 17:43:16, gab wrote: > There already seems to be a skip first run pref, I don't see why we need a new > one. OEMs that don't want first run dialogs just don't want them, I can't see a > case where they'd want some, but not default browser one... Hmm... weird. I don't see anything in a diff here. Have I added anything here? I am not sure how to understand your remark. If this is a general comment then we should discuss it with PMs, their comments led me to believe that was expressly discussed with some OEMs. Due to that flag below, we can have a valid first-run experience with different levels of 'intrusiveness'. https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/installer/u... chrome/installer/util/master_preferences_constants.h:74: // shown. Relevant in Windows 8 context only. On 2012/07/24 17:43:16, gab wrote: > In general it's preferable to refer to Windows 8+ (instead of just Windows 8) so > that comments are still valid when future versions of Windows come out. Done.
lg. just some small comments. https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/installer/u... File chrome/installer/util/master_preferences_constants.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/installer/u... chrome/installer/util/master_preferences_constants.cc:35: "suppress_first_run_default_browser_prompt"; On 2012/07/25 10:08:58, motek. wrote: > On 2012/07/24 17:43:16, gab wrote: > > I think something more generic like non_interactive_first_run is better as > OEMs > > that don't want this now won't want other types of interactive first runs we > can > > come up with in the future and having one pref to disable them all is easier > imo > > (and renaming a pref later is a pain as it'll be hardcoded all over the > > enterprise guys' scripts). > This one is quite specific now (after merging it with that other flag) and > influences indirectly showing (or not) of the butter bar on first run. > I don't feel strongly about naming, but I am afraid it may cause extra > confusion. > Besides, what if someone comes up with another modal flow that will absolutely > need to be controlled separately? Then we will have a pref that does this one > dialog but purports to control everything... I think the specific name is appropriate given the complex nature of the preference. It's a specific knob for a specific feature request. https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... File chrome/browser/ui/startup/default_browser_prompt.h (right): https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... chrome/browser/ui/startup/default_browser_prompt.h:12: // Shows a prompt UI to set the default browser if necessary. please document |permit_prompt_on_first_run| here. https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator.h:90: bool get_suppressed_set_as_default_dialog() { constify the method https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator.h:92: } add a blank line between } and private: https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator.h:129: // This information needs to be passed down to code setting chrome as the this seems more clear to me: // This information is used to allow the default browser prompt to show on first-run when the dialog has been suppressed. https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:366: bool permit_prompt_on_first_run = browser_creator_ && please add a simple comment like: // The prompt is allowed on first-run when the set-as-default dialog is suppressed. https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/installer/u... File chrome/installer/util/master_preferences_constants.h (right): https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/installer/u... chrome/installer/util/master_preferences_constants.h:74: // shown. Relevant in Windows 8+ context only. please mention in this comment that the default browser butterbar will appear on first run when this is set to true.
please document in the CL description specifically what this adds/removes:
- master_prefefrencs/installerdata file may now contain { "distribution": {
"suppress_first_run_default_browser_prompt": true } } to suppress the Win8+
first-run dialog that walks the user through making Chrome the default browser.
instead, the default browser butterbar/prompt appears on first run.
- master_preferences/installerdata file may now contain { "browser": {
"suppress_switch_to_metro_mode_on_set_default": true } } to suppress switching
to metro mode on Win8+ immediately after Chrome is made the default browser via
the first-run dialog.
- anything else?
also, please put some instructions for QA in the TEST= line.
I've addressed grt@'s comments and updated the CL's description as suggested. https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... File chrome/browser/ui/startup/default_browser_prompt.h (right): https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... chrome/browser/ui/startup/default_browser_prompt.h:12: // Shows a prompt UI to set the default browser if necessary. On 2012/07/25 10:54:55, grt wrote: > please document |permit_prompt_on_first_run| here. Done. https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator.h:90: bool get_suppressed_set_as_default_dialog() { On 2012/07/25 10:54:55, grt wrote: > constify the method Done. https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator.h:92: } On 2012/07/25 10:54:55, grt wrote: > add a blank line between } and private: Done. https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator.h:129: // This information needs to be passed down to code setting chrome as the On 2012/07/25 10:54:55, grt wrote: > this seems more clear to me: > // This information is used to allow the default browser prompt to show on > first-run when the dialog has been suppressed. Done. https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:366: bool permit_prompt_on_first_run = browser_creator_ && On 2012/07/25 10:54:55, grt wrote: > please add a simple comment like: > // The prompt is allowed on first-run when the set-as-default dialog is > suppressed. Done. https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/installer/u... File chrome/installer/util/master_preferences_constants.h (right): https://chromiumcodereview.appspot.com/10702097/diff/22001/chrome/installer/u... chrome/installer/util/master_preferences_constants.h:74: // shown. Relevant in Windows 8+ context only. On 2012/07/25 10:54:56, grt wrote: > please mention in this comment that the default browser butterbar will appear on > first run when this is set to true. Done.
lgtm
Hi sky@, Could I possibly bother you for OWNER's review? This has been lgtmd for content and style by grt@. Cheers, motek@
https://chromiumcodereview.appspot.com/10702097/diff/25002/chrome/browser/ui/... File chrome/browser/ui/startup/default_browser_prompt.h (right): https://chromiumcodereview.appspot.com/10702097/diff/25002/chrome/browser/ui/... chrome/browser/ui/startup/default_browser_prompt.h:16: bool permit_prompt_on_first_run); Why is this taking a boolean indicating if does anything. Why can't the caller just check it? https://chromiumcodereview.appspot.com/10702097/diff/25002/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://chromiumcodereview.appspot.com/10702097/diff/25002/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator.h:90: bool get_suppressed_set_as_default_dialog() const { Inline accessors should match the name of the field. So, either name this 'suppressed_set_as_default_dialog'. That said, 'suppressed_set_as_default_dilaog' is a rather confusing name, how about 'is_set_as_default_dialog_suppressed'?
Comments on comments. @gideonwald, @santhoshba: PTAL at the comment you are tagged in below. Cheers, Gab https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... File chrome/browser/ui/browser_ui_prefs.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... chrome/browser/ui/browser_ui_prefs.cc:95: prefs->RegisterBooleanPref(prefs::kSuppressSwitchToMetroModeOnSetDefault, On 2012/07/25 10:08:58, motek. wrote: > While I agree with the principle, I have to disagree with this suggestion in > this specific context. This is about some default behavior which itself is > triggered in some unusual circumstances (as in: most of the time nobody cares). > We want to alter this behaviour for some reason, this must be a deliberate > action. Thus, the default of 'change the default behaviour' would have to be > true. > I feel this would be as confusing (or as understandable) as what I propose. In > fact, personally I feel better about 'suppress' than about 'switch to metro' set > to true because it applies in very specific circumstances. > > On 2012/07/24 17:43:16, gab wrote: > > Double-negation in settings usually leads to confusion. > > > > I would prefer: kSwitchToMetroModeOnsetDefault and have it default to true. > Ok, maybe I'm not clear on the "very specific circumstances", I was under the impression this was going to be a user option in settings (and even if it isn't now, someone could definitely turn it into one later..). I don't see how it being a very specific setting (i.e. barely ever turned on (on as in disabled... see how confusing it is as "on" has the opposite meaning for the pref and what it triggers...)) is a good argument against having a positive property always set to true (in fact if you look in browser_ui_prefs.cc where defaults are set, there are much more options that default to true than to false). https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... File chrome/browser/ui/webui/set_as_default_browser_ui.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... chrome/browser/ui/webui/set_as_default_browser_ui.cc:317: new SetAsDefaultBrowserDialogImpl(profile, browser); On 2012/07/25 10:08:58, motek. wrote: > On 2012/07/24 17:43:16, gab wrote: > > Use scoped_ptr, otherwise you are leaking this object. > I don't think I am leaking it. This instance is necessary for the duration of > the dialog's lifetime and is deleted in > SetAsDefaultBrowserDialogImpl::OnDialogClosed (line 287), in keeping with > instructions given with ui::WebDialogDelegate. Ah ok I see, my bad, thanks for clarifying. https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/installer/u... File chrome/installer/util/master_preferences_constants.h (right): https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/installer/u... chrome/installer/util/master_preferences_constants.h:53: extern const char kDistroSkipFirstRunPref[]; On 2012/07/25 10:08:59, motek. wrote: > On 2012/07/24 17:43:16, gab wrote: > > There already seems to be a skip first run pref, I don't see why we need a new > > one. OEMs that don't want first run dialogs just don't want them, I can't see > a > > case where they'd want some, but not default browser one... > Hmm... weird. I don't see anything in a diff here. Have I added anything here? I > am not sure how to understand your remark. If this is a general comment then we > should discuss it with PMs, their comments led me to believe that was expressly > discussed with some OEMs. > Due to that flag below, we can have a valid first-run experience with different > levels of 'intrusiveness'. Yes this a general comment as I thought we should make the other pref generic and it turns out a generic "no_first_run_dialogs" pref already exists here. I talked about it with santhosh yesterday after finding this pref. We agreed that we only need one pref to disable any kind of first run (i.e. there is no OEM that would want some part of first run, but not all.... if an OEM doesn't want first run stuff they don't want any, point final imo). Given this generic pref already exists I feel we should use it (i.e. not introduce kSuppressFirstRunDefaultBrowserPrompt). The only downside I can see is that we end up disabling our first run things before even being requested for the OEMs that already have this pref turned on (but again they'd probably just be mad that we turned some first run things back on although they have this pref and would eventually just turn on both...) @gideonwald, @santhoshba: Can you comment on this please?
On 2012/07/25 16:39:55, gab wrote: > Comments on comments. > > @gideonwald, @santhoshba: PTAL at the comment you are tagged in below. > > Cheers, > Gab > > https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... > File chrome/browser/ui/browser_ui_prefs.cc (right): > > https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... > chrome/browser/ui/browser_ui_prefs.cc:95: > prefs->RegisterBooleanPref(prefs::kSuppressSwitchToMetroModeOnSetDefault, > On 2012/07/25 10:08:58, motek. wrote: > > While I agree with the principle, I have to disagree with this suggestion in > > this specific context. This is about some default behavior which itself is > > triggered in some unusual circumstances (as in: most of the time nobody > cares). > > We want to alter this behaviour for some reason, this must be a deliberate > > action. Thus, the default of 'change the default behaviour' would have to be > > true. > > I feel this would be as confusing (or as understandable) as what I propose. In > > fact, personally I feel better about 'suppress' than about 'switch to metro' > set > > to true because it applies in very specific circumstances. > > > > On 2012/07/24 17:43:16, gab wrote: > > > Double-negation in settings usually leads to confusion. > > > > > > I would prefer: kSwitchToMetroModeOnsetDefault and have it default to true. > > > > Ok, maybe I'm not clear on the "very specific circumstances", I was under the > impression this was going to be a user option in settings (and even if it isn't > now, someone could definitely turn it into one later..). > > I don't see how it being a very specific setting (i.e. barely ever turned on (on > as in disabled... see how confusing it is as "on" has the opposite meaning for > the pref and what it triggers...)) is a good argument against having a positive > property always set to true (in fact if you look in browser_ui_prefs.cc where > defaults are set, there are much more options that default to true than to > false). > > https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... > File chrome/browser/ui/webui/set_as_default_browser_ui.cc (right): > > https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... > chrome/browser/ui/webui/set_as_default_browser_ui.cc:317: new > SetAsDefaultBrowserDialogImpl(profile, browser); > On 2012/07/25 10:08:58, motek. wrote: > > On 2012/07/24 17:43:16, gab wrote: > > > Use scoped_ptr, otherwise you are leaking this object. > > I don't think I am leaking it. This instance is necessary for the duration of > > the dialog's lifetime and is deleted in > > SetAsDefaultBrowserDialogImpl::OnDialogClosed (line 287), in keeping with > > instructions given with ui::WebDialogDelegate. > > Ah ok I see, my bad, thanks for clarifying. > > https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/installer/u... > File chrome/installer/util/master_preferences_constants.h (right): > > https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/installer/u... > chrome/installer/util/master_preferences_constants.h:53: extern const char > kDistroSkipFirstRunPref[]; > On 2012/07/25 10:08:59, motek. wrote: > > On 2012/07/24 17:43:16, gab wrote: > > > There already seems to be a skip first run pref, I don't see why we need a > new > > > one. OEMs that don't want first run dialogs just don't want them, I can't > see > > a > > > case where they'd want some, but not default browser one... > > Hmm... weird. I don't see anything in a diff here. Have I added anything here? > I > > am not sure how to understand your remark. If this is a general comment then > we > > should discuss it with PMs, their comments led me to believe that was > expressly > > discussed with some OEMs. > > Due to that flag below, we can have a valid first-run experience with > different > > levels of 'intrusiveness'. > > Yes this a general comment as I thought we should make the other pref generic > and it turns out a generic "no_first_run_dialogs" pref already exists here. > > I talked about it with santhosh yesterday after finding this pref. We agreed > that we only need one pref to disable any kind of first run (i.e. there is no > OEM that would want some part of first run, but not all.... if an OEM doesn't > want first run stuff they don't want any, point final imo). > > Given this generic pref already exists I feel we should use it (i.e. not > introduce kSuppressFirstRunDefaultBrowserPrompt). > > The only downside I can see is that we end up disabling our first run things > before even being requested for the OEMs that already have this pref turned on > (but again they'd probably just be mad that we turned some first run things back > on although they have this pref and would eventually just turn on both...) > > @gideonwald, @santhoshba: Can you comment on this please? I agree with leaving the existing one, even though it'll mean that the new first-run flow will be disabled before OEMs even need to make a conscious decision about it. Our distribution folks will bring this topic up explicitly with them anyway and suggest changing the value of the flag if it's currently disabling the FRF, so I think it's fine. @Somas, do you agree that we should preserve the existing flag to turn off the FRF rather than create a new one, even though it means that OEMs' existing preferences might turn off the FRF without them making a considered decision about it?
[+somast] (as i think gideon intended in the previous message!) On Wed, Jul 25, 2012 at 9:46 AM, <gideonwald@chromium.org> wrote: > On 2012/07/25 16:39:55, gab wrote: > >> Comments on comments. >> > > @gideonwald, @santhoshba: PTAL at the comment you are tagged in below. >> > > Cheers, >> Gab >> > > > https://chromiumcodereview.**appspot.com/10702097/diff/** > 12001/chrome/browser/ui/**browser_ui_prefs.cc<https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/browser_ui_prefs.cc> > >> File chrome/browser/ui/browser_ui_**prefs.cc (right): >> > > > https://chromiumcodereview.**appspot.com/10702097/diff/** > 12001/chrome/browser/ui/**browser_ui_prefs.cc#newcode95<https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/browser_ui_prefs.cc#newcode95> > >> chrome/browser/ui/browser_ui_**prefs.cc:95: >> prefs->RegisterBooleanPref(**prefs::**kSuppressSwitchToMetroModeOnSe** >> tDefault, >> On 2012/07/25 10:08:58, motek. wrote: >> > While I agree with the principle, I have to disagree with this >> suggestion in >> > this specific context. This is about some default behavior which itself >> is >> > triggered in some unusual circumstances (as in: most of the time nobody >> cares). >> > We want to alter this behaviour for some reason, this must be a >> deliberate >> > action. Thus, the default of 'change the default behaviour' would have >> to be >> > true. >> > I feel this would be as confusing (or as understandable) as what I >> propose. >> > In > >> > fact, personally I feel better about 'suppress' than about 'switch to >> metro' >> set >> > to true because it applies in very specific circumstances. >> > >> > On 2012/07/24 17:43:16, gab wrote: >> > > Double-negation in settings usually leads to confusion. >> > > >> > > I would prefer: kSwitchToMetroModeOnsetDefault and have it default to >> > true. > >> > >> > > Ok, maybe I'm not clear on the "very specific circumstances", I was under >> the >> impression this was going to be a user option in settings (and even if it >> > isn't > >> now, someone could definitely turn it into one later..). >> > > I don't see how it being a very specific setting (i.e. barely ever turned >> on >> > (on > >> as in disabled... see how confusing it is as "on" has the opposite >> meaning for >> the pref and what it triggers...)) is a good argument against having a >> > positive > >> property always set to true (in fact if you look in browser_ui_prefs.cc >> where >> defaults are set, there are much more options that default to true than to >> false). >> > > > https://chromiumcodereview.**appspot.com/10702097/diff/** > 12001/chrome/browser/ui/webui/**set_as_default_browser_ui.cc<https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/webui/set_as_default_browser_ui.cc> > >> File chrome/browser/ui/webui/set_**as_default_browser_ui.cc (right): >> > > > https://chromiumcodereview.**appspot.com/10702097/diff/** > 12001/chrome/browser/ui/webui/**set_as_default_browser_ui.cc#**newcode317<https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/webui/set_as_default_browser_ui.cc#newcode317> > >> chrome/browser/ui/webui/set_**as_default_browser_ui.cc:317: new >> SetAsDefaultBrowserDialogImpl(**profile, browser); >> On 2012/07/25 10:08:58, motek. wrote: >> > On 2012/07/24 17:43:16, gab wrote: >> > > Use scoped_ptr, otherwise you are leaking this object. >> > I don't think I am leaking it. This instance is necessary for the >> duration >> > of > >> > the dialog's lifetime and is deleted in >> > SetAsDefaultBrowserDialogImpl:**:OnDialogClosed (line 287), in keeping >> with >> > instructions given with ui::WebDialogDelegate. >> > > Ah ok I see, my bad, thanks for clarifying. >> > > > https://chromiumcodereview.**appspot.com/10702097/diff/** > 12001/chrome/installer/util/**master_preferences_constants.h<https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/installer/util/master_preferences_constants.h> > >> File chrome/installer/util/master_**preferences_constants.h (right): >> > > > https://chromiumcodereview.**appspot.com/10702097/diff/** > 12001/chrome/installer/util/**master_preferences_constants.**h#newcode53<https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/installer/util/master_preferences_constants.h#newcode53> > >> chrome/installer/util/master_**preferences_constants.h:53: extern const >> char >> kDistroSkipFirstRunPref[]; >> On 2012/07/25 10:08:59, motek. wrote: >> > On 2012/07/24 17:43:16, gab wrote: >> > > There already seems to be a skip first run pref, I don't see why we >> need a >> new >> > > one. OEMs that don't want first run dialogs just don't want them, I >> can't >> see >> > a >> > > case where they'd want some, but not default browser one... >> > Hmm... weird. I don't see anything in a diff here. Have I added anything >> > here? > >> I >> > am not sure how to understand your remark. If this is a general comment >> then >> we >> > should discuss it with PMs, their comments led me to believe that was >> expressly >> > discussed with some OEMs. >> > Due to that flag below, we can have a valid first-run experience with >> different >> > levels of 'intrusiveness'. >> > > Yes this a general comment as I thought we should make the other pref >> generic >> and it turns out a generic "no_first_run_dialogs" pref already exists >> here. >> > > I talked about it with santhosh yesterday after finding this pref. We >> agreed >> that we only need one pref to disable any kind of first run (i.e. there >> is no >> OEM that would want some part of first run, but not all.... if an OEM >> doesn't >> want first run stuff they don't want any, point final imo). >> > > Given this generic pref already exists I feel we should use it (i.e. not >> introduce kSuppressFirstRunDefaultBrowse**rPrompt). >> > > The only downside I can see is that we end up disabling our first run >> things >> before even being requested for the OEMs that already have this pref >> turned on >> (but again they'd probably just be mad that we turned some first run >> things >> > back > >> on although they have this pref and would eventually just turn on both...) >> > > @gideonwald, @santhoshba: Can you comment on this please? >> > > I agree with leaving the existing one, even though it'll mean that the new > first-run flow will be disabled before OEMs even need to make a conscious > decision about it. Our distribution folks will bring this topic up > explicitly > with them anyway and suggest changing the value of the flag if it's > currently > disabling the FRF, so I think it's fine. > > @Somas, do you agree that we should preserve the existing flag to turn off > the > FRF rather than create a new one, even though it means that OEMs' existing > preferences might turn off the FRF without them making a considered > decision > about it? > > https://chromiumcodereview.**appspot.com/10702097/<https://chromiumcodereview... >
> I agree with leaving the existing one, even though it'll mean that the new > first-run flow will be disabled before OEMs even need to make a conscious > decision about it. Our distribution folks will bring this topic up explicitly > with them anyway and suggest changing the value of the flag if it's currently > disabling the FRF, so I think it's fine. > > @Somas, do you agree that we should preserve the existing flag to turn off the > FRF rather than create a new one, even though it means that OEMs' existing > preferences might turn off the FRF without them making a considered decision > about it? An additional point: right now that pref in masterprefs really select between two different 'first run' ways of making Chrome the default browser: dialog vs. butter-bar. So, the original spec did call for two different first-run options. I'll need to check if that newly found pref actually 'emulates' no first-run flow (in which case we would get the entire functionality without code change). Either way, I need a clarification on that.
On 2012/07/25 17:16:15, motek. wrote: > > I agree with leaving the existing one, even though it'll mean that the new > > first-run flow will be disabled before OEMs even need to make a conscious > > decision about it. Our distribution folks will bring this topic up explicitly > > with them anyway and suggest changing the value of the flag if it's currently > > disabling the FRF, so I think it's fine. > > > > @Somas, do you agree that we should preserve the existing flag to turn off the > > FRF rather than create a new one, even though it means that OEMs' existing > > preferences might turn off the FRF without them making a considered decision > > about it? > An additional point: right now that pref in masterprefs really select between > two different 'first run' ways of making Chrome the default browser: dialog vs. > butter-bar. So, the original spec did call for two different first-run options. > I'll need to check if that newly found pref actually 'emulates' no first-run > flow (in which case we would get the entire functionality without code change). > > Either way, I need a clarification on that. Actually it's best to have a separate flag for the new FRF. The reason is that this is relevant not just to OEMs but to our bundle partners like Adobe as well. Adobe installs need to have the first run bubble by contract (controlled by skip first run ui flag) but they might not want to have the FRF (or at least be able to experiment with and without it) and just have the default browser butter bar come up on first launch instead. This applies to others like Real and Avast as well. So we'd need to be able to control these independently.
Ah ok, I see, thanks for your input Somas. We should keep the new pref then. Cheers, Gab On Wed, Jul 25, 2012 at 10:19 AM, <somast@google.com> wrote: > On 2012/07/25 17:16:15, motek. wrote: > >> > I agree with leaving the existing one, even though it'll mean that the >> new >> > first-run flow will be disabled before OEMs even need to make a >> conscious >> > decision about it. Our distribution folks will bring this topic up >> > explicitly > >> > with them anyway and suggest changing the value of the flag if it's >> > currently > >> > disabling the FRF, so I think it's fine. >> > >> > @Somas, do you agree that we should preserve the existing flag to turn >> off >> > the > >> > FRF rather than create a new one, even though it means that OEMs' >> existing >> > preferences might turn off the FRF without them making a considered >> decision >> > about it? >> An additional point: right now that pref in masterprefs really select >> between >> two different 'first run' ways of making Chrome the default browser: >> dialog >> > vs. > >> butter-bar. So, the original spec did call for two different first-run >> > options. > >> I'll need to check if that newly found pref actually 'emulates' no >> first-run >> flow (in which case we would get the entire functionality without code >> > change). > > Either way, I need a clarification on that. >> > > Actually it's best to have a separate flag for the new FRF. The reason is > that > this is relevant not just to OEMs but to our bundle partners like Adobe as > well. > Adobe installs need to have the first run bubble by contract (controlled > by skip > first run ui flag) but they might not want to have the FRF (or at least be > able > to experiment with and without it) and just have the default browser > butter bar > come up on first launch instead. This applies to others like Real and > Avast as > well. So we'd need to be able to control these independently. > > https://chromiumcodereview.**appspot.com/10702097/<https://chromiumcodereview... >
Yeah, after thinking about it I agree with Somas here. I understand the concern about not wanting to clog master prefs with too many flags, but also don't want to entangle two unrelated issues. The issue is that there could be a much more elaborate first-run process (related to sync maybe) beyond the dialog we're discussing. It makes sense to keep them separate. Thanks for bringing this to our attention though, Gab. On 2012/07/25 18:31:42, gab wrote: > Ah ok, I see, thanks for your input Somas. We should keep the new pref then. > > Cheers, > Gab > > > On Wed, Jul 25, 2012 at 10:19 AM, <mailto:somast@google.com> wrote: > > > On 2012/07/25 17:16:15, motek. wrote: > > > >> > I agree with leaving the existing one, even though it'll mean that the > >> new > >> > first-run flow will be disabled before OEMs even need to make a > >> conscious > >> > decision about it. Our distribution folks will bring this topic up > >> > > explicitly > > > >> > with them anyway and suggest changing the value of the flag if it's > >> > > currently > > > >> > disabling the FRF, so I think it's fine. > >> > > >> > @Somas, do you agree that we should preserve the existing flag to turn > >> off > >> > > the > > > >> > FRF rather than create a new one, even though it means that OEMs' > >> existing > >> > preferences might turn off the FRF without them making a considered > >> decision > >> > about it? > >> An additional point: right now that pref in masterprefs really select > >> between > >> two different 'first run' ways of making Chrome the default browser: > >> dialog > >> > > vs. > > > >> butter-bar. So, the original spec did call for two different first-run > >> > > options. > > > >> I'll need to check if that newly found pref actually 'emulates' no > >> first-run > >> flow (in which case we would get the entire functionality without code > >> > > change). > > > > Either way, I need a clarification on that. > >> > > > > Actually it's best to have a separate flag for the new FRF. The reason is > > that > > this is relevant not just to OEMs but to our bundle partners like Adobe as > > well. > > Adobe installs need to have the first run bubble by contract (controlled > > by skip > > first run ui flag) but they might not want to have the FRF (or at least be > > able > > to experiment with and without it) and just have the default browser > > butter bar > > come up on first launch instead. This applies to others like Real and > > Avast as > > well. So we'd need to be able to control these independently. > > > > > https://chromiumcodereview.**appspot.com/10702097/%3Chttps://chromiumcoderevi...> > >
OK. So it stays as proposed. I am moving on to addressing sky@'s remarks. On 2012/07/25 21:37:04, santhoshba wrote: > Yeah, after thinking about it I agree with Somas here. I understand the concern > about not wanting to clog master prefs with too many flags, but also don't want > to entangle two unrelated issues. The issue is that there could be a much more > elaborate first-run process (related to sync maybe) beyond the dialog we're > discussing. It makes sense to keep them separate. > > Thanks for bringing this to our attention though, Gab. > > On 2012/07/25 18:31:42, gab wrote: > > Ah ok, I see, thanks for your input Somas. We should keep the new pref then. > > > > Cheers, > > Gab > > > > > > On Wed, Jul 25, 2012 at 10:19 AM, <mailto:somast@google.com> wrote: > > > > > On 2012/07/25 17:16:15, motek. wrote: > > > > > >> > I agree with leaving the existing one, even though it'll mean that the > > >> new > > >> > first-run flow will be disabled before OEMs even need to make a > > >> conscious > > >> > decision about it. Our distribution folks will bring this topic up > > >> > > > explicitly > > > > > >> > with them anyway and suggest changing the value of the flag if it's > > >> > > > currently > > > > > >> > disabling the FRF, so I think it's fine. > > >> > > > >> > @Somas, do you agree that we should preserve the existing flag to turn > > >> off > > >> > > > the > > > > > >> > FRF rather than create a new one, even though it means that OEMs' > > >> existing > > >> > preferences might turn off the FRF without them making a considered > > >> decision > > >> > about it? > > >> An additional point: right now that pref in masterprefs really select > > >> between > > >> two different 'first run' ways of making Chrome the default browser: > > >> dialog > > >> > > > vs. > > > > > >> butter-bar. So, the original spec did call for two different first-run > > >> > > > options. > > > > > >> I'll need to check if that newly found pref actually 'emulates' no > > >> first-run > > >> flow (in which case we would get the entire functionality without code > > >> > > > change). > > > > > > Either way, I need a clarification on that. > > >> > > > > > > Actually it's best to have a separate flag for the new FRF. The reason is > > > that > > > this is relevant not just to OEMs but to our bundle partners like Adobe as > > > well. > > > Adobe installs need to have the first run bubble by contract (controlled > > > by skip > > > first run ui flag) but they might not want to have the FRF (or at least be > > > able > > > to experiment with and without it) and just have the default browser > > > butter bar > > > come up on first launch instead. This applies to others like Real and > > > Avast as > > > well. So we'd need to be able to control these independently. > > > > > > > > > https://chromiumcodereview.**appspot.com/10702097/%253Chttps://chromiumcodere...> > > >
On 2012/07/25 16:39:55, gab wrote: > Ok, maybe I'm not clear on the "very specific circumstances", I was under the > impression this was going to be a user option in settings (and even if it isn't > now, someone could definitely turn it into one later..). > > I don't see how it being a very specific setting (i.e. barely ever turned on (on > as in disabled... see how confusing it is as "on" has the opposite meaning for > the pref and what it triggers...)) is a good argument against having a positive > property always set to true (in fact if you look in browser_ui_prefs.cc where > defaults are set, there are much more options that default to true than to > false). Well, I see it as a matter of preference. Argument can be made both ways. Here, there is a standard designed behaviour of something and then, on a special request there is a flag to suppress it. Such convention exists and it is neither unusual nor confusing when applied. On the other hand, I believe there is very limited value in discussing such matters over and over. So I am renaming this flag as suggested.
@sky: all fixed as suggested. https://chromiumcodereview.appspot.com/10702097/diff/25002/chrome/browser/ui/... File chrome/browser/ui/startup/default_browser_prompt.h (right): https://chromiumcodereview.appspot.com/10702097/diff/25002/chrome/browser/ui/... chrome/browser/ui/startup/default_browser_prompt.h:16: bool permit_prompt_on_first_run); On 2012/07/25 16:09:57, sky wrote: > Why is this taking a boolean indicating if does anything. Why can't the caller > just check it? Well, mainly because I wanted to preserve the call structure. Although it does indeed make more sense not to call it in such situation, period. Changed as suggested. https://chromiumcodereview.appspot.com/10702097/diff/25002/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://chromiumcodereview.appspot.com/10702097/diff/25002/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator.h:90: bool get_suppressed_set_as_default_dialog() const { On 2012/07/25 16:09:57, sky wrote: > Inline accessors should match the name of the field. So, either name this > 'suppressed_set_as_default_dialog'. That said, > 'suppressed_set_as_default_dilaog' is a rather confusing name, how about > 'is_set_as_default_dialog_suppressed'? Sure. The original came about to avoid set_set setter function. Now, set_is_set is somehow better, so ... done.
Naming issues clarified offline :). https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... File chrome/browser/ui/browser_ui_prefs.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/12001/chrome/browser/ui/... chrome/browser/ui/browser_ui_prefs.cc:95: prefs->RegisterBooleanPref(prefs::kSuppressSwitchToMetroModeOnSetDefault, On 2012/07/25 16:39:55, gab wrote: > On 2012/07/25 10:08:58, motek. wrote: > > While I agree with the principle, I have to disagree with this suggestion in > > this specific context. This is about some default behavior which itself is > > triggered in some unusual circumstances (as in: most of the time nobody > cares). > > We want to alter this behaviour for some reason, this must be a deliberate > > action. Thus, the default of 'change the default behaviour' would have to be > > true. > > I feel this would be as confusing (or as understandable) as what I propose. In > > fact, personally I feel better about 'suppress' than about 'switch to metro' > set > > to true because it applies in very specific circumstances. > > > > On 2012/07/24 17:43:16, gab wrote: > > > Double-negation in settings usually leads to confusion. > > > > > > I would prefer: kSwitchToMetroModeOnsetDefault and have it default to true. > > > > Ok, maybe I'm not clear on the "very specific circumstances", I was under the > impression this was going to be a user option in settings (and even if it isn't > now, someone could definitely turn it into one later..). > > I don't see how it being a very specific setting (i.e. barely ever turned on (on > as in disabled... see how confusing it is as "on" has the opposite meaning for > the pref and what it triggers...)) is a good argument against having a positive > property always set to true (in fact if you look in browser_ui_prefs.cc where > defaults are set, there are much more options that default to true than to > false). Discussed offline, this is a browser pref, not a setting, this all makes sense now :), please do use "suppress_..."!
On 2012/07/26 14:52:08, gab wrote: > Naming issues clarified offline :). This is now reverted.
LGTM (very good in fact!) :)! (see one nit below) https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:368: bool permit_prompt_on_first_run = browser_creator_ && nit: s/bool/const bool This make it clearer to the reader (and maybe even gets inlined in the if statement below if the compiler is smart..?)
OWNERS LGTM https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator.h:132: bool is_set_as_default_dialog_suppressed_; Nit: Might be better as "default_browser_dialog_suppressed_" https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:368: bool permit_prompt_on_first_run = browser_creator_ && On 2012/07/26 17:06:18, gab wrote: > nit: s/bool/const bool > > This make it clearer to the reader (and maybe even gets inlined in the if > statement below if the compiler is smart..?) I'd probably just inline the whole thing into the conditional. https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:371: !chrome::ShowAutolaunchPrompt(profile)) { Nit: {} unnecessary https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... File chrome/browser/ui/webui/set_as_default_browser_ui.cc (left): https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... chrome/browser/ui/webui/set_as_default_browser_ui.cc:174: chrome::ShowSyncSetup(browser, SyncPromoUI::SOURCE_START_PAGE); Is deleting this correct? I didn't see anything in your description about removing the sync promo but I'm not very familiar with any of this code.
https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator.h:132: bool is_set_as_default_dialog_suppressed_; On 2012/07/27 18:48:06, Peter Kasting wrote: > Nit: Might be better as "default_browser_dialog_suppressed_" It might. But then the one suggested by sky (is_ser_as_default_dialog_supressed_) seems to be equally informative, even if wordy. I suggest we keep it. https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:368: bool permit_prompt_on_first_run = browser_creator_ && On 2012/07/27 18:48:06, Peter Kasting wrote: > On 2012/07/26 17:06:18, gab wrote: > > nit: s/bool/const bool > > > > This make it clearer to the reader (and maybe even gets inlined in the if > > statement below if the compiler is smart..?) > > I'd probably just inline the whole thing into the conditional. Done. https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:368: bool permit_prompt_on_first_run = browser_creator_ && On 2012/07/26 17:06:18, gab wrote: > nit: s/bool/const bool > > This make it clearer to the reader (and maybe even gets inlined in the if > statement below if the compiler is smart..?) Inlined explicitly as suggested by pkasting@. https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:371: !chrome::ShowAutolaunchPrompt(profile)) { On 2012/07/27 18:48:06, Peter Kasting wrote: > Nit: {} unnecessary But the convention in this file seems to be that if the condition is multi-line, braces are used. Please correct me if I am wrong here... https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... File chrome/browser/ui/webui/set_as_default_browser_ui.cc (left): https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... chrome/browser/ui/webui/set_as_default_browser_ui.cc:174: chrome::ShowSyncSetup(browser, SyncPromoUI::SOURCE_START_PAGE); On 2012/07/27 18:48:06, Peter Kasting wrote: > Is deleting this correct? I didn't see anything in your description about > removing the sync promo but I'm not very familiar with any of this code. This is OK. This does not remove sync promo, but a piece of code which would be executed only when this page is shown in-tab. This mode (non-popup) is being removed here so this became dead code. Trust me, I put it here in the first place.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/motek@chromium.org/10702097/28005
Presubmit check for 10702097-28005 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
Presubmit checks took 4.3s to calculate.
Thanks for reassuring me on the sync thing :) https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator.h:132: bool is_set_as_default_dialog_suppressed_; On 2012/07/28 17:46:44, motek. wrote: > On 2012/07/27 18:48:06, Peter Kasting wrote: > > Nit: Might be better as "default_browser_dialog_suppressed_" > It might. But then the one suggested by sky > (is_ser_as_default_dialog_supressed_) seems to be equally informative, even if > wordy. I suggest we keep it. We generally try to avoid bools starting with "is" or similar. I also think that this results in a really crazy setter name above. It's up to you though. https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/32020/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:371: !chrome::ShowAutolaunchPrompt(profile)) { On 2012/07/28 17:46:44, motek. wrote: > On 2012/07/27 18:48:06, Peter Kasting wrote: > > Nit: {} unnecessary > But the convention in this file seems to be that if the condition is multi-line, > braces are used. Please correct me if I am wrong here... I think you're probably right.
https://chromiumcodereview.appspot.com/10702097/diff/28005/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://chromiumcodereview.appspot.com/10702097/diff/28005/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator.h:86: void set_is_set_as_default_dialog_suppressed(bool new_value) { set_default_browser_dialog_suppressed the double set hurts my head. https://chromiumcodereview.appspot.com/10702097/diff/28005/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/28005/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:371: !chrome::ShowAutolaunchPrompt(profile)) { So, the only reason ShowDefaultBrowserPrompt() is called from SBCImpl is that it needs browsers to have been created for it to work, because it appends an infobar to the active browser asking for default browser status. It looks like you're using this as an opportunity to show other UI that isn't in the browser window. This makes it seem to me to be the wrong place to integrate. I think this would be better done from ChromeBrowserMain, before SBC is even invoked (i.e. before we even create any browser windows).
I'm removing myself as a reviewer since Ben is taking this one.
Renamed the variable as suggested + a response / request for clarification. https://chromiumcodereview.appspot.com/10702097/diff/28005/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://chromiumcodereview.appspot.com/10702097/diff/28005/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator.h:86: void set_is_set_as_default_dialog_suppressed(bool new_value) { On 2012/08/01 18:00:20, Ben Goodger (Google) wrote: > set_default_browser_dialog_suppressed > > the double set hurts my head. I liked this one for its comedic value. But you are right. Renamed to is_default_browser_dialog_suppressed as suggested earlier by pkasting@. https://chromiumcodereview.appspot.com/10702097/diff/28005/chrome/browser/ui/... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://chromiumcodereview.appspot.com/10702097/diff/28005/chrome/browser/ui/... chrome/browser/ui/startup/startup_browser_creator_impl.cc:371: !chrome::ShowAutolaunchPrompt(profile)) { That other UI (a webUI dlg) is spawned from chrome_browser_main.cc, right after profile has been created (it needs it). Here, I only change the exact condition of showing that bar: if that other UI was suppressed through master_prefs, we will want the bar on first run. In fact, the main reason for connecting that other UI with SBC (through this new field) is to alter this condition. I am not quite sure I am answering your comment. If I somehow missed your point, please clarify.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/motek@chromium.org/10702097/26040
Change committed as 149881 |
