Cater for the "double close" when clicking "Learn More" on auto-dismiss extension bubbles.

Cater for the "double close" when clicking "Learn More" on auto-dismiss extension bubbles.

This fixes a regression from r459382 where clicking a "Learn More" link
on the NTP override bubble can spew out tabs. The problem is that
opening a tab may defocus the bubble, triggering a Close(). The
extension bubble client must still call close since there's no guarantee
that the defocus may trigger a close.

To fix, Close() needs more awareness of whether it is closed via being
deactivated by the user, via being deactivated by the delegate opening a
link, or via being explicitly closed by LinkClicked(). Currently it only
caters for two of these cases. We can more clearly capture the use case
simply by ensuring the delegate is not notified multiple times by
setting a flag.

BUG=712545
Review-Url: https://codereview.chromium.org/2827603005
Cr-Commit-Position: refs/heads/master@{#466228}
Committed: https://chromium.googlesource.com/chromium/src/+/d7340b19e7dd13c8379174ad50ce2c8220637aa7

[tapted, 2017-04-19 05:02:28]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 05:02:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-04-19 05:07:13]

Description was changed from

==========
Cater for the "double close" when clicking "Learn More" on auto-dismiss
extension bubbles.

This fixes a regression from r459382 where clicking a "Learn More" link
on the NTP override bubble can spew out tabs. The problem is that opening
a tab may defocus the bubble, triggering a Close(). The extension bubble
client must still call close since there's no guarantee that the defocus
may trigger a close.

BUG=712545
==========

to

==========
Cater for the "double close" when clicking "Learn More" on auto-dismiss
extension bubbles.

This fixes a regression from r459382 where clicking a "Learn More" link
on the NTP override bubble can spew out tabs. The problem is that
opening a tab may defocus the bubble, triggering a Close(). The
extension bubble client must still call close since there's no guarantee
that the defocus may trigger a close.

To fix, Close() needs more awareness of whether it is closed via being
deactivated by the user, via being deactivated by the delegate opening a
link, or via being explicitly closed by LinkClicked(). Currently it only
caters for two of these cases.

BUG=712545
==========

[tapted, 2017-04-19 05:07:13]

tapted@chromium.org changed reviewers:
+ finnur@chromium.org

[commit-bot: I haz the power, 2017-04-19 05:47:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 05:47:42]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-04-19 06:22:03]

Hi Finnur - please take a look

(you were right - there _was_ a weird case that could result in multiple calls.
So yay more test coverage. But what a trap! I'm exploring a possible fix in the
views framework as well, but it won't be mergable - crrev/2825243002 )

[Finnur, 2017-04-19 14:11:06]

finnur@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Finnur, 2017-04-19 14:11:06]

Thanks for following up on this. I would like Devlin to take a look at the
changes in toolbar_actions_bar_bubble_views.cc, his mind is fresher when it
comes to this implementation. If he OKs that file, then you can treat that as an
LG on my part.

[Devlin, 2017-04-19 15:04:22]

A few nits.  I think these aren't too much from the new code as the bubble code
being slightly tweaked to fix various bugs a few too many times, and making a
little less sense after each. :)

https://codereview.chromium.org/2827603005/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
(right):

https://codereview.chromium.org/2827603005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:104: // When
LinkClicked() calls OnBubbleClosed(), the Widget may close
This sounds a bit off to me.  Maybe "delegate_->OnBubbleClosed() will be called
by LinkClicked() since the bubble may close automatically in response..."

https://codereview.chromium.org/2827603005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:106: // a
browser is raised).
browser is raised -> tab is opened?

https://codereview.chromium.org/2827603005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:107: if
(close_reason_ != ToolbarActionsBarBubbleDelegate::CLOSE_LEARN_MORE)
close_reason_ is only set for LinkClicked(), which now (like Cancel and Accept)
calls OnBubbleClosed() directly.  Perhaps this would be better as a bool
closed_through_user_action_ or similar, and then here we check
if (!closed_through_user_action_)
  delgate_->OnBubbleClosed(CLOSE_DISMISS_DEACTIVATION);

WDYT?  Alternatively, we could set close_reason_ in the other methods to make
this slightly less weird, and then check if (close_reason_ ==
DIMISS_BY_DEACTIVATION), but that seems a little roundabout.

https://codereview.chromium.org/2827603005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:168: //
Close() can distinguish between the codepaths.
I don't follow the "so that Close() can distinguish between the codepaths" part
- Close() distinguishes based on the close_reason_.  Maybe rephrase?

[tapted, 2017-04-20 08:57:31]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 08:58:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 09:41:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 09:41:51]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-04-20 11:55:42]

PTAL

On 2017/04/19 15:04:22, Devlin wrote:
> A few nits.  I think these aren't too much from the new code as the bubble
code
> being slightly tweaked to fix various bugs a few too many times, and making a
> little less sense after each. :)

heh - hopefully I can make the dialog code slightly more friendly with some
things I'm trying in crrev.com/2825243002 . crbug.com/624560 is an open bug very
similar to crbug.com/702554 (whose fix caused this regression). There's also
crbug.com/672344 (fixed). But I might be able to convince sky@ that these fixes
are actually workarounds for a bug that can be fixed in the framework.

https://codereview.chromium.org/2827603005/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
(right):

https://codereview.chromium.org/2827603005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:104: // When
LinkClicked() calls OnBubbleClosed(), the Widget may close
On 2017/04/19 15:04:22, Devlin wrote:
> This sounds a bit off to me.  Maybe "delegate_->OnBubbleClosed() will be
called
> by LinkClicked() since the bubble may close automatically in response..."

Done.

https://codereview.chromium.org/2827603005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:106: // a
browser is raised).
On 2017/04/19 15:04:22, Devlin wrote:
> browser is raised -> tab is opened?

Done.

https://codereview.chromium.org/2827603005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:107: if
(close_reason_ != ToolbarActionsBarBubbleDelegate::CLOSE_LEARN_MORE)
On 2017/04/19 15:04:22, Devlin wrote:
> close_reason_ is only set for LinkClicked(), which now (like Cancel and
Accept)
> calls OnBubbleClosed() directly.  Perhaps this would be better as a bool
> closed_through_user_action_ or similar, and then here we check
> if (!closed_through_user_action_)
>   delgate_->OnBubbleClosed(CLOSE_DISMISS_DEACTIVATION);
> 
> WDYT?  Alternatively, we could set close_reason_ in the other methods to make
> this slightly less weird, and then check if (close_reason_ ==
> DIMISS_BY_DEACTIVATION), but that seems a little roundabout.

I went for a bool. Called it |delegate_notified_of_close_| .. I guess
technically it's "about_to_notifiy_delegate_of_close_" but that sounds a little
transient.

https://codereview.chromium.org/2827603005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:168: //
Close() can distinguish between the codepaths.
On 2017/04/19 15:04:22, Devlin wrote:
> I don't follow the "so that Close() can distinguish between the codepaths"
part
> - Close() distinguishes based on the close_reason_.  Maybe rephrase? 

I think this captures it:

  // Force the delegate action to happen here rather than in Close(). Close() is
  // called from Widget::CanClose() so attempting to open a link there can cause
  // recursion.

[Devlin, 2017-04-20 16:12:15]

lgtm

https://codereview.chromium.org/2827603005/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
(right):

https://codereview.chromium.org/2827603005/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:103: // If
delegate_->OnBubbleClosed() was called by LinkClicked(), the bubble may
We could make this a little less corner-casey (and perhaps more resilient) by
adding delegate_notified_of_close_ = true in Cancel() and Accept().  Then this
basically becomes:
// If the user took any action, the delegate will have been notified already.
Otherwise, this was dismissal due to deactivation.

That way, we also don't need to trail around the logic of Close() being called
sync or async from various actions, being called in the case of some closes but
not others, etc.

https://codereview.chromium.org/2827603005/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:168: //
Force the delegate action to happen here rather than in Close(). Close() is
Similarly here, we could just have this be the norm (since we already do it in
Cancel() and Accept())

[tapted, 2017-04-21 00:23:54]

Description was changed from

==========
Cater for the "double close" when clicking "Learn More" on auto-dismiss
extension bubbles.

This fixes a regression from r459382 where clicking a "Learn More" link
on the NTP override bubble can spew out tabs. The problem is that
opening a tab may defocus the bubble, triggering a Close(). The
extension bubble client must still call close since there's no guarantee
that the defocus may trigger a close.

To fix, Close() needs more awareness of whether it is closed via being
deactivated by the user, via being deactivated by the delegate opening a
link, or via being explicitly closed by LinkClicked(). Currently it only
caters for two of these cases.

BUG=712545
==========

to

==========
Cater for the "double close" when clicking "Learn More" on auto-dismiss
extension bubbles.

This fixes a regression from r459382 where clicking a "Learn More" link
on the NTP override bubble can spew out tabs. The problem is that
opening a tab may defocus the bubble, triggering a Close(). The
extension bubble client must still call close since there's no guarantee
that the defocus may trigger a close.

To fix, Close() needs more awareness of whether it is closed via being
deactivated by the user, via being deactivated by the delegate opening a
link, or via being explicitly closed by LinkClicked(). Currently it only
caters for two of these cases. We can more clearly capture the use case
simply by ensuring the delegate is not notified multiple times by
setting a flag.

BUG=712545
==========

[tapted, 2017-04-21 00:24:33]

https://codereview.chromium.org/2827603005/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
(right):

https://codereview.chromium.org/2827603005/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:103: // If
delegate_->OnBubbleClosed() was called by LinkClicked(), the bubble may
On 2017/04/20 16:12:15, Devlin wrote:
> We could make this a little less corner-casey (and perhaps more resilient) by
> adding delegate_notified_of_close_ = true in Cancel() and Accept().  Then this
> basically becomes:
> // If the user took any action, the delegate will have been notified already.
> Otherwise, this was dismissal due to deactivation.
> 
> That way, we also don't need to trail around the logic of Close() being called
> sync or async from various actions, being called in the case of some closes
but
> not others, etc.

Done.

https://codereview.chromium.org/2827603005/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:168: //
Force the delegate action to happen here rather than in Close(). Close() is
On 2017/04/20 16:12:15, Devlin wrote:
> Similarly here, we could just have this be the norm (since we already do it in
> Cancel() and Accept())

Done.

[tapted, 2017-04-21 00:24:38]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-04-21 00:24:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2827603005/#ps60001
(title: "resilience")

[commit-bot: I haz the power, 2017-04-21 00:24:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 00:27:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 00:27:02]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[tapted, 2017-04-21 01:02:14]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-04-21 01:02:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 02:08:19]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492736534226100,
"parent_rev": "87970b8cd534fb76c6b8a4406227617c47537d28", "commit_rev":
"d7340b19e7dd13c8379174ad50ce2c8220637aa7"}

[commit-bot: I haz the power, 2017-04-21 02:08:30]

Description was changed from

==========
Cater for the "double close" when clicking "Learn More" on auto-dismiss
extension bubbles.

This fixes a regression from r459382 where clicking a "Learn More" link
on the NTP override bubble can spew out tabs. The problem is that
opening a tab may defocus the bubble, triggering a Close(). The
extension bubble client must still call close since there's no guarantee
that the defocus may trigger a close.

To fix, Close() needs more awareness of whether it is closed via being
deactivated by the user, via being deactivated by the delegate opening a
link, or via being explicitly closed by LinkClicked(). Currently it only
caters for two of these cases. We can more clearly capture the use case
simply by ensuring the delegate is not notified multiple times by
setting a flag.

BUG=712545
==========

to

==========
Cater for the "double close" when clicking "Learn More" on auto-dismiss
extension bubbles.

This fixes a regression from r459382 where clicking a "Learn More" link
on the NTP override bubble can spew out tabs. The problem is that
opening a tab may defocus the bubble, triggering a Close(). The
extension bubble client must still call close since there's no guarantee
that the defocus may trigger a close.

To fix, Close() needs more awareness of whether it is closed via being
deactivated by the user, via being deactivated by the delegate opening a
link, or via being explicitly closed by LinkClicked(). Currently it only
caters for two of these cases. We can more clearly capture the use case
simply by ensuring the delegate is not notified multiple times by
setting a flag.

BUG=712545

Review-Url: https://codereview.chromium.org/2827603005
Cr-Commit-Position: refs/heads/master@{#466228}
Committed:
https://chromium.googlesource.com/chromium/src/+/d7340b19e7dd13c8379174ad50ce...
==========

[commit-bot: I haz the power, 2017-04-21 02:08:31]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d7340b19e7dd13c8379174ad50ce...