Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2148523002:
Removed the "Don't ask again" button on the default browser prompt (Closed)
|
Created:
4 years, 5 months ago by Patrick Monette Modified:
4 years, 5 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemoved the "Don't ask again" button on the default browser prompt
The close (X) button now sets the kDefaultBrowserLastDeclined pref.
BUG=627615
Committed: https://crrev.com/7f981a48bb0217637adc1646fe0c94ba9fc2c84d
Cr-Commit-Position: refs/heads/master@{#405603}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Rebasing #
Total comments: 4
Patch Set 3 : Nits #
Total comments: 5
Patch Set 4 : Nit #Patch Set 5 : Rewrote comment #
Messages
Total messages: 25 (10 generated)
Description was changed from ========== Removed the "Don't ask again" button on the default browser prompt BUG= ========== to ========== Removed the "Don't ask again" button on the default browser prompt BUG=627615 ==========
Description was changed from ========== Removed the "Don't ask again" button on the default browser prompt BUG=627615 ========== to ========== Removed the "Don't ask again" button on the default browser prompt The close (X) button now sets the kDefaultBrowserLastDeclined pref. BUG=627615 ==========
pmonette@chromium.org changed reviewers: + grt@chromium.org
PTAL
https://codereview.chromium.org/2148523002/diff/1/chrome/browser/ui/startup/d... File chrome/browser/ui/startup/default_browser_infobar_delegate.cc (right): https://codereview.chromium.org/2148523002/diff/1/chrome/browser/ui/startup/d... chrome/browser/ui/startup/default_browser_infobar_delegate.cc:130: // This can get reached in tests where profile_ is null. does this treat 'x' and ignore equally? IIUC, 'x' should do what Cancel once did, and ignore should still be ignore. https://codereview.chromium.org/2148523002/diff/1/chrome/browser/ui/startup/d... File chrome/browser/ui/startup/default_browser_infobar_delegate.h (right): https://codereview.chromium.org/2148523002/diff/1/chrome/browser/ui/startup/d... chrome/browser/ui/startup/default_browser_infobar_delegate.h:48: IGNORE_INFO_BAR = 2, how about distinct buckets for "didn't interact" and "clicked the 'x' to dismiss"?
I've rebased my patch https://codereview.chromium.org/2148523002/diff/1/chrome/browser/ui/startup/d... File chrome/browser/ui/startup/default_browser_infobar_delegate.cc (right): https://codereview.chromium.org/2148523002/diff/1/chrome/browser/ui/startup/d... chrome/browser/ui/startup/default_browser_infobar_delegate.cc:130: // This can get reached in tests where profile_ is null. On 2016/07/14 06:31:16, grt (UTC plus 2) wrote: > does this treat 'x' and ignore equally? IIUC, 'x' should do what Cancel once > did, and ignore should still be ignore. InfoBarDismissed() is only called when the (X) button is used, not when the infobar is ignored. https://codereview.chromium.org/2148523002/diff/1/chrome/browser/ui/startup/d... File chrome/browser/ui/startup/default_browser_infobar_delegate.h (right): https://codereview.chromium.org/2148523002/diff/1/chrome/browser/ui/startup/d... chrome/browser/ui/startup/default_browser_infobar_delegate.h:48: IGNORE_INFO_BAR = 2, On 2016/07/14 06:31:16, grt (UTC plus 2) wrote: > how about distinct buckets for "didn't interact" and "clicked the 'x' to > dismiss"? Fixed this recently https://codereview.chromium.org/2139753002/
lgtm with a nit https://codereview.chromium.org/2148523002/diff/1/chrome/browser/ui/startup/d... File chrome/browser/ui/startup/default_browser_infobar_delegate.h (right): https://codereview.chromium.org/2148523002/diff/1/chrome/browser/ui/startup/d... chrome/browser/ui/startup/default_browser_infobar_delegate.h:48: IGNORE_INFO_BAR = 2, On 2016/07/14 17:42:57, Patrick Monette wrote: > On 2016/07/14 06:31:16, grt (UTC plus 2) wrote: > > how about distinct buckets for "didn't interact" and "clicked the 'x' to > > dismiss"? > > Fixed this recently > https://codereview.chromium.org/2139753002/ Damn, you're good! https://codereview.chromium.org/2148523002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_infobar_delegate.cc (right): https://codereview.chromium.org/2148523002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.cc:72: UMA_HISTOGRAM_ENUMERATION("DefaultBrowser.InfoBar.UserInteraction", nit: use content::RecordAction here as well https://codereview.chromium.org/2148523002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.cc:164: ? IDS_DEFAULT_BROWSER_INFOBAR_OK_BUTTON_LABEL_WIN_10 i think we're clear to remove this in a separate CL. do you have cycles to take care of that?
pmonette@chromium.org changed reviewers: + asvitkine@chromium.org, msw@chromium.org
msw@ PTAL at chrome/browser/ui/ asvitkine@ PTAL at actions.xml https://codereview.chromium.org/2148523002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_infobar_delegate.cc (right): https://codereview.chromium.org/2148523002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.cc:72: UMA_HISTOGRAM_ENUMERATION("DefaultBrowser.InfoBar.UserInteraction", On 2016/07/14 18:54:16, grt (UTC plus 2) wrote: > nit: use content::RecordAction here as well Done. https://codereview.chromium.org/2148523002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.cc:164: ? IDS_DEFAULT_BROWSER_INFOBAR_OK_BUTTON_LABEL_WIN_10 On 2016/07/14 18:54:16, grt (UTC plus 2) wrote: > i think we're clear to remove this in a separate CL. do you have cycles to take > care of that? Sure!
The CQ bit was checked by pmonette@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2148523002/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2148523002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:3119: The user did not interact with the default browser info bar. Does the mean the user clicked [x]? If so, I'd mention it in the description.
https://codereview.chromium.org/2148523002/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2148523002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:3119: The user did not interact with the default browser info bar. On 2016/07/14 19:48:41, Alexei Svitkine (slow) wrote: > Does the mean the user clicked [x]? If so, I'd mention it in the description. No. DefaultBrowserInfoBar_Dismiss means the user clicked [x]. This one is for when the user closed the tab and the info bar was still visible.
lgtm https://codereview.chromium.org/2148523002/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_infobar_delegate.cc (right): https://codereview.chromium.org/2148523002/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.cc:140: // This can get reached in tests where profile_ is null. optional nit: // |profile_| may be null in tests.
https://codereview.chromium.org/2148523002/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2148523002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:3119: The user did not interact with the default browser info bar. On 2016/07/14 19:51:43, Patrick Monette wrote: > On 2016/07/14 19:48:41, Alexei Svitkine (slow) wrote: > > Does the mean the user clicked [x]? If so, I'd mention it in the description. > > No. DefaultBrowserInfoBar_Dismiss means the user clicked [x]. This one is for > when the user closed the tab and the info bar was still visible. Ah, then please mention tab closing in this description.
PTAnL https://codereview.chromium.org/2148523002/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2148523002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:3119: The user did not interact with the default browser info bar. On 2016/07/14 20:32:09, Alexei Svitkine (slow) wrote: > On 2016/07/14 19:51:43, Patrick Monette wrote: > > On 2016/07/14 19:48:41, Alexei Svitkine (slow) wrote: > > > Does the mean the user clicked [x]? If so, I'd mention it in the > description. > > > > No. DefaultBrowserInfoBar_Dismiss means the user clicked [x]. This one is for > > when the user closed the tab and the info bar was still visible. > > Ah, then please mention tab closing in this description. Done.
lgtm
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2148523002/#ps80001 (title: "Rewrote comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Removed the "Don't ask again" button on the default browser prompt The close (X) button now sets the kDefaultBrowserLastDeclined pref. BUG=627615 ========== to ========== Removed the "Don't ask again" button on the default browser prompt The close (X) button now sets the kDefaultBrowserLastDeclined pref. BUG=627615 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Removed the "Don't ask again" button on the default browser prompt The close (X) button now sets the kDefaultBrowserLastDeclined pref. BUG=627615 ========== to ========== Removed the "Don't ask again" button on the default browser prompt The close (X) button now sets the kDefaultBrowserLastDeclined pref. BUG=627615 Committed: https://crrev.com/7f981a48bb0217637adc1646fe0c94ba9fc2c84d Cr-Commit-Position: refs/heads/master@{#405603} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7f981a48bb0217637adc1646fe0c94ba9fc2c84d Cr-Commit-Position: refs/heads/master@{#405603} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
