Fixed overlapping subtle notifications on Mac.

Fixed overlapping subtle notifications on Mac.

Can happen when a fullscreen/pointer-lock bubble and a new-back-shortcut
bubble are visible at the same time. Now the new bubble will dismiss the
old bubble (which is the behaviour on Views platforms).

BUG=615971
Committed: https://crrev.com/e1ec367efc1eeae3ba9fcb7309d4e0be44e6c44f
Cr-Commit-Position: refs/heads/master@{#397973}

[Matt Giuca, 2016-06-01 07:49:05]

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

[tapted, 2016-06-01 11:40:34]

lgtm

[Matt Giuca, 2016-06-02 00:30:56]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2016-06-02 00:33:16]

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

[commit-bot: I haz the power, 2016-06-02 02:36:12]

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

[commit-bot: I haz the power, 2016-06-02 02:36:13]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Matt Giuca, 2016-06-06 04:21:54]

Patchset #2 (id:20001) has been deleted

[Matt Giuca, 2016-06-06 04:21:59]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2016-06-06 04:22:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028963002/1

[commit-bot: I haz the power, 2016-06-06 05:01:27]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-06 05:03:11]

Description was changed from

==========
Fixed overlapping subtle notifications on Mac.

Can happen when a fullscreen/pointer-lock bubble and a new-back-shortcut
bubble are visible at the same time. Now the new bubble will dismiss the
old bubble (which is the behaviour on Views platforms).

BUG=615971
==========

to

==========
Fixed overlapping subtle notifications on Mac.

Can happen when a fullscreen/pointer-lock bubble and a new-back-shortcut
bubble are visible at the same time. Now the new bubble will dismiss the
old bubble (which is the behaviour on Views platforms).

BUG=615971

Committed: https://crrev.com/e1ec367efc1eeae3ba9fcb7309d4e0be44e6c44f
Cr-Commit-Position: refs/heads/master@{#397973}
==========

[commit-bot: I haz the power, 2016-06-06 05:03:12]

Patchset 1 (id:??) landed as
https://crrev.com/e1ec367efc1eeae3ba9fcb7309d4e0be44e6c44f
Cr-Commit-Position: refs/heads/master@{#397973}

[Peter Kasting, 2016-06-07 07:14:37]

Drive-by: Does ShowNewBackShortcutBubble() need to do the other stuff in
Destroy() (or maybe should call that method)?  Or is resetting that other state
incorrect?

[Matt Giuca, 2016-06-07 07:37:20]

Hmm, this is a bit messy but I think it's fine to not destroy the NBSB in
Destroy:

https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/browser_window_c...

Destroy is only called when we want to hide the fullscreen bubble. That probably
shouldn't close the NBSB. It's an unfortunate effect of repurposing the
ExclusiveAccessController for the new back bubble.

[Peter Kasting, 2016-06-07 21:40:21]

On 2016/06/07 07:37:20, Matt Giuca wrote:
> Hmm, this is a bit messy but I think it's fine to not destroy the NBSB in
> Destroy:
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/browser_window_c...
> 
> Destroy is only called when we want to hide the fullscreen bubble. That
probably
> shouldn't close the NBSB. It's an unfortunate effect of repurposing the
> ExclusiveAccessController for the new back bubble.

I think you got my question reversed.  I didn't mean "should Destroy() also
close the NBSB".  I meant "should the code that closes the fullscreen bubble
when we show the NBSB being doing all the things Destroy() does, not just
deleting the fullscreen bubble".

[tapted, 2016-06-08 00:54:45]

On 2016/06/07 21:40:21, Peter Kasting wrote:
> On 2016/06/07 07:37:20, Matt Giuca wrote:
> > Hmm, this is a bit messy but I think it's fine to not destroy the NBSB in
> > Destroy:
> > 
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/browser_window_c...
> > 
> > Destroy is only called when we want to hide the fullscreen bubble. That
> probably
> > shouldn't close the NBSB. It's an unfortunate effect of repurposing the
> > ExclusiveAccessController for the new back bubble.
> 
> I think you got my question reversed.  I didn't mean "should Destroy() also
> close the NBSB".  I meant "should the code that closes the fullscreen bubble
> when we show the NBSB being doing all the things Destroy() does, not just
> deleting the fullscreen bubble".

I noticed this in review, and I had a quick peek at the Destroy stuff. Apart
from tearing down the window, the only other stuff in Destroy deals with things
specifically for fullscreen -- and the codepaths for fullscreen that used to
call Destroy will still call Destroy. Calling Destroy() multiple times is fine,
so ShowNewBackShortcutBubble() could call Destroy instead, but it's not required
to hide the window, so I didn't have strong feelings about it.

Looking deeper, there is a side-effect of not calling Destroy(), but I think
it's the one we want. Fullscreen on Mac takes a ~second or so and is
asynchronous, the fullscreen bubble is configured at the start and shown at the
end. If Destroy() were to be called _during_ that transition due to backspace to
go back then the call to [BrowserWindowController
showFullscreenExitBubbleIfNecessary] at the end of the fullscreen transition
wouldn't show anything and wouldn't update the location of the
backspace-to-go-back bubble. Skipping the other stuff in Destroy ensures that
the bubble will be the fullscreen one, but also ensures it will be shown at the
right place.
 
There's also probably dead code in Destroy() that can be removed -- E.g. I don't
think url_ is used for anything any more -- it's never displayed on the
simplified fullscreen bubble.

[Matt Giuca, 2016-06-08 00:57:36]

On 2016/06/08 00:54:45, tapted wrote:
> On 2016/06/07 21:40:21, Peter Kasting wrote:
> > On 2016/06/07 07:37:20, Matt Giuca wrote:
> > > Hmm, this is a bit messy but I think it's fine to not destroy the NBSB in
> > > Destroy:
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/browser_window_c...
> > > 
> > > Destroy is only called when we want to hide the fullscreen bubble. That
> > probably
> > > shouldn't close the NBSB. It's an unfortunate effect of repurposing the
> > > ExclusiveAccessController for the new back bubble.
> > 
> > I think you got my question reversed.  I didn't mean "should Destroy() also
> > close the NBSB".  I meant "should the code that closes the fullscreen bubble
> > when we show the NBSB being doing all the things Destroy() does, not just
> > deleting the fullscreen bubble".
> 
> I noticed this in review, and I had a quick peek at the Destroy stuff. Apart
> from tearing down the window, the only other stuff in Destroy deals with
things
> specifically for fullscreen -- and the codepaths for fullscreen that used to
> call Destroy will still call Destroy. Calling Destroy() multiple times is
fine,
> so ShowNewBackShortcutBubble() could call Destroy instead, but it's not
required
> to hide the window, so I didn't have strong feelings about it.
> 
> Looking deeper, there is a side-effect of not calling Destroy(), but I think
> it's the one we want. Fullscreen on Mac takes a ~second or so and is
> asynchronous, the fullscreen bubble is configured at the start and shown at
the
> end. If Destroy() were to be called _during_ that transition due to backspace
to
> go back then the call to [BrowserWindowController
> showFullscreenExitBubbleIfNecessary] at the end of the fullscreen transition
> wouldn't show anything and wouldn't update the location of the
> backspace-to-go-back bubble. Skipping the other stuff in Destroy ensures that
> the bubble will be the fullscreen one, but also ensures it will be shown at
the
> right place.
>  
> There's also probably dead code in Destroy() that can be removed -- E.g. I
don't
> think url_ is used for anything any more -- it's never displayed on the
> simplified fullscreen bubble.

OK thanks for clarifying Trent. It sounds like this code is a bit unclear but is
probably doing the right thing.