[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back. Pressing ESC *once* works as expected.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by making the PermissionRequestManager
dismiss any open permission bubble, rather than hide them. This does not
destroy the prompt UI surface.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard, and the site is not informed of the
denied permission. This CL also addresses that issue by overriding
PermissionBubbleController::cancel to call
PermissionRequestManager::Closing(), and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This ensures that
old requests are always cleared when the bubble is hidden (otherwise they
will remain in the queue).

BUG=385088, 653497, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.

Committed: https://crrev.com/3fe468bfc1f6180bacebb8fcc0cbf9effc9394ab
Cr-Commit-Position: refs/heads/master@{#431096}

[dominickn, 2016-10-10 05:37:21]

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

[commit-bot: I haz the power, 2016-10-10 05:37:34]

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

[dominickn, 2016-10-10 05:39:20]

Description was changed from

==========
[Mac] Improve permission bubble behaviour when ESC is pressed.

When the browser window is focused on Mac, pressing the ESC key calls
BrowserWindowController::dismissPermissionBubble, which asks the
PermissionRequestManager to hide permission bubbles. This destroys the
UI surface on which permission bubbles are displayed.

If PermissionRequestManager::AddRequest is called before
PermissionRequestManager::DisplayPendingRequests, then there will be no
UI surface, and the permission display will be deferred until the user
switches to a different tab and switches back (which triggers
PermissionRequestManager::DisplayPendingRequests).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist. This
method may trigger more bubbles and is called on main frame navigations
and user interaction with the bubble. This ensures that the bubble
exists after navigation if it has been previously destroyed via
HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses this issue by
calling PermissionRequestManager::Closing() in the bubble dealloc and in
HideBubble. This matches the implementation in views and ensures that
old requests are always cleared when the bubble is hidden (otherwise
they will remain in the queue and will be redisplayed later).

BUG=385088,653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Press ESC a second time. Refresh the page and ensure
the permission prompt can be seen.
TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Refresh the page and ensure the permission prompt can be
seen.
==========

to

==========
[Mac] Improve permission bubble behaviour when ESC is pressed.

When the browser window is focused on Mac, pressing the ESC key calls
BrowserWindowController::dismissPermissionBubble, which asks the
PermissionRequestManager to hide permission bubbles. This destroys the
UI surface on which permission bubbles are displayed.

If a new permission prompt is triggered before DisplayPendingRequests is
called, then there will be no UI surface, and the permission display will be
deferred until the user switches to a different tab and switches back
(which triggers PermissionRequestManager::DisplayPendingRequests).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist. This
method may trigger more bubbles and is called on main frame navigations
and user interaction with the bubble. This ensures that the bubble
exists after navigation if it has been previously destroyed via
HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
calling PermissionRequestManager::Closing() in the bubble dealloc and in
HideBubble. This matches the implementation in views and ensures that
old requests are always cleared when the bubble is hidden (otherwise
they will remain in the queue and will be redisplayed later).

BUG=385088,653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Press ESC a second time. Refresh the page and ensure
the permission prompt can be seen.
TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Refresh the page and ensure the permission prompt can be
seen.
==========

[dominickn, 2016-10-10 05:39:45]

dominickn@chromium.org changed reviewers:
+ felt@chromium.org, groby@chromium.org

[dominickn, 2016-10-10 05:46:12]

Description was changed from

==========
[Mac] Improve permission bubble behaviour when ESC is pressed.

When the browser window is focused on Mac, pressing the ESC key calls
BrowserWindowController::dismissPermissionBubble, which asks the
PermissionRequestManager to hide permission bubbles. This destroys the
UI surface on which permission bubbles are displayed.

If a new permission prompt is triggered before DisplayPendingRequests is
called, then there will be no UI surface, and the permission display will be
deferred until the user switches to a different tab and switches back
(which triggers PermissionRequestManager::DisplayPendingRequests).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist. This
method may trigger more bubbles and is called on main frame navigations
and user interaction with the bubble. This ensures that the bubble
exists after navigation if it has been previously destroyed via
HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
calling PermissionRequestManager::Closing() in the bubble dealloc and in
HideBubble. This matches the implementation in views and ensures that
old requests are always cleared when the bubble is hidden (otherwise
they will remain in the queue and will be redisplayed later).

BUG=385088,653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Press ESC a second time. Refresh the page and ensure
the permission prompt can be seen.
TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Refresh the page and ensure the permission prompt can be
seen.
==========

to

==========
[Mac] Improve permission bubble behaviour when ESC is pressed.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back.

This happens beacuse pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all
permission bubbles, which has the side-effect of destroying the
permission prompt UI surface. The next triggered prompt then has no
surface to be displayed on, delaying its display until
DisplayPendingRequests is called and the surface is recreated (e.g.
when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist. This
method may trigger more bubbles and is called on main frame navigations
and user interaction with the bubble, so it is correct to have a new view
created in it, ensuring that the bubble exists after navigation if it has
been previously destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
calling PermissionRequestManager::Closing() in
PermissionBubbleController's dealloc and in HideBubble. This matches
the implementation in views and ensures that old requests are always
cleared when the bubble is hidden (otherwise they will remain in the
queue and will be redisplayed later).

BUG=385088,653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run. Press
ESC a second time. Refresh the page and ensure the permission prompt
can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh the
page and ensure the permission prompt can be seen.
==========

[dominickn, 2016-10-10 05:46:48]

Description was changed from

==========
[Mac] Improve permission bubble behaviour when ESC is pressed.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back.

This happens beacuse pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all
permission bubbles, which has the side-effect of destroying the
permission prompt UI surface. The next triggered prompt then has no
surface to be displayed on, delaying its display until
DisplayPendingRequests is called and the surface is recreated (e.g.
when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist. This
method may trigger more bubbles and is called on main frame navigations
and user interaction with the bubble, so it is correct to have a new view
created in it, ensuring that the bubble exists after navigation if it has
been previously destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
calling PermissionRequestManager::Closing() in
PermissionBubbleController's dealloc and in HideBubble. This matches
the implementation in views and ensures that old requests are always
cleared when the bubble is hidden (otherwise they will remain in the
queue and will be redisplayed later).

BUG=385088,653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run. Press
ESC a second time. Refresh the page and ensure the permission prompt
can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh the
page and ensure the permission prompt can be seen.
==========

to

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back.

This happens beacuse pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all
permission bubbles, which has the side-effect of destroying the
permission prompt UI surface. The next triggered prompt then has no
surface to be displayed on, delaying its display until
DisplayPendingRequests is called and the surface is recreated (e.g.
when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist. This
method may trigger more bubbles and is called on main frame navigations
and user interaction with the bubble, so it is correct to have a new view
created in it, ensuring that the bubble exists after navigation if it has
been previously destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
calling PermissionRequestManager::Closing() in
PermissionBubbleController's dealloc and in HideBubble. This matches
the implementation in views and ensures that old requests are always
cleared when the bubble is hidden (otherwise they will remain in the
queue and will be redisplayed later).

BUG=385088,653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run. Press
ESC a second time. Refresh the page and ensure the permission prompt
can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh the
page and ensure the permission prompt can be seen.
==========

[dominickn, 2016-10-10 05:51:01]

Description was changed from

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back.

This happens beacuse pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all
permission bubbles, which has the side-effect of destroying the
permission prompt UI surface. The next triggered prompt then has no
surface to be displayed on, delaying its display until
DisplayPendingRequests is called and the surface is recreated (e.g.
when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist. This
method may trigger more bubbles and is called on main frame navigations
and user interaction with the bubble, so it is correct to have a new view
created in it, ensuring that the bubble exists after navigation if it has
been previously destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
calling PermissionRequestManager::Closing() in
PermissionBubbleController's dealloc and in HideBubble. This matches
the implementation in views and ensures that old requests are always
cleared when the bubble is hidden (otherwise they will remain in the
queue and will be redisplayed later).

BUG=385088,653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run. Press
ESC a second time. Refresh the page and ensure the permission prompt
can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh the
page and ensure the permission prompt can be seen.
==========

to

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back.

This happens beacuse pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist. This
method may trigger more bubbles and is called on main frame navigations
and user interaction with the bubble, so it is correct to have a new
view created in it, ensuring that the bubble exists after navigation if
it has been previously destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
calling PermissionRequestManager::Closing() in
PermissionBubbleController's dealloc and in HideBubble. This matches the
implementation in views and ensures that old requests are always cleared
when the bubble is hidden (otherwise they will remain in the queue).

BUG=385088, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

[commit-bot: I haz the power, 2016-10-10 06:06:54]

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

[commit-bot: I haz the power, 2016-10-10 06:06:55]

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

[dominickn, 2016-10-10 06:46:04]

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

[commit-bot: I haz the power, 2016-10-10 06:46:08]

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, 2016-10-10 07:17:06]

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

[commit-bot: I haz the power, 2016-10-10 07:17:07]

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

[dominickn, 2016-10-10 09:46:29]

Description was changed from

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back.

This happens beacuse pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist. This
method may trigger more bubbles and is called on main frame navigations
and user interaction with the bubble, so it is correct to have a new
view created in it, ensuring that the bubble exists after navigation if
it has been previously destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
calling PermissionRequestManager::Closing() in
PermissionBubbleController's dealloc and in HideBubble. This matches the
implementation in views and ensures that old requests are always cleared
when the bubble is hidden (otherwise they will remain in the queue).

BUG=385088, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

to

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist.
This ensures that the surface is recreated if it has been previously
destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
calling PermissionRequestManager::Closing() in
PermissionBubbleController's dealloc and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This matches the
implementation in views and ensures that old requests are always cleared
when the bubble is hidden (otherwise they will remain in the queue).

BUG=385088, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

[dominickn, 2016-10-10 09:50:50]

Patchset #3 (id:40001) has been deleted

[dominickn, 2016-10-10 10:05:34]

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

[commit-bot: I haz the power, 2016-10-10 10:05:38]

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

[dominickn, 2016-10-10 10:20:12]

Description was changed from

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist.
This ensures that the surface is recreated if it has been previously
destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
calling PermissionRequestManager::Closing() in
PermissionBubbleController's dealloc and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This matches the
implementation in views and ensures that old requests are always cleared
when the bubble is hidden (otherwise they will remain in the queue).

BUG=385088, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

to

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist.
This ensures that the surface is recreated if it has been previously
destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
calling PermissionRequestManager::Closing() in
PermissionBubbleController's dealloc and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This matches the
implementation in views and ensures that old requests are always cleared
when the bubble is hidden (otherwise they will remain in the queue).

BUG=385088, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

[dominickn, 2016-10-10 10:20:12]

dominickn@chromium.org changed reviewers:
- felt@chromium.org, groby@chromium.org

[dominickn, 2016-10-10 10:49:20]

Description was changed from

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist.
This ensures that the surface is recreated if it has been previously
destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
calling PermissionRequestManager::Closing() in
PermissionBubbleController's dealloc and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This matches the
implementation in views and ensures that old requests are always cleared
when the bubble is hidden (otherwise they will remain in the queue).

BUG=385088, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

to

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist.
This ensures that the surface is recreated if it has been previously
destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
overriding PermissionBubbleController::cancel to call
PermissionRequestManager::Closing() and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This ensures that
old requests are always cleared when the bubble is hidden (otherwise they
will remain in the queue).

BUG=385088, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

[dominickn, 2016-10-10 10:55:22]

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

[commit-bot: I haz the power, 2016-10-10 10:55:34]

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

[dominickn, 2016-10-10 11:05:16]

Description was changed from

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist.
This ensures that the surface is recreated if it has been previously
destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
overriding PermissionBubbleController::cancel to call
PermissionRequestManager::Closing() and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This ensures that
old requests are always cleared when the bubble is hidden (otherwise they
will remain in the queue).

BUG=385088, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

to

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist.
This ensures that the surface is recreated if it has been previously
destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
overriding PermissionBubbleController::cancel to call
PermissionRequestManager::Closing() and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This ensures that
old requests are always cleared when the bubble is hidden (otherwise they
will remain in the queue).

BUG=385088, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

[dominickn, 2016-10-10 11:05:16]

dominickn@chromium.org changed reviewers:
+ felt@chromium.org, groby@chromium.org

[commit-bot: I haz the power, 2016-10-10 11:44:52]

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

[commit-bot: I haz the power, 2016-10-10 11:44:53]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-10-10 12:00:12]

Description was changed from

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist.
This ensures that the surface is recreated if it has been previously
destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
overriding PermissionBubbleController::cancel to call
PermissionRequestManager::Closing() and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This ensures that
old requests are always cleared when the bubble is hidden (otherwise they
will remain in the queue).

BUG=385088, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

to

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back. Pressing ESC *once* works as expected.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist.
This ensures that the surface is recreated if it has been previously
destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
overriding PermissionBubbleController::cancel to call
PermissionRequestManager::Closing() and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This ensures that
old requests are always cleared when the bubble is hidden (otherwise they
will remain in the queue).

BUG=385088, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

[dominickn, 2016-10-10 12:01:17]

Description was changed from

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back. Pressing ESC *once* works as expected.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist.
This ensures that the surface is recreated if it has been previously
destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard. This CL also addresses that issue by
overriding PermissionBubbleController::cancel to call
PermissionRequestManager::Closing() and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This ensures that
old requests are always cleared when the bubble is hidden (otherwise they
will remain in the queue).

BUG=385088, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

to

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back. Pressing ESC *once* works as expected.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist.
This ensures that the surface is recreated if it has been previously
destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard, and the site is not informed of the
denied permission. This CL also addresses that issue by overriding
PermissionBubbleController::cancel to call
PermissionRequestManager::Closing(), and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This ensures that
old requests are always cleared when the bubble is hidden (otherwise they
will remain in the queue).

BUG=385088, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

[dominickn, 2016-10-10 12:01:48]

Hi felt, groby!

felt: PTAL at permissions/ and website_settings.

groby: PTAL at cocoa.

This CL stemmed from crbug.com/653498 (possibly the weirdest bug I'd ever seen
at first glance). The issue ultimately came about from trying to make Esc always
dismiss permission prompts in crbug.com/385088, so I've added you two as
reviewers.

[dominickn, 2016-10-10 12:05:38]

Description was changed from

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back. Pressing ESC *once* works as expected.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist.
This ensures that the surface is recreated if it has been previously
destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard, and the site is not informed of the
denied permission. This CL also addresses that issue by overriding
PermissionBubbleController::cancel to call
PermissionRequestManager::Closing(), and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This ensures that
old requests are always cleared when the bubble is hidden (otherwise they
will remain in the queue).

BUG=385088, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

to

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back. Pressing ESC *once* works as expected.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist.
This ensures that the surface is recreated if it has been previously
destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard, and the site is not informed of the
denied permission. This CL also addresses that issue by overriding
PermissionBubbleController::cancel to call
PermissionRequestManager::Closing(), and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This ensures that
old requests are always cleared when the bubble is hidden (otherwise they
will remain in the queue).

BUG=385088, 653497, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

[groby-ooo-7-16, 2016-10-10 14:37:45]

groby@chromium.org changed reviewers:
+ hcarmona@chromium.org

[groby-ooo-7-16, 2016-10-10 14:37:55]

On 2016/10/10 12:01:48, dominickn wrote:
> Hi felt, groby!
> 
> felt: PTAL at permissions/ and website_settings.
> 
> groby: PTAL at cocoa.
> 
> This CL stemmed from crbug.com/653498 (possibly the weirdest bug I'd ever seen
> at first glance). The issue ultimately came about from trying to make Esc
always
> dismiss permission prompts in crbug.com/385088, so I've added you two as
> reviewers.


Traveling, can only look at this tomorrow. I've also added in hcarmona, who's
quite familiar with bubble machinations.

But I do want to say thank you for writing a detailed CL description!

[hcarmona, 2016-10-10 18:08:19]

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request_manager.cc (right):

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.cc:226: void
PermissionRequestManager::DismissBubble() {
|Closing| calls |FinalizeBubble| which hides the bubble. Can we use that
instead of creating a new method?

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.cc:408: // Ensure that the
view exists. It may have been deleted by HideBubble,
Good description. However, since your change is fixing this, can we
remove the crbug reference? Whenever I see a link to the bug tracker I
assume it's an active bug.

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm
(right):

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:15: :
browser_(browser), delegate_(nullptr), bubbleController_(nil) {}
Why is the DCHECK moved?

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/permission_prompt_impl.cc (right):

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:397:
bubble_delegate_(nullptr) {}
Same here.

[felt, 2016-10-10 18:18:59]

wow this is quite an interesting bug!

[felt, 2016-10-10 18:21:00]

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request_manager.h (right):

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.h:80: void HideBubble();
Do we *ever* want this behavior? Should we get rid of HideBubble, or make it act
like DismissBubble now does?

[hcarmona, 2016-10-10 18:28:51]

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request_manager.h (right):

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.h:80: void HideBubble();
On 2016/10/10 18:21:00, felt wrote:
> Do we *ever* want this behavior? Should we get rid of HideBubble, or make it
act
> like DismissBubble now does?

|HideBubble| is used when switching tabs to hide the bubble without
dismissing. The bubble will be shown again in that case.

[groby-ooo-7-16, 2016-10-10 18:58:10]

On 2016/10/10 18:28:51, Hector Carmona wrote:
>
https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
> File chrome/browser/permissions/permission_request_manager.h (right):
> 
>
https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
> chrome/browser/permissions/permission_request_manager.h:80: void HideBubble();
> On 2016/10/10 18:21:00, felt wrote:
> > Do we *ever* want this behavior? Should we get rid of HideBubble, or make it
> act
> > like DismissBubble now does?
> 
> |HideBubble| is used when switching tabs to hide the bubble without
> dismissing. The bubble will be shown again in that case.

we should probably mention that in a comment, then - I had the same question :)

[dominickn, 2016-10-10 22:55:12]

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

[commit-bot: I haz the power, 2016-10-10 22:55:28]

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

[dominickn, 2016-10-10 22:55:48]

Thanks everyone!

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request_manager.cc (right):

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.cc:226: void
PermissionRequestManager::DismissBubble() {
On 2016/10/10 18:08:19, Hector Carmona wrote:
> |Closing| calls |FinalizeBubble| which hides the bubble. Can we use that
> instead of creating a new method?

This would work, and I did think of doing that. The main caveat is that
|Closing| is private and an inherited delegate method within
PermissionRequestManager, so BrowserWindowController can't currently access it.
I thought about exposing it - in that case, it would be the only public delegate
method exposed in this way. felt, do you have OWNER thoughts on this?

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.cc:408: // Ensure that the
view exists. It may have been deleted by HideBubble,
On 2016/10/10 18:08:19, Hector Carmona wrote:
> Good description. However, since your change is fixing this, can we
> remove the crbug reference? Whenever I see a link to the bug tracker I
> assume it's an active bug.

Done.

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request_manager.h (right):

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.h:80: void HideBubble();
On 2016/10/10 18:28:51, Hector Carmona wrote:
> On 2016/10/10 18:21:00, felt wrote:
> > Do we *ever* want this behavior? Should we get rid of HideBubble, or make it
> act
> > like DismissBubble now does?
> 
> |HideBubble| is used when switching tabs to hide the bubble without
> dismissing. The bubble will be shown again in that case.

I clarified the comment here.

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm
(right):

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:15: :
browser_(browser), delegate_(nullptr), bubbleController_(nil) {}
On 2016/10/10 18:08:19, Hector Carmona wrote:
> Why is the DCHECK moved?

A number of browser and unit tests do navigations, which run through
PermissionRequestManager::DidNavigateMainFrame, which calls FinalizeBubble,
which now creates a bubble if one doesn't exist. In many cases, a null browser
pointer is sent into the constructor here (and in the views impl too).

To fix those tests, I moved the DCHECK to the methods which actually need the
non-null pointer. This means that the tests work, and we still ensure that any
test which tries to show or otherwise do UI-related things to bubbles still have
the DCHECK.

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/permission_prompt_impl.cc (right):

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:397:
bubble_delegate_(nullptr) {}
On 2016/10/10 18:08:19, Hector Carmona wrote:
> Same here.

See other comment

[commit-bot: I haz the power, 2016-10-11 00:41:28]

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

[commit-bot: I haz the power, 2016-10-11 00:41:30]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hcarmona, 2016-10-11 14:45:33]

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request_manager.cc (right):

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.cc:226: void
PermissionRequestManager::DismissBubble() {
On 2016/10/10 22:55:48, dominickn wrote:
> On 2016/10/10 18:08:19, Hector Carmona wrote:
> > |Closing| calls |FinalizeBubble| which hides the bubble. Can we use that
> > instead of creating a new method?
> 
> This would work, and I did think of doing that. The main caveat is that
> |Closing| is private and an inherited delegate method within
> PermissionRequestManager, so BrowserWindowController can't currently access
it.
> I thought about exposing it - in that case, it would be the only public
delegate
> method exposed in this way. felt, do you have OWNER thoughts on this?

Casting to PermissionPrompt::Delegate will let you call |Closing|, this
way you don't need to expose the method.

Maybe having |DismissBubble| just call |Closing| so it's more obvious
that this is supposed to do the same thing?

However, I defer to felt@ on what is best to do here.

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request_manager.h (right):

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.h:80: void HideBubble();
On 2016/10/10 22:55:48, dominickn wrote:
> On 2016/10/10 18:28:51, Hector Carmona wrote:
> > On 2016/10/10 18:21:00, felt wrote:
> > > Do we *ever* want this behavior? Should we get rid of HideBubble, or make
it
> > act
> > > like DismissBubble now does?
> > 
> > |HideBubble| is used when switching tabs to hide the bubble without
> > dismissing. The bubble will be shown again in that case.
> 
> I clarified the comment here.

Thanks!

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm
(right):

https://codereview.chromium.org/2403763003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:15: :
browser_(browser), delegate_(nullptr), bubbleController_(nil) {}
On 2016/10/10 22:55:48, dominickn wrote:
> On 2016/10/10 18:08:19, Hector Carmona wrote:
> > Why is the DCHECK moved?
> 
> A number of browser and unit tests do navigations, which run through
> PermissionRequestManager::DidNavigateMainFrame, which calls FinalizeBubble,
> which now creates a bubble if one doesn't exist. In many cases, a null browser
> pointer is sent into the constructor here (and in the views impl too).
> 
> To fix those tests, I moved the DCHECK to the methods which actually need the
> non-null pointer. This means that the tests work, and we still ensure that any
> test which tries to show or otherwise do UI-related things to bubbles still
have
> the DCHECK.

Sounds good.

[dominickn, 2016-10-17 05:34:34]

felt, groby: Ping. :)

I've thought a little about tests for this. Currently, it doesn't look like any
permission actually has any keyboard related tests (they mostly mock out the
interaction with the permission prompt, possibly because it would be flaky and a
bit error prone). It would be nice to rectify this situation for permissions
overall, but that's probably a bit project that will encompass all the
platforms.

[dominickn, 2016-10-25 04:48:05]

dominickn@chromium.org changed reviewers:
+ raymes@chromium.org
- felt@chromium.org

[dominickn, 2016-10-25 04:48:06]

-felt for raymes.

groby: ping, this has been sitting around for quite a while now.... :)

[groby-ooo-7-16, 2016-10-25 19:23:35]

On 2016/10/25 04:48:06, dominickn wrote:
> -felt for raymes.
> 
> groby: ping, this has been sitting around for quite a while now.... :)

Yes, my apologies :( 


I'm still not sure why FinalizeBubble needs to create a UI surface. Could you
outline what specifically requires this?

Also, can you add test cases for the things your fixing, please? It feels like
PermissionBubble's ESC handling comes up on a recurring basis, and having tests
would help prevent regressions.

[raymes, 2016-10-26 04:59:11]

https://codereview.chromium.org/2403763003/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_manager.cc (right):

https://codereview.chromium.org/2403763003/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_manager.cc:229: HideBubble();
Could we just remove HideBubble here and then we wouldn't need to recreate it
later?

https://codereview.chromium.org/2403763003/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_manager.h (right):

https://codereview.chromium.org/2403763003/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_manager.h:75: // display
permission requests.
Could you add a note here that this is only used on Mac and shouldn't be called
anywhere else. And also a TODO to remove this once we switch to views?

[dominickn, 2016-10-26 05:54:23]

groby: I wrote about tests a few comments ago. In discussion with raymes@, we
couldn't come up with any test that wouldn't require a significant amount of
infrastructure.

The bug can't be caught with unit tests, because we need a permission bubble to
appear to interact with it. All of the existing permission bubble browser or UI
tests mock out the bubble; there's no infrastructure for manipulating the
bubble. Also, focus management is needed to send keyboard events and assert that
they happen correctly, because part of this CL addresses an issue that manifests
itself when the browser window versus the bubble itself is focused.

We definitely should improve the testing of permission bubbles. That's a pretty
big project in and upon itself, because there isn't any right now without a
mocked-out bubble. Also, focus is different across ChromeOS/Linux/Mac, and that
adds more complexity. Unfortunately I have a bunch of other priorities at the
moment so I can't invest that time right now.

I would like to land the fix for this bug because it's pretty nasty, unintuitive
behaviour, but right now, I don't really have bandwidth to implement a
comprehensive test. Thoughts?

https://codereview.chromium.org/2403763003/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_manager.cc (right):

https://codereview.chromium.org/2403763003/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_manager.cc:229: HideBubble();
On 2016/10/26 04:59:11, raymes wrote:
> Could we just remove HideBubble here and then we wouldn't need to recreate it
> later?

Done.

https://codereview.chromium.org/2403763003/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_manager.h (right):

https://codereview.chromium.org/2403763003/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_manager.h:75: // display
permission requests.
On 2016/10/26 04:59:11, raymes wrote:
> Could you add a note here that this is only used on Mac and shouldn't be
called
> anywhere else. And also a TODO to remove this once we switch to views?

I don't want to do that yet, because at some point I need to investigate if this
is an issue on views too (the bugs linked claims that it's not, but I'm
suspicious and want to verify).

[raymes, 2016-10-27 01:42:57]

lgtm

It's unclear to me why on Mac we manually need to handle the
dismissPermissionBubble case specially by going directly to the
PermissionManager - but that's already in place and I'll assume it's needed for
an important reason which cocoa OWNERS know about :)

https://codereview.chromium.org/2403763003/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_manager.h (right):

https://codereview.chromium.org/2403763003/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_manager.h:75: // display
permission requests.
On 2016/10/26 05:54:23, dominickn wrote:
> On 2016/10/26 04:59:11, raymes wrote:
> > Could you add a note here that this is only used on Mac and shouldn't be
> called
> > anywhere else. And also a TODO to remove this once we switch to views?
> 
> I don't want to do that yet, because at some point I need to investigate if
this
> is an issue on views too (the bugs linked claims that it's not, but I'm
> suspicious and want to verify).

Can we add a TODO to that effect then, with a bug link? Something saying that
it's currently only needed for Mac and may not be needed at all, but should be
investigated? It would be good to steer people away from using this if we will
get rid of it.

[raymes, 2016-10-27 01:47:52]

A few thoughts after I hit the button that would avoid needing to add that new
function at all.

https://codereview.chromium.org/2403763003/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_manager.cc (right):

https://codereview.chromium.org/2403763003/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_manager.cc:227: if
(IsBubbleVisible())
Do you know why this check is needed? Should we just call Closing() directly?

https://codereview.chromium.org/2403763003/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/2403763003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:1596:
manager->DismissBubble();
Actually, can we just cast manager to a PermissionPrompt::Delegate here and call
closing? That would avoid needing to add DismissBubble at all. IsBubbleVisible
is also public so can be called if it's needed but see my other question.

[dominickn, 2016-10-27 02:09:02]

https://codereview.chromium.org/2403763003/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_manager.cc (right):

https://codereview.chromium.org/2403763003/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_manager.cc:227: if
(IsBubbleVisible())
On 2016/10/27 01:47:52, raymes wrote:
> Do you know why this check is needed? Should we just call Closing() directly?

That was the exact question I posed to felt@, whether the cast and call
Closing() was a better idea.

https://codereview.chromium.org/2403763003/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/2403763003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:1596:
manager->DismissBubble();
On 2016/10/27 01:47:52, raymes wrote:
> Actually, can we just cast manager to a PermissionPrompt::Delegate here and
call
> closing? That would avoid needing to add DismissBubble at all. IsBubbleVisible
> is also public so can be called if it's needed but see my other question.

I asked felt this question. Since you're okay with it as a permissions OWNER,
I'm going to implement it.

[dominickn, 2016-10-27 02:09:50]

Description was changed from

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back. Pressing ESC *once* works as expected.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by recreating the prompt UI surface in
PermissionRequestManager::FinalizeBubble if it does not exist.
This ensures that the surface is recreated if it has been previously
destroyed via HideBubble.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard, and the site is not informed of the
denied permission. This CL also addresses that issue by overriding
PermissionBubbleController::cancel to call
PermissionRequestManager::Closing(), and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This ensures that
old requests are always cleared when the bubble is hidden (otherwise they
will remain in the queue).

BUG=385088, 653497, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

to

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back. Pressing ESC *once* works as expected.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by making the PermissionRequestManager
dismiss any open permission bubble, rather than hide them. This does not
destroy the prompt UI surface.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard, and the site is not informed of the
denied permission. This CL also addresses that issue by overriding
PermissionBubbleController::cancel to call
PermissionRequestManager::Closing(), and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This ensures that
old requests are always cleared when the bubble is hidden (otherwise they
will remain in the queue).

BUG=385088, 653497, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

[dominickn, 2016-10-27 02:10:41]

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

[commit-bot: I haz the power, 2016-10-27 02:10:59]

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

[raymes, 2016-10-27 02:26:39]

https://codereview.chromium.org/2403763003/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_manager.cc (right):

https://codereview.chromium.org/2403763003/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_manager.cc:227: if
(IsBubbleVisible())
On 2016/10/27 02:09:02, dominickn wrote:
> On 2016/10/27 01:47:52, raymes wrote:
> > Do you know why this check is needed? Should we just call Closing()
directly?
> 
> That was the exact question I posed to felt@, whether the cast and call
> Closing() was a better idea.


Do you know whether IsBubbleVisible call is needed? Was there a reason that you
added it?

[dominickn, 2016-10-27 02:37:59]

On 2016/10/27 02:26:39, raymes wrote:
>
https://codereview.chromium.org/2403763003/diff/120001/chrome/browser/permiss...
> File chrome/browser/permissions/permission_request_manager.cc (right):
> 
>
https://codereview.chromium.org/2403763003/diff/120001/chrome/browser/permiss...
> chrome/browser/permissions/permission_request_manager.cc:227: if
> (IsBubbleVisible())
> On 2016/10/27 02:09:02, dominickn wrote:
> > On 2016/10/27 01:47:52, raymes wrote:
> > > Do you know why this check is needed? Should we just call Closing()
> directly?
> > 
> > That was the exact question I posed to felt@, whether the cast and call
> > Closing() was a better idea.
> 
> 
> Do you know whether IsBubbleVisible call is needed? Was there a reason that
you
> added it?

That was to try and avoid calling HideBubble() if a bubble wasn't visible.
Closing() doesn't have the same side effects so it's fine to just call it (it
does an iteration over an empty list if no bubble is present)

[commit-bot: I haz the power, 2016-10-27 03:14:47]

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

[commit-bot: I haz the power, 2016-10-27 03:14:48]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-10-27 04:27:15]

Thanks Dom.

lgtm

[hcarmona, 2016-10-28 14:42:15]

LGTM, in case you are waiting to hear from me.
(I'm not OWNER, so you still need groby@)

[dominickn, 2016-11-02 06:20:36]

groby: Ping. What are your thoughts on my explanations regarding the tests?

[groby-ooo-7-16, 2016-11-09 23:06:42]

I think it makes sense to move the tests into a separate project, but we should
give them some priority. 

Otherwise, LGTM

[dominickn, 2016-11-09 23:18:51]

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

[commit-bot: I haz the power, 2016-11-09 23:19:51]

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

[dominickn, 2016-11-09 23:42:32]

On 2016/11/09 23:06:42, groby wrote:
> I think it makes sense to move the tests into a separate project, but we
should
> give them some priority. 
> 
> Otherwise, LGTM

Thanks! I've filed crbug.com/663935 to track the desktop permissions UI surface
testing work.

[commit-bot: I haz the power, 2016-11-10 00:13:49]

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

[commit-bot: I haz the power, 2016-11-10 00:13:51]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-11-10 00:15:31]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2016-11-10 00:16:24]

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, 2016-11-10 00:28:48]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-11-10 00:38:16]

Description was changed from

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back. Pressing ESC *once* works as expected.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by making the PermissionRequestManager
dismiss any open permission bubble, rather than hide them. This does not
destroy the prompt UI surface.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard, and the site is not informed of the
denied permission. This CL also addresses that issue by overriding
PermissionBubbleController::cancel to call
PermissionRequestManager::Closing(), and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This ensures that
old requests are always cleared when the bubble is hidden (otherwise they
will remain in the queue).

BUG=385088, 653497, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.
==========

to

==========
[Mac] Address buggy permission bubble behaviour on dismissal via ESC.

On Mac, triggering a permission prompt, dismissing it by pressing ESC
*twice*, then refreshing the page and triggering the prompt again
results in no prompt being displayed until the tab is switched away and
back. Pressing ESC *once* works as expected.

This happens because pressing the ESC key with the browser window
focused results in the PermissionRequestManager hiding all permission
bubbles, which has the side-effect of destroying the prompt UI surface.
The next prompt triggered then has no surface to be displayed on,
delaying its display until DisplayPendingRequests is called and the
surface is recreated (e.g. when switching tabs).

This CL fixes the problem by making the PermissionRequestManager
dismiss any open permission bubble, rather than hide them. This does not
destroy the prompt UI surface.

Additionally, pressing ESC on Mac does not properly dismiss a permission
prompt, i.e. the PermissionDecided callback is *not* run when a bubble
is dismissed with the keyboard, and the site is not informed of the
denied permission. This CL also addresses that issue by overriding
PermissionBubbleController::cancel to call
PermissionRequestManager::Closing(), and adding a new method called by the
Mac BrowserWindowController that also calls Closing(). This ensures that
old requests are always cleared when the bubble is hidden (otherwise they
will remain in the queue).

BUG=385088, 653497, 653498
TEST=Visit a page which displays a permission bubble. Dismiss the bubble
by pressing ESC. Ensure that the permission denied callback is run.
Press ESC a second time. Refresh the page and ensure the permission
prompt can be seen.

TEST=Visit a page which displays a permission bubble. Click on the page
(not on the bubble) to change focus. Press ESC and ensure the bubble is
dismissed. Ensure that the permission denied callback is run. Refresh
the page and ensure the permission prompt can be seen.

Committed: https://crrev.com/3fe468bfc1f6180bacebb8fcc0cbf9effc9394ab
Cr-Commit-Position: refs/heads/master@{#431096}
==========

[commit-bot: I haz the power, 2016-11-10 00:38:17]

Patchset 7 (id:??) landed as
https://crrev.com/3fe468bfc1f6180bacebb8fcc0cbf9effc9394ab
Cr-Commit-Position: refs/heads/master@{#431096}