Add a short delay before showing the first run bubble.

Add a short delay before showing the first run bubble.

R=grt@chromium.org
BUG=223939
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193513

[macourteau, 2013-04-09 23:55:04]

Only thing I'm not sure about is the use of base::Unretained - please advise as
to whether that should be done differently.

[macourteau, 2013-04-10 05:26:10]

Looks like gab's on vacation - Greg, can you take a look at this? As I said in
my initial message, I'm not sure about my use of base::Unretained, so please
advise if I should do things differently.

There's also more background about the issue in the associated bug :)

Thanks!

[grt (UTC plus 2), 2013-04-10 15:14:06]

https://codereview.chromium.org/13958003/diff/1/chrome/browser/first_run/firs...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/13958003/diff/1/chrome/browser/first_run/firs...
chrome/browser/first_run/first_run.cc:588: base::Unretained(browser)),
this will be crashy since the browser window could go away before your task
runs. to protect against this you'll need to ensure that your callback is
neutered if the browser vanishes. i suspect you'll need a combination of a
BrowserListObserver and weak pointers. for example:

base::MessageLoop::current()->PostDelayedTask(
    FROM_HERE,
    base::Bind(&FirstRunBubbleTask::ShowFirstRunBubble,
               (new FirstRunBubbleTask(browser))->AsWeakPtr()));

class FirstRunBubbleTask : public BrowserListObserver, public
SupportsWeakPtr<FirstRunBubbleTask> {
 public:
  explicit FirstRunBubbleTask(Browser* browser) : browser_(browser) {
    BrowserList::AddObserver(this);
  }
  virtual ~FirstRunBubbleTask() {
    BrowserList::RemoveObserver(this);
  }    
  void ShowFirstRunBubble() {
    browser_->ShowFirstRunBubble();
  }
 protected:
  virtual void OnBrowserRemoved(Browser* browser) OVERRIDE {
    if (browser == browser_)
      delete this;
  }
 private:
  Browser* browser_;
};

please excuse any silliness there; it's off the top of my head. i think it'll do
the right thing. you may be able to find similar/better patterns in the codebase
with a little searching.

[macourteau, 2013-04-10 20:54:26]

https://codereview.chromium.org/13958003/diff/1/chrome/browser/first_run/firs...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/13958003/diff/1/chrome/browser/first_run/firs...
chrome/browser/first_run/first_run.cc:588: base::Unretained(browser)),
On 2013/04/10 15:14:06, grt wrote:
> this will be crashy since the browser window could go away before your task
> runs. to protect against this you'll need to ensure that your callback is
> neutered if the browser vanishes. i suspect you'll need a combination of a
> BrowserListObserver and weak pointers. for example:
> 
> base::MessageLoop::current()->PostDelayedTask(
>     FROM_HERE,
>     base::Bind(&FirstRunBubbleTask::ShowFirstRunBubble,
>                (new FirstRunBubbleTask(browser))->AsWeakPtr()));
> 
> class FirstRunBubbleTask : public BrowserListObserver, public
> SupportsWeakPtr<FirstRunBubbleTask> {
>  public:
>   explicit FirstRunBubbleTask(Browser* browser) : browser_(browser) {
>     BrowserList::AddObserver(this);
>   }
>   virtual ~FirstRunBubbleTask() {
>     BrowserList::RemoveObserver(this);
>   }    
>   void ShowFirstRunBubble() {
>     browser_->ShowFirstRunBubble();
>   }
>  protected:
>   virtual void OnBrowserRemoved(Browser* browser) OVERRIDE {
>     if (browser == browser_)
>       delete this;
>   }
>  private:
>   Browser* browser_;
> };
> 
> please excuse any silliness there; it's off the top of my head. i think it'll
do
> the right thing. you may be able to find similar/better patterns in the
codebase
> with a little searching.

Thanks for the suggestion - helped a lot. I've merged it into the
FirstRunBubbleLauncher instead of creating another class for this.

[grt (UTC plus 2), 2013-04-10 21:35:19]

lgtm. you've tested closing the browser window before the bubble appears just to
be sure? :-)

[macourteau, 2013-04-10 22:44:00]

On 2013/04/10 21:35:19, grt wrote:
> lgtm. you've tested closing the browser window before the bubble appears just
to
> be sure? :-)

Yes, and I've also double-checked to be sure :)

Thanks for your help!

[commit-bot: I haz the power, 2013-04-10 22:44:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/macourteau@chromium.org/13958003/13001

[commit-bot: I haz the power, 2013-04-11 00:14:38]

Change committed as 193513