MacViews: Trigger shadow invalidation when a translucent window is shown.

MacViews: Trigger shadow invalidation when a translucent window is shown.

Currently, invalidate_shadow_on_frame_swap_ is not set when a MacViews window is
shown. It is only set when the layer is created, changed or the window resized.
This causes translucent windows like the "Restore Pages" bubble and Find-In-Page
window to not have a shadow, when they are shown after being hidden.

This CL modifies BridgedNativeWidget::OnVisibilityChanged() to trigger shadow
invalidation while showing translucent windows. This fixes the shadow for the
"Restore Pages" bubble and also of the Find-In-Page window on tab switching. A
unit test is also added which demonstrates the problem.

Note that this does not fix the shadow for the Find-In-Page window when it is
displayed initially on pressing Cmd+F.

BUG=636707, 646734
Committed: https://crrev.com/e5cbef5ab4cea989839c24925113f5a062a2fb9a
Cr-Commit-Position: refs/heads/master@{#418803}

[karandeepb, 2016-09-13 05:13:14]

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

[commit-bot: I haz the power, 2016-09-13 05:13:24]

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-09-13 05:54:13]

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

[commit-bot: I haz the power, 2016-09-13 05:54:14]

Dry run: This issue passed the CQ dry run.

[karandeepb, 2016-09-13 06:07:02]

karandeepb@chromium.org changed reviewers:
+ avi@chromium.org

[karandeepb, 2016-09-13 06:07:02]

PTAL avi@. Can you please review this since tapted@ is OOO.

[karandeepb, 2016-09-13 06:46:28]

Patchset #3 (id:40001) has been deleted

[Avi (use Gerrit), 2016-09-13 15:37:03]

avi@chromium.org changed reviewers:
+ ellyjones@chromium.org

[Avi (use Gerrit), 2016-09-13 15:37:04]

This seems reasonable, but I'm not up-to-speed on MacViews. Maybe Elly?

[karandeepb, 2016-09-14 01:22:31]

On 2016/09/13 15:37:04, Avi wrote:
> This seems reasonable, but I'm not up-to-speed on MacViews. Maybe Elly?

That's ok. Trent is back tomorrow so I'll hold this till then. Thanks.

[karandeepb, 2016-09-14 01:24:44]

On 2016/09/14 01:22:31, karandeepb wrote:
> On 2016/09/13 15:37:04, Avi wrote:
> > This seems reasonable, but I'm not up-to-speed on MacViews. Maybe Elly?
> 
> That's ok. Trent is back tomorrow so I'll hold this till then. Thanks.

Or if Elly can review it, that's fine as well.

[karandeepb, 2016-09-14 07:26:04]

Description was changed from

==========
MacViews: Trigger shadow invalidation when a translucent window is shown.

Currently, invalidate_shadow_on_frame_swap_ is not set when a MacViews window is
shown. It is only set when the layer is created, changed or the window resized.
This causes translucent bubbles like the "Restore Pages" bubble to not have a
shadow, when they are shown after being hidden.

This CL modifies BridgedNativeWidget::OnVisibilityChanged() to trigger shadow
invalidation while showing translucent windows. This fixes the shadow for the
"Restore Pages" bubble. A unit test is also added which demonstrates the
problem.

BUG=636707
==========

to

==========
MacViews: Trigger shadow invalidation when a translucent window is shown.

Currently, invalidate_shadow_on_frame_swap_ is not set when a MacViews window is
shown. It is only set when the layer is created, changed or the window resized.
This causes translucent windows like the "Restore Pages" bubble and Find-In-Page
window to not have a shadow, when they are shown after being hidden.

This CL modifies BridgedNativeWidget::OnVisibilityChanged() to trigger shadow
invalidation while showing translucent windows. This fixes the shadow for the
"Restore Pages" bubble and also of the Find-In-Page bubble on tab switching. A
unit test is also added which demonstrates the problem.

Note that this does not fix the shadow for the Find-In-Page window when it is
displayed initially on pressing Cmd+F. 

BUG=636707, 646734
==========

[karandeepb, 2016-09-14 07:26:50]

Description was changed from

==========
MacViews: Trigger shadow invalidation when a translucent window is shown.

Currently, invalidate_shadow_on_frame_swap_ is not set when a MacViews window is
shown. It is only set when the layer is created, changed or the window resized.
This causes translucent windows like the "Restore Pages" bubble and Find-In-Page
window to not have a shadow, when they are shown after being hidden.

This CL modifies BridgedNativeWidget::OnVisibilityChanged() to trigger shadow
invalidation while showing translucent windows. This fixes the shadow for the
"Restore Pages" bubble and also of the Find-In-Page bubble on tab switching. A
unit test is also added which demonstrates the problem.

Note that this does not fix the shadow for the Find-In-Page window when it is
displayed initially on pressing Cmd+F. 

BUG=636707, 646734
==========

to

==========
MacViews: Trigger shadow invalidation when a translucent window is shown.

Currently, invalidate_shadow_on_frame_swap_ is not set when a MacViews window is
shown. It is only set when the layer is created, changed or the window resized.
This causes translucent windows like the "Restore Pages" bubble and Find-In-Page
window to not have a shadow, when they are shown after being hidden.

This CL modifies BridgedNativeWidget::OnVisibilityChanged() to trigger shadow
invalidation while showing translucent windows. This fixes the shadow for the
"Restore Pages" bubble and also of the Find-In-Page window on tab switching. A
unit test is also added which demonstrates the problem.

Note that this does not fix the shadow for the Find-In-Page window when it is
displayed initially on pressing Cmd+F. 

BUG=636707, 646734
==========

[Elly Fong-Jones, 2016-09-14 14:55:04]

lgtm

Looks legit.

[karandeepb, 2016-09-14 23:42:06]

karandeepb@chromium.org changed reviewers:
+ tapted@chromium.org
- avi@chromium.org

[karandeepb, 2016-09-14 23:42:06]

Since Trent is back, adding him for owner's review.

[tapted, 2016-09-15 04:12:26]

https://codereview.chromium.org/2336983002/diff/60001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/2336983002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:844: // For translucent windows which
are made visible, recalculate shadow when the
this probably belongs in the if (layer()) block above; with the associated
comment. -- (i.e. if layer() is false, there is no compositor for a frame to
come from. But, more importantly, we might later investigate whether we should
not tie the window visibility to layer visibility, and instead rely on occlusion
APIs and keep the layer always "visible", in which case there won't be a repaint
triggered here and hence no point waiting for a frame swap that may never
happen.)

And support for widgets not backed by layers actually got removed after the
block of code above was written, so layer() should only be false during
teardown.

[karandeepb, 2016-09-15 06:30:10]

PTAL Trent.

https://codereview.chromium.org/2336983002/diff/60001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/2336983002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:844: // For translucent windows which
are made visible, recalculate shadow when the
On 2016/09/15 04:12:26, tapted (catching up) wrote:
> this probably belongs in the if (layer()) block above; with the associated
> comment. -- (i.e. if layer() is false, there is no compositor for a frame to
> come from. 
Done. I probably need to do this for the initial_visibility_suppressed_ flag
that I added for the "flash of grey" bug as well.

> But, more importantly, we might later investigate whether we should
> not tie the window visibility to layer visibility, and instead rely on
occlusion
> APIs and keep the layer always "visible", in which case there won't be a
repaint
> triggered here and hence no point waiting for a frame swap that may never
> happen.)
Why is the window and layer visibility tied up currently though? Does It save
memory/suppresses frame generation when the window is not visible? Also how
would relying on occlusion state help here? 

> And support for widgets not backed by layers actually got removed after the
> block of code above was written, so layer() should only be false during
> teardown.
I guess we should still keep the if checks since it's probably safer as some of
these codepaths can be entered during teardown?

[tapted, 2016-09-15 06:37:41]

lgtm - thanks!

https://codereview.chromium.org/2336983002/diff/60001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/2336983002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:844: // For translucent windows which
are made visible, recalculate shadow when the
On 2016/09/15 06:30:10, karandeepb wrote:
> On 2016/09/15 04:12:26, tapted (catching up) wrote:
> > this probably belongs in the if (layer()) block above; with the associated
> > comment. -- (i.e. if layer() is false, there is no compositor for a frame to
> > come from. 
> Done. I probably need to do this for the initial_visibility_suppressed_ flag
> that I added for the "flash of grey" bug as well.
> 
> > But, more importantly, we might later investigate whether we should
> > not tie the window visibility to layer visibility, and instead rely on
> occlusion
> > APIs and keep the layer always "visible", in which case there won't be a
> repaint
> > triggered here and hence no point waiting for a frame swap that may never
> > happen.)
> Why is the window and layer visibility tied up currently though? Does It save
> memory/suppresses frame generation when the window is not visible? Also how
> would relying on occlusion state help here? 

The most concrete answer is that there are tests that ensure that it is tied
together :). i.e. without the goo above, tests will fail, but it might not be
the necessary for those tests to pass on Mac. Needs some investigation.

the occlusion APIs helps with performance, e.g., by turning off animations or
increasing timer intervals -- WebContents uses them on Mac for "things".
Occlusion on Mac could help us properly support the same rationale that window
visibility is used for on other platforms, since window visibility is tricky to
pin down on Mac. 

> 
> > And support for widgets not backed by layers actually got removed after the
> > block of code above was written, so layer() should only be false during
> > teardown.
> I guess we should still keep the if checks since it's probably safer as some
of
> these codepaths can be entered during teardown?

Yeah I think so (of course setting a bool is "safe" whereas dereferencing null
is not, but conceptually these should be treated the same.

[karandeepb, 2016-09-15 07:31:04]

The CQ bit was checked by karandeepb@chromium.org

[karandeepb, 2016-09-15 07:31:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
ellyjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2336983002/#ps80001
(title: "Address review.")

[commit-bot: I haz the power, 2016-09-15 07:31:47]

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-09-15 08:06:23]

Description was changed from

==========
MacViews: Trigger shadow invalidation when a translucent window is shown.

Currently, invalidate_shadow_on_frame_swap_ is not set when a MacViews window is
shown. It is only set when the layer is created, changed or the window resized.
This causes translucent windows like the "Restore Pages" bubble and Find-In-Page
window to not have a shadow, when they are shown after being hidden.

This CL modifies BridgedNativeWidget::OnVisibilityChanged() to trigger shadow
invalidation while showing translucent windows. This fixes the shadow for the
"Restore Pages" bubble and also of the Find-In-Page window on tab switching. A
unit test is also added which demonstrates the problem.

Note that this does not fix the shadow for the Find-In-Page window when it is
displayed initially on pressing Cmd+F. 

BUG=636707, 646734
==========

to

==========
MacViews: Trigger shadow invalidation when a translucent window is shown.

Currently, invalidate_shadow_on_frame_swap_ is not set when a MacViews window is
shown. It is only set when the layer is created, changed or the window resized.
This causes translucent windows like the "Restore Pages" bubble and Find-In-Page
window to not have a shadow, when they are shown after being hidden.

This CL modifies BridgedNativeWidget::OnVisibilityChanged() to trigger shadow
invalidation while showing translucent windows. This fixes the shadow for the
"Restore Pages" bubble and also of the Find-In-Page window on tab switching. A
unit test is also added which demonstrates the problem.

Note that this does not fix the shadow for the Find-In-Page window when it is
displayed initially on pressing Cmd+F. 

BUG=636707, 646734
==========

[commit-bot: I haz the power, 2016-09-15 08:06:24]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-09-15 08:07:52]

Description was changed from

==========
MacViews: Trigger shadow invalidation when a translucent window is shown.

Currently, invalidate_shadow_on_frame_swap_ is not set when a MacViews window is
shown. It is only set when the layer is created, changed or the window resized.
This causes translucent windows like the "Restore Pages" bubble and Find-In-Page
window to not have a shadow, when they are shown after being hidden.

This CL modifies BridgedNativeWidget::OnVisibilityChanged() to trigger shadow
invalidation while showing translucent windows. This fixes the shadow for the
"Restore Pages" bubble and also of the Find-In-Page window on tab switching. A
unit test is also added which demonstrates the problem.

Note that this does not fix the shadow for the Find-In-Page window when it is
displayed initially on pressing Cmd+F. 

BUG=636707, 646734
==========

to

==========
MacViews: Trigger shadow invalidation when a translucent window is shown.

Currently, invalidate_shadow_on_frame_swap_ is not set when a MacViews window is
shown. It is only set when the layer is created, changed or the window resized.
This causes translucent windows like the "Restore Pages" bubble and Find-In-Page
window to not have a shadow, when they are shown after being hidden.

This CL modifies BridgedNativeWidget::OnVisibilityChanged() to trigger shadow
invalidation while showing translucent windows. This fixes the shadow for the
"Restore Pages" bubble and also of the Find-In-Page window on tab switching. A
unit test is also added which demonstrates the problem.

Note that this does not fix the shadow for the Find-In-Page window when it is
displayed initially on pressing Cmd+F.

BUG=636707, 646734

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

[commit-bot: I haz the power, 2016-09-15 08:07:53]

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