Add setVisibilityState API to WebFrameWidget

Add setVisibilityState API to WebFrameWidget

Currently, WebFrameWidget does not update its layerTreeHost's visibility
state when it has been changed. This causes out-of-process iframes
to fail to draw in some cases.

This adds an API to WebFrameWidget that can be called by RenderFrameImpl
when it receives a WasShown/WasHidden notification.

R=dcheng@chromium.org, japhet@chromium.org
BUG=450674
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191016

[kenrb, 2015-02-25 20:51:06]

Daniel, Nate: PTAL?

Right now when we get WasShown or WasHidden from the browser, we update the
Page's visibility state, but this doesn't find its way to the LayerTreeHost for
an out-of-process iframe which needs to be updates so that any pending
BeginFrames can proceed.

I have a FIXME in there in part because I think we need to review how we do
visibility -- we have separate notions of visibility (per-page vs per-widget)
and they get a bit confused. Most of that change will be on the Chromium side.

I will upload a Chromium patch that calls the new API.

[dcheng, 2015-02-26 17:42:08]

Mostly questions and simple things to help me understand this patch better. I'm
a little confused about the patch description as well.

It says:
> WebFrameWidget does not update its layerTreeHost's visibility state when its
size has been changed.

And then it says:
> This adds an API to WebFrameWidget that can be called by RenderFrameImpl when
it receives a WasShown/WasHidden notification.

Which seems to have nothing to do with sizing?

https://codereview.chromium.org/955143002/diff/1/Source/web/WebFrameWidgetImp...
File Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/955143002/diff/1/Source/web/WebFrameWidgetImp...
Source/web/WebFrameWidgetImpl.cpp:1022: ASSERT(visibilityState ==
WebPageVisibilityStateVisible || visibilityState == WebPageVisibilityStateHidden
|| visibilityState == WebPageVisibilityStatePrerender);
I guess the original implementation has this, but this assert seems unnecessary.
Do we usually assert on things like this? This assert just contains all the
valid enum values, which is what I would have expected to begin with =)

https://codereview.chromium.org/955143002/diff/1/Source/web/WebFrameWidgetImp...
Source/web/WebFrameWidgetImpl.cpp:1023:
m_page->setVisibilityState(static_cast<PageVisibilityState>(static_cast<int>(visibilityState)),
isInitialState);
Can we just static_cast<> directly to PageVisibilityState here?

https://codereview.chromium.org/955143002/diff/1/public/web/WebFrameWidget.h
File public/web/WebFrameWidget.h (right):

https://codereview.chromium.org/955143002/diff/1/public/web/WebFrameWidget.h#...
public/web/WebFrameWidget.h:47: // Set frame-level visibility state.
So this says frame-level state, but the enum is called page visibility state. I
think this comment should be updated to describe how the whole page visibility
thing works in conjunction with OOPIF, and why this is needed.

https://codereview.chromium.org/955143002/diff/1/public/web/WebFrameWidget.h#...
public/web/WebFrameWidget.h:49: // trickle down this notification instead?
I think this FIXME is unnecessary.

[kenrb, 2015-02-26 18:08:52]

I have corrected the CL description. The reference to sizing was a mistake --
the bug was manifesting as not drawing after the compositor had been notified of
a resize with the correct size, but the actual cause of the bug was that the
visibility state had not been correctly set.

https://codereview.chromium.org/955143002/diff/1/Source/web/WebFrameWidgetImp...
File Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/955143002/diff/1/Source/web/WebFrameWidgetImp...
Source/web/WebFrameWidgetImpl.cpp:1022: ASSERT(visibilityState ==
WebPageVisibilityStateVisible || visibilityState == WebPageVisibilityStateHidden
|| visibilityState == WebPageVisibilityStatePrerender);
On 2015/02/26 17:42:08, dcheng wrote:
> I guess the original implementation has this, but this assert seems
unnecessary.
> Do we usually assert on things like this? This assert just contains all the
> valid enum values, which is what I would have expected to begin with =)

I think it is just a guard. Looks like brettw originally suggested it, in a
review 4 years ago, and was intended to substitute for a switch.

https://codereview.chromium.org/955143002/diff/1/Source/web/WebFrameWidgetImp...
Source/web/WebFrameWidgetImpl.cpp:1023:
m_page->setVisibilityState(static_cast<PageVisibilityState>(static_cast<int>(visibilityState)),
isInitialState);
On 2015/02/26 17:42:08, dcheng wrote:
> Can we just static_cast<> directly to PageVisibilityState here?

Apparently we can. Looks like on oversight in the original code.

https://codereview.chromium.org/955143002/diff/1/public/web/WebFrameWidget.h
File public/web/WebFrameWidget.h (right):

https://codereview.chromium.org/955143002/diff/1/public/web/WebFrameWidget.h#...
public/web/WebFrameWidget.h:47: // Set frame-level visibility state.
On 2015/02/26 17:42:08, dcheng wrote:
> So this says frame-level state, but the enum is called page visibility state.
I
> think this comment should be updated to describe how the whole page visibility
> thing works in conjunction with OOPIF, and why this is needed.

Done.

https://codereview.chromium.org/955143002/diff/1/public/web/WebFrameWidget.h#...
public/web/WebFrameWidget.h:49: // trickle down this notification instead?
On 2015/02/26 17:42:08, dcheng wrote:
> I think this FIXME is unnecessary.

Ok. I think we need to invest some time to understand how visibility would work
better, but probably better just to file a bug.

[Nate Chapin, 2015-02-26 21:16:34]

Seems reasonable to me.

https://codereview.chromium.org/955143002/diff/20001/Source/web/WebFrameWidge...
File Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/955143002/diff/20001/Source/web/WebFrameWidge...
Source/web/WebFrameWidgetImpl.cpp:1022: ASSERT(visibilityState ==
WebPageVisibilityStateVisible || visibilityState == WebPageVisibilityStateHidden
|| visibilityState == WebPageVisibilityStatePrerender);
I agree this is wonky, but if there's precedent, I'll get over it.

https://codereview.chromium.org/955143002/diff/20001/Source/web/WebFrameWidge...
Source/web/WebFrameWidgetImpl.cpp:1023:
m_page->setVisibilityState(static_cast<PageVisibilityState>(visibilityState),
isInitialState);
Just to make sure I understand correctly, setVisibilityState() is only called on
one WebFrameWidget per page?

[kenrb, 2015-02-26 22:29:56]

https://codereview.chromium.org/955143002/diff/20001/Source/web/WebFrameWidge...
File Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/955143002/diff/20001/Source/web/WebFrameWidge...
Source/web/WebFrameWidgetImpl.cpp:1022: ASSERT(visibilityState ==
WebPageVisibilityStateVisible || visibilityState == WebPageVisibilityStateHidden
|| visibilityState == WebPageVisibilityStatePrerender);
On 2015/02/26 21:16:34, Nate Chapin wrote:
> I agree this is wonky, but if there's precedent, I'll get over it.

Or, I could just remove it. There is nothing I can find to suggest a really good
reason.

https://codereview.chromium.org/955143002/diff/20001/Source/web/WebFrameWidge...
Source/web/WebFrameWidgetImpl.cpp:1023:
m_page->setVisibilityState(static_cast<PageVisibilityState>(visibilityState),
isInitialState);
On 2015/02/26 21:16:34, Nate Chapin wrote:
> Just to make sure I understand correctly, setVisibilityState() is only called
on
> one WebFrameWidget per page?

Aha, no. :(

This is what I was talking about before with confusion of page and frame level
visibility state. This has the potential to cause some visual issues when you
have multiple OOPIFs in the same process -- the Page will temporarily think it
is invisible while one OOPIF is being loaded, even though another OOPIF might be
available and rendering.

Are you okay with this if I file a bug against myself to sort this out later?

[Nate Chapin, 2015-02-26 22:35:36]

On 2015/02/26 22:29:56, kenrb wrote:
>
https://codereview.chromium.org/955143002/diff/20001/Source/web/WebFrameWidge...
> File Source/web/WebFrameWidgetImpl.cpp (right):
> 
>
https://codereview.chromium.org/955143002/diff/20001/Source/web/WebFrameWidge...
> Source/web/WebFrameWidgetImpl.cpp:1022: ASSERT(visibilityState ==
> WebPageVisibilityStateVisible || visibilityState ==
WebPageVisibilityStateHidden
> || visibilityState == WebPageVisibilityStatePrerender);
> On 2015/02/26 21:16:34, Nate Chapin wrote:
> > I agree this is wonky, but if there's precedent, I'll get over it.
> 
> Or, I could just remove it. There is nothing I can find to suggest a really
good
> reason.
> 
>
https://codereview.chromium.org/955143002/diff/20001/Source/web/WebFrameWidge...
> Source/web/WebFrameWidgetImpl.cpp:1023:
> m_page->setVisibilityState(static_cast<PageVisibilityState>(visibilityState),
> isInitialState);
> On 2015/02/26 21:16:34, Nate Chapin wrote:
> > Just to make sure I understand correctly, setVisibilityState() is only
called
> on
> > one WebFrameWidget per page?
> 
> Aha, no. :(
> 
> This is what I was talking about before with confusion of page and frame level
> visibility state. This has the potential to cause some visual issues when you
> have multiple OOPIFs in the same process -- the Page will temporarily think it
> is invisible while one OOPIF is being loaded, even though another OOPIF might
be
> available and rendering.
> 
> Are you okay with this if I file a bug against myself to sort this out later?

Yeah, that LGTM. We should probably add a FIXME to go along with it.

[dcheng, 2015-02-26 22:40:30]

lgtm, adding a FIXME inside WebFrameWidgetImpl is probably a good idea though.

https://codereview.chromium.org/955143002/diff/40001/public/web/WebFrameWidget.h
File public/web/WebFrameWidget.h (right):

https://codereview.chromium.org/955143002/diff/40001/public/web/WebFrameWidge...
public/web/WebFrameWidget.h:47: // Set frame-level visibility state.
This comment isn't entirely accurate. I'd probably just remove it until we
figure out our story for page vs widget visibility during loads.

[kenrb, 2015-02-27 16:27:21]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2015-02-27 16:27:22]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/955143002/#ps60001 (title:
"Comments updated")

[commit-bot: I haz the power, 2015-02-27 16:28:36]

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

[dcheng, 2015-02-27 16:28:49]

https://codereview.chromium.org/955143002/diff/40001/public/web/WebFrameWidget.h
File public/web/WebFrameWidget.h (right):

https://codereview.chromium.org/955143002/diff/40001/public/web/WebFrameWidge...
public/web/WebFrameWidget.h:47: // Set frame-level visibility state.
On 2015/02/26 22:40:30, dcheng wrote:
> This comment isn't entirely accurate. I'd probably just remove it until we
> figure out our story for page vs widget visibility during loads.

I meant line 47 (set frame-level visibility state) rather than the paragraph
below =)

[kenrb, 2015-02-27 16:39:32]

The CQ bit was unchecked by kenrb@chromium.org

[kenrb, 2015-02-27 16:44:36]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2015-02-27 16:44:37]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/955143002/#ps80001 (title:
"Another comment update")

[commit-bot: I haz the power, 2015-02-27 16:45:14]

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

[commit-bot: I haz the power, 2015-02-27 17:56:21]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191016