Fire visibilityChange event on out-of-process iframes.

Fire visibilityChange event on out-of-process iframes.

Design doc: https://docs.google.com/document/d/1qdlgaC277SnnmQv7hoNFTGp-OCvokEc4mA2NmAzOyBU/edit?usp=sharing

BUG=550967
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/7171540721a57cf5926709a277e5d70b09965643
Cr-Commit-Position: refs/heads/master@{#403176}

[lfg, 2016-05-19 21:29:42]

Description was changed from

==========
Fire visibilityChange event on out-of-process iframes.

BUG=550967
==========

to

==========
Fire visibilityChange event on out-of-process iframes.

BUG=550967
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[lfg, 2016-05-19 23:56:32]

lfg@chromium.org changed reviewers:
+ dcheng@chromium.org

[lfg, 2016-05-19 23:56:32]

Daniel, can you do a first pass review?

[lfg, 2016-05-19 23:58:00]

Description was changed from

==========
Fire visibilityChange event on out-of-process iframes.

BUG=550967
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fire visibilityChange event on out-of-process iframes.

BUG=550967
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[lfg, 2016-05-24 17:57:12]

lfg@chromium.org changed reviewers:
+ kenrb@chromium.org

[lfg, 2016-05-24 17:57:12]

Ken, can you do an initial review (just to lighten the load on Daniel)?

[kenrb, 2016-05-24 20:25:18]

Please add a CL description, I don't entirely understand everything you are
doing here (hence, lots of questions in the comments), and a summary would help.

Also, please add a test for the new page messages.

https://codereview.chromium.org/1991273003/diff/1/content/common/page_messages.h
File content/common/page_messages.h (right):

https://codereview.chromium.org/1991273003/diff/1/content/common/page_message...
content/common/page_messages.h:33: // Informs the renderer the the page is not
longer hidden.
nit: "the the" -> "that the", "not longer" -> "no longer"

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4401: if (IsLocalRoot() &&
GetWebFrame()->frameWidget()) {
Can GetWebFrame() return nullptr here, if this is called after frame detach?

Also, I think a non-null frameWidget() implies this is a local root, so the
IsLocalRoot() check is unnecessary.

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.h:675: blink::WebPageVisibilityState
GetVisibilityState() const override;
This should be into the RenderFrame implementation block.

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2789: GetMainRenderFrame() ?
GetMainRenderFrame()->visibilityState()
Why do we defer to the main frame's visibility state for the RenderViewImpl in
the top-level frame's renderer? It looks like they should always match, since
the top RWHV will get a Hide message immediately before the PageMsg_WasHidden,
but in future when RenderWidget is separated from RenderView this seems like it
would be wrong.

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3083: // This method should only be used to
determine initial visibility.
This comment isn't very clear, what does 'initial visibility' mean exactly? And
should the comment be moved to the declaration in WebViewClient.h, since it is
relevant to callers of the method?

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/Page.cpp (right):

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/Page.cpp:362:
m_mainFrame->didChangeVisibilityState();
How does this not crash in an OOPIF renderer?

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:1076: if (m_layerTreeView)
Why is it okay to remove the call to WebFrameScheduler::setPageVisible()?

[kenrb, 2016-05-24 20:33:21]

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/Page.cpp (right):

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/Page.cpp:362:
m_mainFrame->didChangeVisibilityState();
On 2016/05/24 20:25:18, kenrb wrote:
> How does this not crash in an OOPIF renderer?

Never mind, I just looked at this a bit closer. This ends up walking the entire
frame tree, whether the main frame is local or not. This is probably fine.

[dcheng, 2016-05-24 20:38:24]

Also, agree with Ken's comments about updating the CL description to provide an
overview of this CL.

https://codereview.chromium.org/1991273003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1991273003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1348: SendPageMessage(new
PageMsg_WasHidden(MSG_ROUTING_NONE));
MSG_ROUTING_NONE? This looks weird. Either it should be a control message or we
should attach a real routing ID.

https://codereview.chromium.org/1991273003/diff/1/content/common/page_messages.h
File content/common/page_messages.h (right):

https://codereview.chromium.org/1991273003/diff/1/content/common/page_message...
content/common/page_messages.h:31: IPC_MESSAGE_ROUTED0(PageMsg_WasHidden)
I know the existing messages are called ViewMsg_WasHidden, but I think the
general naming convention is TypeMsg_DidX and TypeMsg_DidY.

https://codereview.chromium.org/1991273003/diff/1/content/public/renderer/ren...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/1991273003/diff/1/content/public/renderer/ren...
content/public/renderer/render_frame.h:207: // Returns the current visibility of
the WebView.
Frame?

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4387: if (IsLocalRoot() &&
GetWebFrame()->frameWidget()) {
Why do we need both checks here? I would expect one or the other to be
sufficient.

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1820: while
(localRoot->parent() && localRoot->parent()->isWebLocalFrame())
Why do we have to change this line?

[lfg, 2016-05-24 23:21:43]

Patchset #2 (id:20001) has been deleted

[lfg, 2016-05-24 23:33:51]

Please take another look. I realize a lot of this stuff is not obvious, I tried
to address the comments and I wrote a design doc about this:
https://docs.google.com/document/d/1qdlgaC277SnnmQv7hoNFTGp-OCvokEc4mA2NmAzOy...

Ken: what do you mean by adding a page test? I've added a site isolation
browsertest that tests that the OOPIF renderers receive the message, do you have
something else in mind?

https://codereview.chromium.org/1991273003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1991273003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1348: SendPageMessage(new
PageMsg_WasHidden(MSG_ROUTING_NONE));
On 2016/05/24 20:38:23, dcheng wrote:
> MSG_ROUTING_NONE? This looks weird. Either it should be a control message or
we
> should attach a real routing ID.

SendPageMessage() will attach the appropriate routing IDs (and we can't create
routed message classes without providing a routing ID).

https://codereview.chromium.org/1991273003/diff/1/content/common/page_messages.h
File content/common/page_messages.h (right):

https://codereview.chromium.org/1991273003/diff/1/content/common/page_message...
content/common/page_messages.h:31: IPC_MESSAGE_ROUTED0(PageMsg_WasHidden)
On 2016/05/24 20:38:23, dcheng wrote:
> I know the existing messages are called ViewMsg_WasHidden, but I think the
> general naming convention is TypeMsg_DidX and TypeMsg_DidY.

I could change that to a single message that has a bool parameter like
PageMsg_DidChangeVisibilityState, WDYT?

https://codereview.chromium.org/1991273003/diff/1/content/common/page_message...
content/common/page_messages.h:33: // Informs the renderer the the page is not
longer hidden.
On 2016/05/24 20:25:18, kenrb wrote:
> nit: "the the" -> "that the", "not longer" -> "no longer"

Done.

https://codereview.chromium.org/1991273003/diff/1/content/public/renderer/ren...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/1991273003/diff/1/content/public/renderer/ren...
content/public/renderer/render_frame.h:207: // Returns the current visibility of
the WebView.
On 2016/05/24 20:38:24, dcheng wrote:
> Frame?

Done.

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4387: if (IsLocalRoot() &&
GetWebFrame()->frameWidget()) {
On 2016/05/24 20:38:24, dcheng wrote:
> Why do we need both checks here? I would expect one or the other to be
> sufficient.

Removed.

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4401: if (IsLocalRoot() &&
GetWebFrame()->frameWidget()) {
On 2016/05/24 20:25:18, kenrb wrote:
> Can GetWebFrame() return nullptr here, if this is called after frame detach?

No, the RenderFrameImpl is synchronously deleted in frameDetached(). Maybe if
this was called during detach, but I don't see why that would happen.

> Also, I think a non-null frameWidget() implies this is a local root, so the
> IsLocalRoot() check is unnecessary.

I can't remember why I had it like this, but I agree that it should be
unnecessary. Removed.

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.h:675: blink::WebPageVisibilityState
GetVisibilityState() const override;
On 2016/05/24 20:25:18, kenrb wrote:
> This should be into the RenderFrame implementation block.

Done.

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2789: GetMainRenderFrame() ?
GetMainRenderFrame()->visibilityState()
On 2016/05/24 20:25:18, kenrb wrote:
> Why do we defer to the main frame's visibility state for the RenderViewImpl in
> the top-level frame's renderer? It looks like they should always match, since
> the top RWHV will get a Hide message immediately before the PageMsg_WasHidden,
> but in future when RenderWidget is separated from RenderView this seems like
it
> would be wrong.

This is a bit tricky to fix, it's not as simple as tracking separately the
visibility for the view. The only thing that overrides the visibilityState is
prerender, which does by looking up the frame, and I can't fully split the
visibility without fixing prerender to make it OOPIF-aware.

Note that I just moved the code that was implicitly done in
RenderViewImpl::visibilityState() before. Would it be ok to just add a TODO
here?

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3083: // This method should only be used to
determine initial visibility.
On 2016/05/24 20:25:18, kenrb wrote:
> This comment isn't very clear, what does 'initial visibility' mean exactly?
And
> should the comment be moved to the declaration in WebViewClient.h, since it is
> relevant to callers of the method?

Perhaps it should be moved. When the WebViewImpl is created, there's no main
frame created yet, and we must provide the WebView with an initial
WebPageVisibilityState. This initial visibility is set by calling
WebViewImpl::setVisibilityState() with 'true' on the second argument. Subsequent
calls to setVisibilityState() will only trigger a visibilityChange event if the
new visibility state is different from the previous one.

I was afraid of changing the initial visibility to be only executed after the
main frame is created. since there's a lot of stuff that happens between those
two states, but I could give it a try. WDYT?

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/Page.cpp (right):

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/Page.cpp:362:
m_mainFrame->didChangeVisibilityState();
On 2016/05/24 20:33:20, kenrb wrote:
> On 2016/05/24 20:25:18, kenrb wrote:
> > How does this not crash in an OOPIF renderer?
> 
> Never mind, I just looked at this a bit closer. This ends up walking the
entire
> frame tree, whether the main frame is local or not. This is probably fine.

Right, Daniel moved that block from LocalFrame to Frame already.

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:1076: if (m_layerTreeView)
On 2016/05/24 20:25:18, kenrb wrote:
> Why is it okay to remove the call to WebFrameScheduler::setPageVisible()?

This is already done in WebViewSchedulerImpl::setPageVisible, which iterates
over all LocalFrames in the tree and updates its visibility.
WebViewSchedulerImpl::setPageVisible is called when setting page visibility.

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1820: while
(localRoot->parent() && localRoot->parent()->isWebLocalFrame())
On 2016/05/24 20:38:24, dcheng wrote:
> Why do we have to change this line?

After the renderer receives a ViewMsg_Close, the WebFrameWidget is deleted and
this method starts crashing.

[lfg, 2016-05-24 23:34:02]

Description was changed from

==========
Fire visibilityChange event on out-of-process iframes.

BUG=550967
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fire visibilityChange event on out-of-process iframes.

Design doc:
https://docs.google.com/document/d/1qdlgaC277SnnmQv7hoNFTGp-OCvokEc4mA2NmAzOy...

BUG=550967
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[kenrb, 2016-05-25 20:29:14]

This is basically fine from my perspective, just a couple more minor comments.

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2789: GetMainRenderFrame() ?
GetMainRenderFrame()->visibilityState()
On 2016/05/24 23:33:50, lfg wrote:
> On 2016/05/24 20:25:18, kenrb wrote:
> > Why do we defer to the main frame's visibility state for the RenderViewImpl
in
> > the top-level frame's renderer? It looks like they should always match,
since
> > the top RWHV will get a Hide message immediately before the
PageMsg_WasHidden,
> > but in future when RenderWidget is separated from RenderView this seems like
> it
> > would be wrong.
> 
> This is a bit tricky to fix, it's not as simple as tracking separately the
> visibility for the view. The only thing that overrides the visibilityState is
> prerender, which does by looking up the frame, and I can't fully split the
> visibility without fixing prerender to make it OOPIF-aware.
> 
> Note that I just moved the code that was implicitly done in
> RenderViewImpl::visibilityState() before. Would it be ok to just add a TODO
> here?

I agree it should work fine as is, but I think this becomes a race condition
when RenderWidget and RenderView get split. It is reasonable to file a bug for
this and make it a blocker for that work (if you I'm right about the race).

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3083: // This method should only be used to
determine initial visibility.
On 2016/05/24 23:33:51, lfg wrote:
> On 2016/05/24 20:25:18, kenrb wrote:
> > This comment isn't very clear, what does 'initial visibility' mean exactly?
> And
> > should the comment be moved to the declaration in WebViewClient.h, since it
is
> > relevant to callers of the method?
> 
> Perhaps it should be moved. When the WebViewImpl is created, there's no main
> frame created yet, and we must provide the WebView with an initial
> WebPageVisibilityState. This initial visibility is set by calling
> WebViewImpl::setVisibilityState() with 'true' on the second argument.
Subsequent
> calls to setVisibilityState() will only trigger a visibilityChange event if
the
> new visibility state is different from the previous one.
> 
> I was afraid of changing the initial visibility to be only executed after the
> main frame is created. since there's a lot of stuff that happens between those
> two states, but I could give it a try. WDYT?

Now, that I understand this better, I'm not sure if there is a good reason to
change that. Would it simplify anything?

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/Page.cpp (right):

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/Page.cpp:362:
m_mainFrame->didChangeVisibilityState();
On 2016/05/24 23:33:51, lfg wrote:
> On 2016/05/24 20:33:20, kenrb wrote:
> > On 2016/05/24 20:25:18, kenrb wrote:
> > > How does this not crash in an OOPIF renderer?
> > 
> > Never mind, I just looked at this a bit closer. This ends up walking the
> entire
> > frame tree, whether the main frame is local or not. This is probably fine.
> 
> Right, Daniel moved that block from LocalFrame to Frame already.

As an aside, related to a discussion we had on this earlier, it doesn't look
like didChangeVisibilityState() triggers RemoteFrame::visibilityChanged(), which
sends the IPC to the browser process. That is a different condition.

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:1076: if (m_layerTreeView)
On 2016/05/24 23:33:51, lfg wrote:
> On 2016/05/24 20:25:18, kenrb wrote:
> > Why is it okay to remove the call to WebFrameScheduler::setPageVisible()?
> 
> This is already done in WebViewSchedulerImpl::setPageVisible, which iterates
> over all LocalFrames in the tree and updates its visibility.
> WebViewSchedulerImpl::setPageVisible is called when setting page visibility.

Acknowledged. This is fine, although I notice that there is a
WebFrameScheduler::setFrameVisible() that currently has no callers. I assume
that is a work in progress by the scheduler people.

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebFrameClient.h:714: // Visibility
-----------------------------------------------------------
Nit: there is one more hyphen in this line than the others. :)

[lfg, 2016-05-31 19:54:01]

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2789: GetMainRenderFrame() ?
GetMainRenderFrame()->visibilityState()
On 2016/05/25 20:29:14, kenrb wrote:
> On 2016/05/24 23:33:50, lfg wrote:
> > On 2016/05/24 20:25:18, kenrb wrote:
> > > Why do we defer to the main frame's visibility state for the
RenderViewImpl
> in
> > > the top-level frame's renderer? It looks like they should always match,
> since
> > > the top RWHV will get a Hide message immediately before the
> PageMsg_WasHidden,
> > > but in future when RenderWidget is separated from RenderView this seems
like
> > it
> > > would be wrong.
> > 
> > This is a bit tricky to fix, it's not as simple as tracking separately the
> > visibility for the view. The only thing that overrides the visibilityState
is
> > prerender, which does by looking up the frame, and I can't fully split the
> > visibility without fixing prerender to make it OOPIF-aware.
> > 
> > Note that I just moved the code that was implicitly done in
> > RenderViewImpl::visibilityState() before. Would it be ok to just add a TODO
> > here?
> 
> I agree it should work fine as is, but I think this becomes a race condition
> when RenderWidget and RenderView get split. It is reasonable to file a bug for
> this and make it a blocker for that work (if you I'm right about the race).

I've added a TODO with a link to the prerendering bug, since fixing prerendering
is the proper way of handling this.

https://codereview.chromium.org/1991273003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3083: // This method should only be used to
determine initial visibility.
On 2016/05/25 20:29:14, kenrb wrote:
> On 2016/05/24 23:33:51, lfg wrote:
> > On 2016/05/24 20:25:18, kenrb wrote:
> > > This comment isn't very clear, what does 'initial visibility' mean
exactly?
> > And
> > > should the comment be moved to the declaration in WebViewClient.h, since
it
> is
> > > relevant to callers of the method?
> > 
> > Perhaps it should be moved. When the WebViewImpl is created, there's no main
> > frame created yet, and we must provide the WebView with an initial
> > WebPageVisibilityState. This initial visibility is set by calling
> > WebViewImpl::setVisibilityState() with 'true' on the second argument.
> Subsequent
> > calls to setVisibilityState() will only trigger a visibilityChange event if
> the
> > new visibility state is different from the previous one.
> > 
> > I was afraid of changing the initial visibility to be only executed after
the
> > main frame is created. since there's a lot of stuff that happens between
those
> > two states, but I could give it a try. WDYT?
> 
> Now, that I understand this better, I'm not sure if there is a good reason to
> change that. Would it simplify anything?

Yes, we would be able to remove visibilityState() from WebViewClient. I'll try
to do that, but I'll do in a separate patch.

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/Page.cpp (right):

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/Page.cpp:362:
m_mainFrame->didChangeVisibilityState();
On 2016/05/25 20:29:14, kenrb wrote:
> On 2016/05/24 23:33:51, lfg wrote:
> > On 2016/05/24 20:33:20, kenrb wrote:
> > > On 2016/05/24 20:25:18, kenrb wrote:
> > > > How does this not crash in an OOPIF renderer?
> > > 
> > > Never mind, I just looked at this a bit closer. This ends up walking the
> > entire
> > > frame tree, whether the main frame is local or not. This is probably fine.
> > 
> > Right, Daniel moved that block from LocalFrame to Frame already.
> 
> As an aside, related to a discussion we had on this earlier, it doesn't look
> like didChangeVisibilityState() triggers RemoteFrame::visibilityChanged(),
which
> sends the IPC to the browser process. That is a different condition.

Acknowledged.

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:1076: if (m_layerTreeView)
On 2016/05/25 20:29:14, kenrb wrote:
> Acknowledged. This is fine, although I notice that there is a
> WebFrameScheduler::setFrameVisible() that currently has no callers. I assume
> that is a work in progress by the scheduler people.

That's right. However, if we do that here, the correct way to do it is to
iterate over all LocalFrames in this local root.

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1991273003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebFrameClient.h:714: // Visibility
-----------------------------------------------------------
On 2016/05/25 20:29:14, kenrb wrote:
> Nit: there is one more hyphen in this line than the others. :)

Done.

[kenrb, 2016-06-01 20:22:51]

lgtm from my side.

[dcheng, 2016-06-02 22:16:07]

https://codereview.chromium.org/1991273003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1991273003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:716: // Returns the current
visibility of the WebView.
WebView?

[lfg, 2016-06-03 18:34:58]

https://codereview.chromium.org/1991273003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1991273003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:716: // Returns the current
visibility of the WebView.
On 2016/06/02 22:16:07, dcheng wrote:
> WebView?

Oops. I fixed that in render_frame.h but forgot to fix it here. Done.

[lfg, 2016-06-03 18:35:23]

lfg@chromium.org changed reviewers:
+ creis@chromium.org

[lfg, 2016-06-03 18:35:24]

creis@chromium.org: Please review changes in 

content/

[dcheng, 2016-06-03 21:22:51]

dcheng@chromium.org changed reviewers:
- creis@chromium.org

[dcheng, 2016-06-03 21:22:52]

https://codereview.chromium.org/1991273003/diff/140001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1991273003/diff/140001/content/renderer/rende...
content/renderer/render_view_impl.cc:3091: // This method should only be used to
determine initial visibility.
Is there any chance we can just pass this value as a parameter to
WebView::create()?

[lfg, 2016-06-03 21:41:36]

On 2016/06/03 21:22:52, dcheng wrote:
>
https://codereview.chromium.org/1991273003/diff/140001/content/renderer/rende...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/1991273003/diff/140001/content/renderer/rende...
> content/renderer/render_view_impl.cc:3091: // This method should only be used
to
> determine initial visibility.
> Is there any chance we can just pass this value as a parameter to
> WebView::create()?

Yes, I think that's doable, however, we would have to change the signature of
WebView::create(), which is used in quite a few places (26 according to code
search). I'm a bit torn on this, since it would change how things are done
(currently, we only pass the client and the client answers for everything), but
I think passing the initial visibility on creation makes sense. WDYT?

[lfg, 2016-06-03 21:43:10]

lfg@chromium.org changed reviewers:
+ creis@chromium.org

[lfg, 2016-06-03 21:43:11]

+creis for real.

[dcheng, 2016-06-04 04:58:01]

On 2016/06/03 at 21:41:36, lfg wrote:
> On 2016/06/03 21:22:52, dcheng wrote:
> >
https://codereview.chromium.org/1991273003/diff/140001/content/renderer/rende...
> > File content/renderer/render_view_impl.cc (right):
> > 
> >
https://codereview.chromium.org/1991273003/diff/140001/content/renderer/rende...
> > content/renderer/render_view_impl.cc:3091: // This method should only be
used to
> > determine initial visibility.
> > Is there any chance we can just pass this value as a parameter to
> > WebView::create()?
> 
> Yes, I think that's doable, however, we would have to change the signature of
WebView::create(), which is used in quite a few places (26 according to code
search). I'm a bit torn on this, since it would change how things are done
(currently, we only pass the client and the client answers for everything), but
I think passing the initial visibility on creation makes sense. WDYT?

I think we should pass it on creation; having it here but then saying that it
should only be used for the initial state is something that can easily be
overlooked.

We could always use a default arg ;-)

[dcheng, 2016-06-04 04:58:21]

On 2016/06/04 at 04:58:01, dcheng wrote:
> On 2016/06/03 at 21:41:36, lfg wrote:
> > On 2016/06/03 21:22:52, dcheng wrote:
> > >
https://codereview.chromium.org/1991273003/diff/140001/content/renderer/rende...
> > > File content/renderer/render_view_impl.cc (right):
> > > 
> > >
https://codereview.chromium.org/1991273003/diff/140001/content/renderer/rende...
> > > content/renderer/render_view_impl.cc:3091: // This method should only be
used to
> > > determine initial visibility.
> > > Is there any chance we can just pass this value as a parameter to
> > > WebView::create()?
> > 
> > Yes, I think that's doable, however, we would have to change the signature
of WebView::create(), which is used in quite a few places (26 according to code
search). I'm a bit torn on this, since it would change how things are done
(currently, we only pass the client and the client answers for everything), but
I think passing the initial visibility on creation makes sense. WDYT?
> 
> I think we should pass it on creation; having it here but then saying that it
should only be used for the initial state is something that can easily be
overlooked.
> 
> We could always use a default arg ;-)

(Though I would personally just fix the callsites, 26 is not too bad, and most
are concentrated in a few test files)

[lfg, 2016-06-06 21:49:43]

On 2016/06/04 04:58:21, dcheng wrote:
> > I think we should pass it on creation; having it here but then saying that
it
> should only be used for the initial state is something that can easily be
> overlooked.
> > 
> > We could always use a default arg ;-)
> 
> (Though I would personally just fix the callsites, 26 is not too bad, and most
> are concentrated in a few test files)

Done. Please take another look.

[Charlie Reis, 2016-06-06 23:15:07]

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

[Charlie Reis, 2016-06-06 23:15:09]

content/ seems mostly ok to me, with a few clarifications.  Adding avi@ to also
take a look at RenderView/RenderFrame, since he has a lot of TODOs affected
there.

https://codereview.chromium.org/1991273003/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1991273003/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1772:
delegate_->IsHidden());
This scares me a bit, but maybe it's ok.

1) We're doing this in CreateRenderFrameProxy.  Is it ok to tell the RVH it's
visible even if it has no visible frames in it?

2) RFHMDelegate::IsHidden() could be a bit of a broken API, and seems to date
back before we moved RVHM to RFHM.  It returns WebContentsImpl::IsHidden.  Is
that the right value to use in all frames?

https://codereview.chromium.org/1991273003/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1991273003/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1314: SendPageMessage(new
PageMsg_WasShown(MSG_ROUTING_NONE));
Is the order of this method important?  It looks arbitrary at first glance, so
maybe mention if this needs to happen before or after the other things.

https://codereview.chromium.org/1991273003/diff/180001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/1991273003/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl.cc:979: // frame-level visibility.
Did this turn out to be wrong?  I see the RegisterRenderFrame call is still
there.

https://codereview.chromium.org/1991273003/diff/180001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1991273003/diff/180001/content/renderer/rende...
content/renderer/render_view_impl.cc:2797: // prerendering must be made aware of
OOPIFs. https://crbug.com/440544
Can you post an update to that bug about this context?  There's not much info on
the bug itself about what work needs to be done.

https://codereview.chromium.org/1991273003/diff/180001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1991273003/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebView.h:130: BLINK_EXPORT static WebView*
create(WebViewClient*, bool);
This seems like it needs some explanation.  (At first glance, I'm not sure if it
means visible or hidden, even knowing the context of this CL.  And worse, it
means visible, but we use hidden in other places like
FrameTree::CreateRenderViewHost.)

[Avi (use Gerrit), 2016-06-07 01:41:53]

All the TODOs affected had my name attached to them by dcheng. Daniel has more
experience on those points than I do.

[dcheng, 2016-06-07 05:28:57]

As for the //content changes, the main TODO I saw removed is in
RenderFrameImpl::WasShown, and the changes there make sense to me (since it also
matches WasHidden now).

https://codereview.chromium.org/1991273003/diff/180001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1991273003/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebView.h:130: BLINK_EXPORT static WebView*
create(WebViewClient*, bool);
On 2016/06/06 at 23:15:09, Charlie Reis wrote:
> This seems like it needs some explanation.  (At first glance, I'm not sure if
it means visible or hidden, even knowing the context of this CL.  And worse, it
means visible, but we use hidden in other places like
FrameTree::CreateRenderViewHost.)

We should just plumb WebPageVisibilityState through here.

[lfg, 2016-06-08 19:54:15]

Patchset #10 (id:200001) has been deleted

[lfg, 2016-06-08 20:00:24]

https://codereview.chromium.org/1991273003/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1991273003/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1772:
delegate_->IsHidden());
On 2016/06/06 23:15:09, Charlie Reis wrote:
> This scares me a bit, but maybe it's ok.
> 
> 1) We're doing this in CreateRenderFrameProxy.  Is it ok to tell the RVH it's
> visible even if it has no visible frames in it?
> 
> 2) RFHMDelegate::IsHidden() could be a bit of a broken API, and seems to date
> back before we moved RVHM to RFHM.  It returns WebContentsImpl::IsHidden.  Is
> that the right value to use in all frames?

That's right, the page visibility is determined by the WebContentsImpl, and is
passed to the RenderViewImpl on creation. This change just passes it through
during the creation of the RVH, so it's forwarded to the renderer and the
correct visibility is set when creating the RenderViewImpl (on the subframe
renderer).

https://codereview.chromium.org/1991273003/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1991273003/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1314: SendPageMessage(new
PageMsg_WasShown(MSG_ROUTING_NONE));
On 2016/06/06 23:15:09, Charlie Reis wrote:
> Is the order of this method important?  It looks arbitrary at first glance, so
> maybe mention if this needs to happen before or after the other things.

The order is not important right now (and I don't think it should ever be
important). We could swap the order and everything should still work.

https://codereview.chromium.org/1991273003/diff/180001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/1991273003/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl.cc:979: // frame-level visibility.
On 2016/06/06 23:15:09, Charlie Reis wrote:
> Did this turn out to be wrong?  I see the RegisterRenderFrame call is still
> there.

Yes, RegisterRenderFrame() is used for many things other the visibility  (look
for FOR_EACH_OBSERVER in render_widget.cc).

I believe that the part that this comment refers to is the hackish code in
RenderFrameImpl::WasShown, which forced the subframe to be set to visible. That
part is cleaned up.

With this patch, RenderFrameImpl still needs to observe WasShown/WasHidden in
order to send the events to plugins inside the frame.

https://codereview.chromium.org/1991273003/diff/180001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1991273003/diff/180001/content/renderer/rende...
content/renderer/render_view_impl.cc:2797: // prerendering must be made aware of
OOPIFs. https://crbug.com/440544
On 2016/06/06 23:15:09, Charlie Reis wrote:
> Can you post an update to that bug about this context?  There's not much info
on
> the bug itself about what work needs to be done.

Done.

https://codereview.chromium.org/1991273003/diff/180001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1991273003/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebView.h:130: BLINK_EXPORT static WebView*
create(WebViewClient*, bool);
On 2016/06/06 23:15:09, Charlie Reis wrote:
> This seems like it needs some explanation.  (At first glance, I'm not sure if
it
> means visible or hidden, even knowing the context of this CL.  And worse, it
> means visible, but we use hidden in other places like
> FrameTree::CreateRenderViewHost.)

Done.

[Charlie Reis, 2016-06-08 21:39:07]

Thanks, LGTM.  I'm excited to see so many TODO's and FIXME's going away!

One sanity check below about whether we get a WasHidden for OOPIF RenderViews
when their last local subframe swaps out.

https://codereview.chromium.org/1991273003/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1991273003/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1772:
delegate_->IsHidden());
On 2016/06/08 20:00:22, lfg (OOO until Jun 25) wrote:
> On 2016/06/06 23:15:09, Charlie Reis wrote:
> > This scares me a bit, but maybe it's ok.
> > 
> > 1) We're doing this in CreateRenderFrameProxy.  Is it ok to tell the RVH
it's
> > visible even if it has no visible frames in it?
> > 
> > 2) RFHMDelegate::IsHidden() could be a bit of a broken API, and seems to
date
> > back before we moved RVHM to RFHM.  It returns WebContentsImpl::IsHidden. 
Is
> > that the right value to use in all frames?
> 
> That's right, the page visibility is determined by the WebContentsImpl, and is
> passed to the RenderViewImpl on creation. This change just passes it through
> during the creation of the RVH, so it's forwarded to the renderer and the
> correct visibility is set when creating the RenderViewImpl (on the subframe
> renderer).

Acknowledged.

https://codereview.chromium.org/1991273003/diff/180001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/1991273003/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl.cc:979: // frame-level visibility.
On 2016/06/08 20:00:24, lfg (OOO until Jun 25) wrote:
> On 2016/06/06 23:15:09, Charlie Reis wrote:
> > Did this turn out to be wrong?  I see the RegisterRenderFrame call is still
> > there.
> 
> Yes, RegisterRenderFrame() is used for many things other the visibility  (look
> for FOR_EACH_OBSERVER in render_widget.cc).
> 
> I believe that the part that this comment refers to is the hackish code in
> RenderFrameImpl::WasShown, which forced the subframe to be set to visible.
That
> part is cleaned up.
> 
> With this patch, RenderFrameImpl still needs to observe WasShown/WasHidden in
> order to send the events to plugins inside the frame.

Acknowledged.

https://codereview.chromium.org/1991273003/diff/240001/content/renderer/media...
File content/renderer/media/android/media_info_loader_unittest.cc (right):

https://codereview.chromium.org/1991273003/diff/240001/content/renderer/media...
content/renderer/media/android/media_info_loader_unittest.cc:42: :
view_(WebView::create(nullptr, true)),
blink::WebPageVisibilityStateVisible?

https://codereview.chromium.org/1991273003/diff/240001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/1991273003/diff/240001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1635: // TODO(creis): Support this for
subframes as well.
Hopefully this happens via another path (e.g., a WasHidden IPC) if the last
local subframe gets swapped out but there are still proxies keeping the
RenderView around?

[lfg, 2016-06-08 21:53:50]

https://codereview.chromium.org/1991273003/diff/240001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/1991273003/diff/240001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1635: // TODO(creis): Support this for
subframes as well.
On 2016/06/08 21:39:07, Charlie Reis wrote:
> Hopefully this happens via another path (e.g., a WasHidden IPC) if the last
> local subframe gets swapped out but there are still proxies keeping the
> RenderView around?

There's actually nothing that would handle this. How can we keep the RenderView
(and therefore the WebView) alive if all the local subframes are gone?

The downside is that in the meantime we'll be using a bit more resources than
needed, but we should clean everything up when the WebView/WebWidget split is
completed.

[Charlie Reis, 2016-06-08 22:18:35]

https://codereview.chromium.org/1991273003/diff/240001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/1991273003/diff/240001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1635: // TODO(creis): Support this for
subframes as well.
On 2016/06/08 21:53:50, lfg (OOO until Jun 25) wrote:
> On 2016/06/08 21:39:07, Charlie Reis wrote:
> > Hopefully this happens via another path (e.g., a WasHidden IPC) if the last
> > local subframe gets swapped out but there are still proxies keeping the
> > RenderView around?
> 
> There's actually nothing that would handle this. How can we keep the
RenderView
> (and therefore the WebView) alive if all the local subframes are gone?

This happens if you have proxies due to a reference from another tab.  For
example, a LocalFrame in one window could have a tree of RemoteFrames in its
opener window.

> The downside is that in the meantime we'll be using a bit more resources than
> needed, but we should clean everything up when the WebView/WebWidget split is
> completed.

It'd be nice to solve it soon, but I'm not too concerned about it.  That said,
maybe we should leave a more detailed TODO here instead of removing it.

[lfg, 2016-06-08 22:47:33]

lfg@chromium.org changed reviewers:
+ piman@chromium.org

[lfg, 2016-06-08 22:47:34]

+piman: Can you comment on how much resources we would be wasting by having the
RenderWidgetCompositor (and therefore the LayerTreeHost) visible, but never
rendering it?

https://codereview.chromium.org/1991273003/diff/240001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/1991273003/diff/240001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1635: // TODO(creis): Support this for
subframes as well.
On 2016/06/08 22:18:35, Charlie Reis wrote:
> 
> This happens if you have proxies due to a reference from another tab.  For
> example, a LocalFrame in one window could have a tree of RemoteFrames in its
> opener window.
> 
> It'd be nice to solve it soon, but I'm not too concerned about it.  That said,
> maybe we should leave a more detailed TODO here instead of removing it.

I see. I think the correct way to do this, is to destroy the WebWidget on swap,
which I believe we already do for subframes (and we should do the same for the
main frame once we split WebView/WebWidget).

Actually, now that I think about this, I think this code should be removed. This
is not the ideal scenario, and we are wasting some resources for now, but
setting the visibility to Hidden in the webview has unwanted consequences, for
instance, the WebViewScheduler will set every local frame to Hidden, regardless
if it's the same local root or not, which is wrong.

The only thing I'm a bit worried here is how much we are wasting in terms of
resources by setting the RenderWidgetCompositor to visible, even though we'll
never draw to this widget.

[dcheng, 2016-06-09 07:15:18]

blink lgtm

[piman, 2016-06-09 19:20:56]

On Wed, Jun 8, 2016 at 3:47 PM, <lfg@chromium.org> wrote:

> +piman: Can you comment on how much resources we would be wasting by
> having the
> RenderWidgetCompositor (and therefore the LayerTreeHost) visible, but never
> rendering it?
>

It can be a lot, actually.
Visibility is the main trigger to the compositor for knowing whether to
allocate resources, keep caches etc. When going hidden we release almost
all GPU resources - that can be several tens of MB typically - and shared
memory pages to communicate with the GPU process (a few MB typically).
Also, visibility is also what triggers the compositor state machine to do
things like pre-rendering of tiles, request for BeginMainFrames, etc. So
I'm not sure about "never rendering it" - maybe it's not displayed, but I
wouldn't be surprised if it still tried to kick some "rendering" work.


>
>
>
>
https://codereview.chromium.org/1991273003/diff/240001/content/renderer/rende...
> File content/renderer/render_frame_impl.cc (left):
>
>
>
https://codereview.chromium.org/1991273003/diff/240001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:1635: // TODO(creis): Support this
> for subframes as well.
> On 2016/06/08 22:18:35, Charlie Reis wrote:
> >
> > This happens if you have proxies due to a reference from another tab.
> For
> > example, a LocalFrame in one window could have a tree of RemoteFrames
> in its
> > opener window.
> >
> > It'd be nice to solve it soon, but I'm not too concerned about it.
> That said,
> > maybe we should leave a more detailed TODO here instead of removing
> it.
>
> I see. I think the correct way to do this, is to destroy the WebWidget
> on swap, which I believe we already do for subframes (and we should do
> the same for the main frame once we split WebView/WebWidget).
>
> Actually, now that I think about this, I think this code should be
> removed. This is not the ideal scenario, and we are wasting some
> resources for now, but setting the visibility to Hidden in the webview
> has unwanted consequences, for instance, the WebViewScheduler will set
> every local frame to Hidden, regardless if it's the same local root or
> not, which is wrong.
>
> The only thing I'm a bit worried here is how much we are wasting in
> terms of resources by setting the RenderWidgetCompositor to visible,
> even though we'll never draw to this widget.
>
> https://codereview.chromium.org/1991273003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[piman, 2016-06-09 19:20:56]

On Wed, Jun 8, 2016 at 3:47 PM, <lfg@chromium.org> wrote:

> +piman: Can you comment on how much resources we would be wasting by
> having the
> RenderWidgetCompositor (and therefore the LayerTreeHost) visible, but never
> rendering it?
>

It can be a lot, actually.
Visibility is the main trigger to the compositor for knowing whether to
allocate resources, keep caches etc. When going hidden we release almost
all GPU resources - that can be several tens of MB typically - and shared
memory pages to communicate with the GPU process (a few MB typically).
Also, visibility is also what triggers the compositor state machine to do
things like pre-rendering of tiles, request for BeginMainFrames, etc. So
I'm not sure about "never rendering it" - maybe it's not displayed, but I
wouldn't be surprised if it still tried to kick some "rendering" work.


>
>
>
>
https://codereview.chromium.org/1991273003/diff/240001/content/renderer/rende...
> File content/renderer/render_frame_impl.cc (left):
>
>
>
https://codereview.chromium.org/1991273003/diff/240001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:1635: // TODO(creis): Support this
> for subframes as well.
> On 2016/06/08 22:18:35, Charlie Reis wrote:
> >
> > This happens if you have proxies due to a reference from another tab.
> For
> > example, a LocalFrame in one window could have a tree of RemoteFrames
> in its
> > opener window.
> >
> > It'd be nice to solve it soon, but I'm not too concerned about it.
> That said,
> > maybe we should leave a more detailed TODO here instead of removing
> it.
>
> I see. I think the correct way to do this, is to destroy the WebWidget
> on swap, which I believe we already do for subframes (and we should do
> the same for the main frame once we split WebView/WebWidget).
>
> Actually, now that I think about this, I think this code should be
> removed. This is not the ideal scenario, and we are wasting some
> resources for now, but setting the visibility to Hidden in the webview
> has unwanted consequences, for instance, the WebViewScheduler will set
> every local frame to Hidden, regardless if it's the same local root or
> not, which is wrong.
>
> The only thing I'm a bit worried here is how much we are wasting in
> terms of resources by setting the RenderWidgetCompositor to visible,
> even though we'll never draw to this widget.
>
> https://codereview.chromium.org/1991273003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[lfg, 2016-06-18 10:44:32]

On 2016/06/09 19:20:56, piman OOO back 2016-06-20 wrote:
> On Wed, Jun 8, 2016 at 3:47 PM, <mailto:lfg@chromium.org> wrote:
> 
> > +piman: Can you comment on how much resources we would be wasting by
> > having the
> > RenderWidgetCompositor (and therefore the LayerTreeHost) visible, but never
> > rendering it?
> >
> 
> It can be a lot, actually.
> Visibility is the main trigger to the compositor for knowing whether to
> allocate resources, keep caches etc. When going hidden we release almost
> all GPU resources - that can be several tens of MB typically - and shared
> memory pages to communicate with the GPU process (a few MB typically).
> Also, visibility is also what triggers the compositor state machine to do
> things like pre-rendering of tiles, request for BeginMainFrames, etc. So
> I'm not sure about "never rendering it" - maybe it's not displayed, but I
> wouldn't be surprised if it still tried to kick some "rendering" work.

Sorry for the delay, I was at BlinkOn this week.

I've implemented a temporary override so that we can have page visibility
without actually setting the compositor to visible. This should go away when
we are able to instantiate a WebFrameWidget for the main frame, and have
the widget own the LayerTree.

dcheng@: Can you take another look? I had to modify WebViewImpl and
WebViewFrameWidget.

[dcheng, 2016-06-20 14:57:10]

On 2016/06/18 10:44:32, lfg (OOO until Jun 25) wrote:
> On 2016/06/09 19:20:56, piman OOO back 2016-06-20 wrote:
> > On Wed, Jun 8, 2016 at 3:47 PM, <mailto:lfg@chromium.org> wrote:
> > 
> > > +piman: Can you comment on how much resources we would be wasting by
> > > having the
> > > RenderWidgetCompositor (and therefore the LayerTreeHost) visible, but
never
> > > rendering it?
> > >
> > 
> > It can be a lot, actually.
> > Visibility is the main trigger to the compositor for knowing whether to
> > allocate resources, keep caches etc. When going hidden we release almost
> > all GPU resources - that can be several tens of MB typically - and shared
> > memory pages to communicate with the GPU process (a few MB typically).
> > Also, visibility is also what triggers the compositor state machine to do
> > things like pre-rendering of tiles, request for BeginMainFrames, etc. So
> > I'm not sure about "never rendering it" - maybe it's not displayed, but I
> > wouldn't be surprised if it still tried to kick some "rendering" work.
> 
> Sorry for the delay, I was at BlinkOn this week.
> 
> I've implemented a temporary override so that we can have page visibility
> without actually setting the compositor to visible. This should go away when
> we are able to instantiate a WebFrameWidget for the main frame, and have
> the widget own the LayerTree.
> 
> dcheng@: Can you take another look? I had to modify WebViewImpl and
> WebViewFrameWidget.

Can you add some comments to setCompositorVisibility() / the associated member?

[lfg, 2016-06-28 12:36:09]

On 2016/06/20 14:57:10, dcheng wrote:
> 
> Can you add some comments to setCompositorVisibility() / the associated
member?

Done.

[lfg, 2016-06-28 13:32:25]

lfg@chromium.org changed reviewers:
+ jochen@chromium.org

[lfg, 2016-06-28 13:32:26]

+jochen@ for changes in chrome/, content/, extensions/.

[lfg, 2016-06-28 13:33:48]

On 2016/06/28 13:32:26, lfg wrote:
> +jochen@ for changes in chrome/, content/, extensions/.

That should've been chrome/, components/, extensions/ (the changes related to
WebView::create).

[jochen (gone - plz use gerrit), 2016-06-29 15:14:41]

lgtm

[lfg, 2016-06-29 16:09:37]

lfg@chromium.org changed reviewers:
+ liberato@chromium.org

[lfg, 2016-06-29 16:09:39]

liberato@chromium.org: Please review changes in 

media/

[liberato (no reviews please), 2016-06-29 16:23:21]

media/ lgtm.

[dcheng, 2016-06-30 06:15:11]

still lgtm

[lfg, 2016-06-30 13:26:37]

The CQ bit was checked by lfg@chromium.org

[lfg, 2016-06-30 13:26:38]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1991273003/#ps360001
(title: "comments")

[commit-bot: I haz the power, 2016-06-30 13:26:56]

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-06-30 15:04:35]

Description was changed from

==========
Fire visibilityChange event on out-of-process iframes.

Design doc:
https://docs.google.com/document/d/1qdlgaC277SnnmQv7hoNFTGp-OCvokEc4mA2NmAzOy...

BUG=550967
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fire visibilityChange event on out-of-process iframes.

Design doc:
https://docs.google.com/document/d/1qdlgaC277SnnmQv7hoNFTGp-OCvokEc4mA2NmAzOy...

BUG=550967
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-06-30 15:04:38]

Committed patchset #17 (id:360001)

[commit-bot: I haz the power, 2016-06-30 15:04:51]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 15:06:41]

Description was changed from

==========
Fire visibilityChange event on out-of-process iframes.

Design doc:
https://docs.google.com/document/d/1qdlgaC277SnnmQv7hoNFTGp-OCvokEc4mA2NmAzOy...

BUG=550967
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fire visibilityChange event on out-of-process iframes.

Design doc:
https://docs.google.com/document/d/1qdlgaC277SnnmQv7hoNFTGp-OCvokEc4mA2NmAzOy...

BUG=550967
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/7171540721a57cf5926709a277e5d70b09965643
Cr-Commit-Position: refs/heads/master@{#403176}
==========

[commit-bot: I haz the power, 2016-06-30 15:06:43]

Patchset 17 (id:??) landed as
https://crrev.com/7171540721a57cf5926709a277e5d70b09965643
Cr-Commit-Position: refs/heads/master@{#403176}