[Extensions] Don't count bubble dismissal from focus loss as acknowledgment

[Extensions] Don't count bubble dismissal from focus loss as acknowledgment

Currently, if an extension message bubble is shown, and then it is dismissed
because it loses focus, we treat it the same as the user clicking the dismiss
button - which serves as acknowledging the extension.  We could ignore focus
loss, but this makes for very noisy, awkward bubbles.  Instead, allow the bubble
to close, but don't treat this as user acknowledgment, and show the bubble again
on next startup.

This also involves tracking the close reason for a BubbleDelegateView.

BUG=548269
Committed: https://crrev.com/1462f63a60f9c7e1e53a3a42f70afc214d67fa71
Cr-Commit-Position: refs/heads/master@{#360373}

[Devlin, 2015-11-12 01:08:31]

Description was changed from

==========
[Extensions] Bubble dismiss
BUG=
==========

to

==========
[Extensions] Don't count bubble dismissal from focus loss as acknowledgment

Currently, if an extension message bubble is shown, and then it is dismissed
because it loses focus, we treat it the same as the user clicking the dismiss
button - which serves as acknowledging the extension.  We could ignore focus
loss, but this makes for very noisy, awkward bubbles.  Instead, allow the bubble
to close, but don't treat this as user acknowledgment, and show the bubble again
on next startup.

This also involves tracking the close reason for a BubbleDelegateView.

BUG=548269
==========

[Devlin, 2015-11-12 01:08:38]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-11-12 01:08:43]

Patchset #1 (id:20001) has been deleted

[Devlin, 2015-11-12 01:09:34]

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

[Devlin, 2015-11-12 01:09:34]

Hey Finnur, mind taking the extensions/ code?  (I'll add on Scott and Avi for
the views/cocoa if it lgty.)

[Finnur, 2015-11-12 15:21:19]

LGTM, with couple of questions/nits.

https://codereview.chromium.org/1437683004/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/ntp_overridden_bubble_delegate.cc (right):

https://codereview.chromium.org/1437683004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/ntp_overridden_bubble_delegate.cc:7: #include
"base/lazy_instance.h"
This intentional?

https://codereview.chromium.org/1437683004/diff/40001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/1437683004/diff/40001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_delegate.cc:186: close_reason_ = CLOSE_BY_DEACTIVATION;
I seem to recall that this would be sent even when closing with Esc and such,
but my memory is hazy. Anyway -- you'd probably see the test trigger a failure
if it were the case...

[Devlin, 2015-11-12 17:17:18]

https://codereview.chromium.org/1437683004/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/ntp_overridden_bubble_delegate.cc (right):

https://codereview.chromium.org/1437683004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/ntp_overridden_bubble_delegate.cc:7: #include
"base/lazy_instance.h"
On 2015/11/12 15:21:19, Finnur wrote:
> This intentional?

Nope. Removed.

https://codereview.chromium.org/1437683004/diff/40001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/1437683004/diff/40001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_delegate.cc:186: close_reason_ = CLOSE_BY_DEACTIVATION;
On 2015/11/12 15:21:19, Finnur wrote:
> I seem to recall that this would be sent even when closing with Esc and such,
> but my memory is hazy. Anyway -- you'd probably see the test trigger a failure
> if it were the case...

Hmm... I would hope not, but I wouldn't be entirely surprised.  Easy solution is
to not overwrite an existing close reason (which, since this class handles the
escape, should be sufficient).  We can do to be safer.

[Devlin, 2015-11-12 17:17:57]

rdevlin.cronin@chromium.org changed reviewers:
+ avi@chromium.org, sky@chromium.org

[Devlin, 2015-11-12 17:17:58]

+Sky for views/ and Avi for cocoa/; mind taking a look?

[sky, 2015-11-12 21:54:38]

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_delegate.cc:156: GetBubbleFrameView() &&
GetBubbleFrameView()->close_button_clicked()) {
Can GetBubbleFrameView() really return null here?

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_delegate.h (right):

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_delegate.h:31: enum CloseReason {
Nit, use enum class (and then rename types for that).

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.h (right):

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.h:87: LabelButton* close_button() { return
close_; }
Can you keep this private and friend the test?

[Devlin, 2015-11-12 22:40:22]

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_delegate.cc:156: GetBubbleFrameView() &&
GetBubbleFrameView()->close_button_clicked()) {
On 2015/11/12 21:54:38, sky wrote:
> Can GetBubbleFrameView() really return null here?

My thought was that this could be possible if the widget is destroyed before
showing, or some similar edge case.  I tried to dive into the code to verify,
but couldn't make sure one way or the other.  But other methods (like
OnWidgetBoundsChanged) also check for nullness.

If it seems wrong to you, though, maybe we should just CHECK it here (and change
the logic if we get some crashes)?

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_delegate.h (right):

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_delegate.h:31: enum CloseReason {
On 2015/11/12 21:54:38, sky wrote:
> Nit, use enum class (and then rename types for that).

Done.

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.h (right):

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.h:87: LabelButton* close_button() { return
close_; }
On 2015/11/12 21:54:38, sky wrote:
> Can you keep this private and friend the test?

Done.

[sky, 2015-11-12 23:56:22]

LGTM with a check/dcheck

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_delegate.cc:156: GetBubbleFrameView() &&
GetBubbleFrameView()->close_button_clicked()) {
On 2015/11/12 22:40:22, Devlin wrote:
> On 2015/11/12 21:54:38, sky wrote:
> > Can GetBubbleFrameView() really return null here?
> 
> My thought was that this could be possible if the widget is destroyed before
> showing, or some similar edge case.  I tried to dive into the code to verify,
> but couldn't make sure one way or the other.  But other methods (like
> OnWidgetBoundsChanged) also check for nullness.
> 
> If it seems wrong to you, though, maybe we should just CHECK it here (and
change
> the logic if we get some crashes)?

I suspect many of the checks are unnecessary. The reason I say that as when the
widget goes away it calls WidgetDelegateView::DeleteDelegate(), which deletes
this (BubbleDelegateView is a WidgetDelegateView). So, unless the function is
called during creation the Widget should always be non-null.
DCHECK sounds good.

[Devlin, 2015-11-13 00:03:19]

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/1437683004/diff/60001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_delegate.cc:156: GetBubbleFrameView() &&
GetBubbleFrameView()->close_button_clicked()) {
On 2015/11/12 23:56:21, sky wrote:
> On 2015/11/12 22:40:22, Devlin wrote:
> > On 2015/11/12 21:54:38, sky wrote:
> > > Can GetBubbleFrameView() really return null here?
> > 
> > My thought was that this could be possible if the widget is destroyed before
> > showing, or some similar edge case.  I tried to dive into the code to
verify,
> > but couldn't make sure one way or the other.  But other methods (like
> > OnWidgetBoundsChanged) also check for nullness.
> > 
> > If it seems wrong to you, though, maybe we should just CHECK it here (and
> change
> > the logic if we get some crashes)?
> 
> I suspect many of the checks are unnecessary. The reason I say that as when
the
> widget goes away it calls WidgetDelegateView::DeleteDelegate(), which deletes
> this (BubbleDelegateView is a WidgetDelegateView). So, unless the function is
> called during creation the Widget should always be non-null.
> DCHECK sounds good.

Done.

[Devlin, 2015-11-17 20:05:33]

Avi, friendly ping?

[Avi (use Gerrit), 2015-11-18 17:19:09]

lgtm

[Devlin, 2015-11-18 18:02:27]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-11-18 18:02:27]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1437683004/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-11-18 18:04:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1437683004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1437683004/120001

[commit-bot: I haz the power, 2015-11-18 18:52:10]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-11-18 18:52:53]

Patchset 5 (id:??) landed as
https://crrev.com/1462f63a60f9c7e1e53a3a42f70afc214d67fa71
Cr-Commit-Position: refs/heads/master@{#360373}

[Devlin, 2015-11-18 23:11:00]

A revert of this CL (patchset #5 id:120001) has been created in
https://codereview.chromium.org/1456213002/ by rdevlin.cronin@chromium.org.

The reason for reverting is: Broken MSAN:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v....

[Derek Bruening, 2015-11-18 23:14:34]

I'm the memory sheriff and this seems to be causing uninitialized read reports
from Valgrind and (pretty sure at least) Msan:

Valgrind on ChromeOS in app_list_unittests:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v...

Valgrind on ChromeOS in ash_unittests:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v...

The error reports are all similar to this:

UninitCondition
Conditional jump or move depends on uninitialised value(s)
views::BubbleDelegateView::OnWidgetClosing(views::Widget*)
(ui/views/bubble/bubble_delegate.cc:156)
non-virtual thunk to views::BubbleDelegateView::OnWidgetClosing(views::Widget*)
(ui/views/bubble/bubble_delegate.cc:154)
views::Widget::Close() (ui/views/widget/widget.cc:611)
app_list::test::(anonymous namespace)::AppListViewTestContext::Close()
(ui/app_list/views/app_list_view_unittest.cc:264)
app_list::test::(anonymous namespace)::AppListViewTestContext::RunDisplayTest()
(ui/app_list/views/app_list_view_unittest.cc:335)
app_list::test::AppListViewTestAura_Display_Test::TestBody()
(ui/app_list/views/app_list_view_unittest.cc:762)

MSan seems to be having stack trace symbol issues -- but likely the same thing
given the same unit test and timing:
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests...

As discussed separately Devlin will revert.