Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1974153002:
Add an experiment for the default browser infobar on Windows 10 (Closed)
|
Created:
4 years, 7 months ago by Patrick Monette Modified:
4 years, 6 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. |
DescriptionAdd an experiment for the default browser infobar on Windows 10
Add an experiment called StickyDefaultBrowserInfobar that keeps the
infobar opens as long as the user haven't made a choice in the Windows
settings or until 2 minutes elapsed, whichever come first.
BUG=576490
Committed: https://crrev.com/fca05f3ef0b065bdbbff695a1d5906dd5b6b55ac
Cr-Commit-Position: refs/heads/master@{#397451}
Patch Set 1 : #
Total comments: 5
Patch Set 2 : Adding a test. Currently violate DEPS rules #Patch Set 3 : Specific include rule for unittests #
Total comments: 38
Patch Set 4 : pkasting comments #
Total comments: 6
Patch Set 5 : Cleaned up stuff #Patch Set 6 : Unused header #Patch Set 7 : Rebase #Patch Set 8 : Private destructor for ref counted class #Patch Set 9 : Compile the test only on desktop platforms #
Messages
Total messages: 33 (13 generated)
Patchset #1 (id:1) has been deleted
pmonette@chromium.org changed reviewers: + pkasting@chromium.org
PTAL https://codereview.chromium.org/1974153002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://codereview.chromium.org/1974153002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt.cc:7: #include <limits> This fixes a lint error.
ping!
I don't see any evidence of the 2 minute timeout thing? Is it possible to test any of this? LGTM otherwise. https://codereview.chromium.org/1974153002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://codereview.chromium.org/1974153002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt.cc:51: static const base::Feature kStickyDefaultBrowserInfobar{ Nit: Can be constexpr https://codereview.chromium.org/1974153002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt.cc:108: shell_integration::DefaultWebClientState /* state */); Nit: No /* */; put the name uncommented here and below. (Chromium does not warn on unused named params.)
On 2016/05/17 03:07:37, Peter Kasting wrote: > I don't see any evidence of the 2 minute timeout thing? Maybe it wasn't worth pointing that out. It's an internal detail of SetAsDefaultBrowserUsingSystemSettings(). > > Is it possible to test any of this? Just added some. > > LGTM otherwise. > > https://codereview.chromium.org/1974153002/diff/20001/chrome/browser/ui/start... > File chrome/browser/ui/startup/default_browser_prompt.cc (right): > > https://codereview.chromium.org/1974153002/diff/20001/chrome/browser/ui/start... > chrome/browser/ui/startup/default_browser_prompt.cc:51: static const > base::Feature kStickyDefaultBrowserInfobar{ > Nit: Can be constexpr > > https://codereview.chromium.org/1974153002/diff/20001/chrome/browser/ui/start... > chrome/browser/ui/startup/default_browser_prompt.cc:108: > shell_integration::DefaultWebClientState /* state */); > Nit: No /* */; put the name uncommented here and below. (Chromium does not warn > on unused named params.)
https://codereview.chromium.org/1974153002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://codereview.chromium.org/1974153002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt.cc:51: static const base::Feature kStickyDefaultBrowserInfobar{ On 2016/05/17 03:07:37, Peter Kasting wrote: > Nit: Can be constexpr I can't manage to make it constexpr now that I'm declaring it in the header file. https://codereview.chromium.org/1974153002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt.cc:108: shell_integration::DefaultWebClientState /* state */); On 2016/05/17 03:07:37, Peter Kasting wrote: > Nit: No /* */; put the name uncommented here and below. (Chromium does not warn > on unused named params.) Done.
https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_infobar_delegate.h (right): https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.h:20: extern const base::Feature kStickyDefaultBrowserPrompt; Is it possible to declare this as non-extern constexpr by putting the definition here? Or does that lead to ODR violations? (I'm not sure how the compiler handles constexpr aggregates in headers w.r.t. emitting definitions.) https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.h:51: void AllowExpiry() { should_expire_ = true; } Nit: If you want to inline this, name it unix_hacker()-style; otherwise deinline the body. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.h:53: // InfoBarDelegate: Nit: This class doesn't directly subclass InfoBarDelegate. These should be included atop the ConfirmInfoBarDelegate list. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.h:68: virtual scoped_refptr<shell_integration::DefaultBrowserWorker> Nit: Perhaps note "Declared virtual so tests can override." https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.h:82: // Whether the info-bar should be dismissed on the next navigation. Nit: No hyphen https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.h:83: bool should_expire_ = false; Nit: Since you initialize everything else in the initializer list, I would do this there as well. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:31: ~FakeDefaultBrowserWorker() override = default; Nit: Is this declaration necessary, or the similar one just below? https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:33: // Check if Chrome is the default browser. Nit: These two comments are misleading because they describe what the functions would do in the real executable, but what they explicitly don't do in this test. I would remove these comments, and if you want instead add a comment above this class like "An implementation of DefaultBrowserWorker that will simply invoke the 'finished' callback immediately." https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:56: const shell_integration::DefaultWebClientWorkerCallback& callback) { Nit: override https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:67: std::unique_ptr<infobars::InfoBar> CreateConfirmInfoBar( Instead of re-implementing this here, which is what leads to your DEPS issues, why not just subclass InfoBarService instead of InfoBarManager? InfoBarService is meant to be the chrome-side functional implementation of the abstract base InfoBarManager, and since you need the functionality of the real implementation (in this case, to add an instance of a concrete infobar implementation), it seems like that's the class you should be using. Then you should get CreateConfirmInfoBar for free. This will also give you an implementation of this method on non-views platforms (e.g. Mac) as well, which seems important since this test file is not views-specific. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:85: void EnableStickDefaultBrowserPrompt() { Nit: Sticky https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:98: infobars::InfoBarManager& infobar_manager() { return infobar_manager_; } Nit: We normally avoid APIs that return non-const refs; returning this by pointer seems fine https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:103: // For UI and File thread. Nit: If this test needs these, expand briefly on what uses them, since nothing refers to them directly. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:119: // on the button so the infobar still exist after calling Accept(). This comment is a bit misleading. Accept() knows nothing about buttons and clicks. Its return value simply tells the caller whether the caller should destroy the infobar after asking the infobar to execute its default action (however that execution is triggered). Perhaps this comment instead: When the sticky default browser prompt experiment is not enabled, the infobar delegate should allow itself to be destroyed immediately if the user activates the default action. Accept() should return true to indicate this. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:140: // DefaultBrowserWorker is done (i.e. after RunUntilIdle()). Nit: For parallel structure with my above suggestion: The sticky default browser prompt experiment should mean activating the infobar's default action does not result in the infobar's destruction. Instead, the infobar will ultimately be destroyed when the DefaultBrowserWorker is done. To indicate this Accept() should return false. Then above the RunUntilIdle() call below: Spin the message lop to allow the FakeDefaultBrowserWorker to complete, which should destroy the infobar. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:142: ASSERT_EQ(1U, infobar_manager().infobar_count()); Nit: EXPECT
Thanks for the suggestion Peter. Can you take a final look? sky@ Need owner's review for shell_integration.h Changed the destructor to protected so it can be subclassed. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_infobar_delegate.h (right): https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.h:20: extern const base::Feature kStickyDefaultBrowserPrompt; On 2016/06/01 00:21:12, Peter Kasting wrote: > Is it possible to declare this as non-extern constexpr by putting the definition > here? Or does that lead to ODR violations? (I'm not sure how the compiler > handles constexpr aggregates in headers w.r.t. emitting definitions.) Seems to work. Done. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.h:51: void AllowExpiry() { should_expire_ = true; } On 2016/06/01 00:21:12, Peter Kasting wrote: > Nit: If you want to inline this, name it unix_hacker()-style; otherwise deinline > the body. Moved the definition to the .cc file. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.h:53: // InfoBarDelegate: On 2016/06/01 00:21:12, Peter Kasting wrote: > Nit: This class doesn't directly subclass InfoBarDelegate. These should be > included atop the ConfirmInfoBarDelegate list. Done. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.h:68: virtual scoped_refptr<shell_integration::DefaultBrowserWorker> On 2016/06/01 00:21:11, Peter Kasting wrote: > Nit: Perhaps note "Declared virtual so tests can override." Done. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.h:82: // Whether the info-bar should be dismissed on the next navigation. On 2016/06/01 00:21:12, Peter Kasting wrote: > Nit: No hyphen Done. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.h:83: bool should_expire_ = false; On 2016/06/01 00:21:12, Peter Kasting wrote: > Nit: Since you initialize everything else in the initializer list, I would do > this there as well. Done. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:31: ~FakeDefaultBrowserWorker() override = default; On 2016/06/01 00:21:12, Peter Kasting wrote: > Nit: Is this declaration necessary, or the similar one just below? Removed. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:33: // Check if Chrome is the default browser. On 2016/06/01 00:21:12, Peter Kasting wrote: > Nit: These two comments are misleading because they describe what the functions > would do in the real executable, but what they explicitly don't do in this test. > > I would remove these comments, and if you want instead add a comment above this > class like "An implementation of DefaultBrowserWorker that will simply invoke > the 'finished' callback immediately." Done. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:56: const shell_integration::DefaultWebClientWorkerCallback& callback) { On 2016/06/01 00:21:12, Peter Kasting wrote: > Nit: override Done. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:67: std::unique_ptr<infobars::InfoBar> CreateConfirmInfoBar( On 2016/06/01 00:21:12, Peter Kasting wrote: > Instead of re-implementing this here, which is what leads to your DEPS issues, > why not just subclass InfoBarService instead of InfoBarManager? InfoBarService > is meant to be the chrome-side functional implementation of the abstract base > InfoBarManager, and since you need the functionality of the real implementation > (in this case, to add an instance of a concrete infobar implementation), it > seems like that's the class you should be using. > > Then you should get CreateConfirmInfoBar for free. This will also give you an > implementation of this method on non-views platforms (e.g. Mac) as well, which > seems important since this test file is not views-specific. Okay this works! I initially didn't want to do that because it require a Profile and a WebContents but it was simpler than I thought https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:85: void EnableStickDefaultBrowserPrompt() { On 2016/06/01 00:21:12, Peter Kasting wrote: > Nit: Sticky Done. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:98: infobars::InfoBarManager& infobar_manager() { return infobar_manager_; } On 2016/06/01 00:21:12, Peter Kasting wrote: > Nit: We normally avoid APIs that return non-const refs; returning this by > pointer seems fine Done. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:103: // For UI and File thread. On 2016/06/01 00:21:12, Peter Kasting wrote: > Nit: If this test needs these, expand briefly on what uses them, since nothing > refers to them directly. Done. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:119: // on the button so the infobar still exist after calling Accept(). On 2016/06/01 00:21:12, Peter Kasting wrote: > This comment is a bit misleading. Accept() knows nothing about buttons and > clicks. Its return value simply tells the caller whether the caller should > destroy the infobar after asking the infobar to execute its default action > (however that execution is triggered). > > Perhaps this comment instead: > > When the sticky default browser prompt experiment is not enabled, the infobar > delegate should allow itself to be destroyed immediately if the user activates > the default action. Accept() should return true to indicate this. Done. I removed the tidbit about the experiment since I felt it was out of place for the DefaultBehavior test. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:140: // DefaultBrowserWorker is done (i.e. after RunUntilIdle()). On 2016/06/01 00:21:12, Peter Kasting wrote: > Nit: For parallel structure with my above suggestion: > > The sticky default browser prompt experiment should mean activating the > infobar's default action does not result in the infobar's destruction. Instead, > the infobar will ultimately be destroyed when the DefaultBrowserWorker is done. > To indicate this Accept() should return false. > > Then above the RunUntilIdle() call below: > > Spin the message lop to allow the FakeDefaultBrowserWorker to complete, which > should destroy the infobar. Thanks for the suggestion! https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:142: ASSERT_EQ(1U, infobar_manager().infobar_count()); On 2016/06/01 00:21:12, Peter Kasting wrote: > Nit: EXPECT Done.
pmonette@chromium.org changed reviewers: + sky@chromium.org
Forgot to add sky@ to reviewers.
LGTM with one substantive comment. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_infobar_delegate.h (right): https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.h:51: void AllowExpiry() { should_expire_ = true; } On 2016/06/01 18:43:48, Patrick Monette wrote: > On 2016/06/01 00:21:12, Peter Kasting wrote: > > Nit: If you want to inline this, name it unix_hacker()-style; otherwise > deinline > > the body. > > Moved the definition to the .cc file. I don't see that... https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:33: // Check if Chrome is the default browser. On 2016/06/01 18:43:48, Patrick Monette wrote: > On 2016/06/01 00:21:12, Peter Kasting wrote: > > Nit: These two comments are misleading because they describe what the > functions > > would do in the real executable, but what they explicitly don't do in this > test. > > > > I would remove these comments, and if you want instead add a comment above > this > > class like "An implementation of DefaultBrowserWorker that will simply invoke > > the 'finished' callback immediately." > > Done. New comment added, but these comments not yet removed. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:119: // on the button so the infobar still exist after calling Accept(). On 2016/06/01 18:43:48, Patrick Monette wrote: > On 2016/06/01 00:21:12, Peter Kasting wrote: > > This comment is a bit misleading. Accept() knows nothing about buttons and > > clicks. Its return value simply tells the caller whether the caller should > > destroy the infobar after asking the infobar to execute its default action > > (however that execution is triggered). > > > > Perhaps this comment instead: > > > > When the sticky default browser prompt experiment is not enabled, the infobar > > delegate should allow itself to be destroyed immediately if the user activates > > the default action. Accept() should return true to indicate this. > > Done. I removed the tidbit about the experiment since I felt it was out of place > for the DefaultBehavior test. The main reason I liked that was that, until I realized the whole point here was to contrast with what happens in that experiment, I was writing a comment to tell you to just remove this test because you're not really testing anything meaningful. When I realized that this test ensures the experimental behavior is not only "don't dismiss" but that it's explicitly different than the default I figured it was worth keeping with a note about why. It's not all that unclear, or a big deal, so up to you. https://codereview.chromium.org/1974153002/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_infobar_delegate.cc (right): https://codereview.chromium.org/1974153002/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.cc:25: "StickyDefaultBrowserPrompt", base::FEATURE_DISABLED_BY_DEFAULT};*/ Remove https://codereview.chromium.org/1974153002/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/1974153002/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:68: infobar_service_ = InfoBarService::FromWebContents(web_contents_); I think this class leaks |web_contents_| and |infobar_service_|. If they are safe to delete directly make them unique_ptrs, otherwise add a destructor that cleans up. https://codereview.chromium.org/1974153002/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:98: InfoBarService* infobar_service_; Heh, I was suggesting subclassing this, but actually using it directly without subclassing it is better -- didn't think that'd work!
Peter is an owner of c/b/ui, so I only looked at the reset of the files. Those LGTM
Thanks! Sorry I rushed the last patch to get it in before a meeting and didn't review it before sending it for review. Won't happen again! https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_infobar_delegate.h (right): https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.h:51: void AllowExpiry() { should_expire_ = true; } On 2016/06/01 19:32:10, Peter Kasting wrote: > On 2016/06/01 18:43:48, Patrick Monette wrote: > > On 2016/06/01 00:21:12, Peter Kasting wrote: > > > Nit: If you want to inline this, name it unix_hacker()-style; otherwise > > deinline > > > the body. > > > > Moved the definition to the .cc file. > > I don't see that... Now moved. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:33: // Check if Chrome is the default browser. On 2016/06/01 19:32:10, Peter Kasting wrote: > On 2016/06/01 18:43:48, Patrick Monette wrote: > > On 2016/06/01 00:21:12, Peter Kasting wrote: > > > Nit: These two comments are misleading because they describe what the > > functions > > > would do in the real executable, but what they explicitly don't do in this > > test. > > > > > > I would remove these comments, and if you want instead add a comment above > > this > > > class like "An implementation of DefaultBrowserWorker that will simply > invoke > > > the 'finished' callback immediately." > > > > Done. > > New comment added, but these comments not yet removed. My bad. https://codereview.chromium.org/1974153002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:119: // on the button so the infobar still exist after calling Accept(). On 2016/06/01 19:32:10, Peter Kasting wrote: > On 2016/06/01 18:43:48, Patrick Monette wrote: > > On 2016/06/01 00:21:12, Peter Kasting wrote: > > > This comment is a bit misleading. Accept() knows nothing about buttons and > > > clicks. Its return value simply tells the caller whether the caller should > > > destroy the infobar after asking the infobar to execute its default action > > > (however that execution is triggered). > > > > > > Perhaps this comment instead: > > > > > > When the sticky default browser prompt experiment is not enabled, the > infobar > > > delegate should allow itself to be destroyed immediately if the user > activates > > > the default action. Accept() should return true to indicate this. > > > > Done. I removed the tidbit about the experiment since I felt it was out of > place > > for the DefaultBehavior test. > > The main reason I liked that was that, until I realized the whole point here was > to contrast with what happens in that experiment, I was writing a comment to > tell you to just remove this test because you're not really testing anything > meaningful. When I realized that this test ensures the experimental behavior is > not only "don't dismiss" but that it's explicitly different than the default I > figured it was worth keeping with a note about why. > > It's not all that unclear, or a big deal, so up to you. Alright I'll add it then. https://codereview.chromium.org/1974153002/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_infobar_delegate.cc (right): https://codereview.chromium.org/1974153002/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate.cc:25: "StickyDefaultBrowserPrompt", base::FEATURE_DISABLED_BY_DEFAULT};*/ On 2016/06/01 19:32:10, Peter Kasting wrote: > Remove Done. https://codereview.chromium.org/1974153002/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/1974153002/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:68: infobar_service_ = InfoBarService::FromWebContents(web_contents_); On 2016/06/01 19:32:10, Peter Kasting wrote: > I think this class leaks |web_contents_| and |infobar_service_|. If they are > safe to delete directly make them unique_ptrs, otherwise add a destructor that > cleans up. The TestWebContentsFactory's documentation is pretty clear about keeping ownership of the WebContents that it creates. It will get cleaned up in the factory's destructor. And since the InfoBarService derives WebContentsUserData, it is scoped to the WebContents. https://codereview.chromium.org/1974153002/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc:98: InfoBarService* infobar_service_; On 2016/06/01 19:32:10, Peter Kasting wrote: > Heh, I was suggesting subclassing this, but actually using it directly without > subclassing it is better -- didn't think that'd work! I was pleasantly surprised as well!
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, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1974153002/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974153002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974153002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 pkasting@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1974153002/#ps160001 (title: "Private destructor for ref counted class")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974153002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974153002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1974153002/#ps180001 (title: "Compile the test only on desktop platforms")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974153002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974153002/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add an experiment for the default browser infobar on Windows 10 Add an experiment called StickyDefaultBrowserInfobar that keeps the infobar opens as long as the user haven't made a choice in the Windows settings or until 2 minutes elapsed, whichever come first. BUG=576490 ========== to ========== Add an experiment for the default browser infobar on Windows 10 Add an experiment called StickyDefaultBrowserInfobar that keeps the infobar opens as long as the user haven't made a choice in the Windows settings or until 2 minutes elapsed, whichever come first. BUG=576490 Committed: https://crrev.com/fca05f3ef0b065bdbbff695a1d5906dd5b6b55ac Cr-Commit-Position: refs/heads/master@{#397451} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/fca05f3ef0b065bdbbff695a1d5906dd5b6b55ac Cr-Commit-Position: refs/heads/master@{#397451}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
you forgot to add your new test to the gn build. please fix? :-)
Message was sent while issue was closed.
On 2016/06/03 19:35:27, Nico wrote: > you forgot to add your new test to the gn build. please fix? :-) Uhh my bad. I changed it from a gyp variable (shared with GN) to a sources list in my last patch. Will fix! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
