Blink: Pass 'unobscured' rect to WebPlugins.

Blink: Pass 'unobscured' rect to WebPlugins.

This is to enable better throttling heuristics for plugins.

The 'unobscured' rect of a plugin is the visible portion of the plugin. It is the same as the 'clip' rect, except it is not clipped to the root window viewport.

That is to say - this still works for below-the-fold-plugins.

BUG=468142
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192708

[tommycli, 2015-03-24 23:23:20]

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

[tommycli, 2015-03-24 23:25:56]

tommycli@chromium.org changed reviewers:
+ japhet@chromium.org

[tommycli, 2015-03-24 23:27:21]

piman/japhet: PTAL

These are the changes piman suggested to allow plugins to get their 'unobscured'
clip rects.

In other words, these rects indicate the plugins' visible size assuming the user
has an infinitely big browser window. (Clips to inner containers and IFRAMES,
but not to the root window, which we assume the user can scroll).

[tommycli, 2015-03-24 23:29:17]

Note: This changes a public Blink interface, and will require a corresponding
update to all the implementing classes on the chrome/ side. I will send that
patch out soon.

[piman, 2015-03-24 23:59:21]

https://codereview.chromium.org/1033603007/diff/40001/Source/web/WebPluginCon...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/1033603007/diff/40001/Source/web/WebPluginCon...
Source/web/WebPluginContainerImpl.cpp:984: }
clipRect and unobscuredRect aren't initialized in the else case. Is that on
purpose? It looks like it would at least change the meaning of clipRect.

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

https://codereview.chromium.org/1033603007/diff/40001/public/web/WebPlugin.h#...
public/web/WebPlugin.h:96: bool isVisible) = 0;
You'll need to stage this change, e.g. by first providing both (old and new)
implementations on the Chrome side, then land this, wait for it to roll, then
remove the old implementations.

[tommycli, 2015-03-25 20:18:35]

piman: PTAL.

I realize some chrome/ changes to handle the Youtube masthead correctly are
still pending, but the Blink side changes look 'solid' to me.

https://codereview.chromium.org/1033603007/diff/40001/Source/web/WebPluginCon...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/1033603007/diff/40001/Source/web/WebPluginCon...
Source/web/WebPluginContainerImpl.cpp:984: }
On 2015/03/24 23:59:21, piman (Very slow to review) wrote:
> clipRect and unobscuredRect aren't initialized in the else case. Is that on
> purpose? It looks like it would at least change the meaning of clipRect.

Done.

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

https://codereview.chromium.org/1033603007/diff/40001/public/web/WebPlugin.h#...
public/web/WebPlugin.h:96: bool isVisible) = 0;
On 2015/03/24 23:59:21, piman (Very slow to review) wrote:
> You'll need to stage this change, e.g. by first providing both (old and new)
> implementations on the Chrome side, then land this, wait for it to roll, then
> remove the old implementations.

Done. I staged this by:

a) Adding both versions to the WebPlugin interface and calling both.
b) In separate patch: Modifying chrome/ to migrate all implementing classes to
the new version.
c) In separate Blink patch: Remove deprecated updateGeometry call.

[piman, 2015-03-25 20:38:51]

lgtm

https://codereview.chromium.org/1033603007/diff/80001/public/web/WebPlugin.h
File public/web/WebPlugin.h (right):

https://codereview.chromium.org/1033603007/diff/80001/public/web/WebPlugin.h#...
public/web/WebPlugin.h:101: bool isVisible) { }
nit: Maybe you can just have this call the other updateGeometry, rather than
calling both. That way you don't need to call 2 methods. If the implementation
still override the old one it's the one that's called, if the implementation
overrides the new one, the old one isn't called.

[tommycli, 2015-03-25 21:51:40]

japhet: PTAL

piman: Thanks!

https://codereview.chromium.org/1033603007/diff/80001/public/web/WebPlugin.h
File public/web/WebPlugin.h (right):

https://codereview.chromium.org/1033603007/diff/80001/public/web/WebPlugin.h#...
public/web/WebPlugin.h:101: bool isVisible) { }
On 2015/03/25 20:38:51, piman (Very slow to review) wrote:
> nit: Maybe you can just have this call the other updateGeometry, rather than
> calling both. That way you don't need to call 2 methods. If the implementation
> still override the old one it's the one that's called, if the implementation
> overrides the new one, the old one isn't called.

Done.

[tommycli, 2015-03-25 22:01:13]

On 2015/03/25 21:51:40, tommycli wrote:
> japhet: PTAL
> 
> piman: Thanks!
> 
> https://codereview.chromium.org/1033603007/diff/80001/public/web/WebPlugin.h
> File public/web/WebPlugin.h (right):
> 
>
https://codereview.chromium.org/1033603007/diff/80001/public/web/WebPlugin.h#...
> public/web/WebPlugin.h:101: bool isVisible) { }
> On 2015/03/25 20:38:51, piman (Very slow to review) wrote:
> > nit: Maybe you can just have this call the other updateGeometry, rather than
> > calling both. That way you don't need to call 2 methods. If the
implementation
> > still override the old one it's the one that's called, if the implementation
> > overrides the new one, the old one isn't called.
> 
> Done.

FYI the corresponding chrome/ patch is here:
https://codereview.chromium.org/1023783008/

But it cannot land until this lands and the blink roll happens.

[Nate Chapin, 2015-03-26 17:06:22]

japhet@chromium.org changed reviewers:
+ jchaffraix@chromium.org

[Nate Chapin, 2015-03-26 17:06:23]

jchaffraix, are these FrameView changes in your domain?

I'm not really qualified to review the FrameView changes. I'm happy to
rubberstamp the rest if need be.

[tommycli, 2015-03-26 17:16:10]

On 2015/03/26 17:06:23, Nate Chapin wrote:
> jchaffraix, are these FrameView changes in your domain?
> 
> I'm not really qualified to review the FrameView changes. I'm happy to
> rubberstamp the rest if need be.

japhet: Yes, that would be appreciated. I will wait for jchaffraix's approval
before sending to CQ.

[Nate Chapin, 2015-03-26 17:30:13]

On 2015/03/26 17:16:10, tommycli wrote:
> On 2015/03/26 17:06:23, Nate Chapin wrote:
> > jchaffraix, are these FrameView changes in your domain?
> > 
> > I'm not really qualified to review the FrameView changes. I'm happy to
> > rubberstamp the rest if need be.
> 
> japhet: Yes, that would be appreciated. I will wait for jchaffraix's approval
> before sending to CQ.

Works for me. public/web/ and Source/web/ LGTM.

[Julien - ping for review, 2015-03-26 20:41:52]

The rectangle logic makes sense but less code duplication would make me a lot
more happy.

https://codereview.chromium.org/1033603007/diff/100001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1033603007/diff/100001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:2205: IntRect
FrameView::unobscuredRectForFrameOwner(const HTMLFrameOwnerElement*
ownerElement) const
This looks awfully close to windowClipRectForFrameOwner, can't we share code
with it instead of completely duplicating code?

https://codereview.chromium.org/1033603007/diff/100001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:2216: DisableCompositingQueryAsserts disabler;
We shouldn't be adding more of these :(

[tommycli, 2015-03-27 19:39:00]

jchaffraix: See if this tickles your fancy better.

The new method combines the previous two. It will need a little scrutiny, as I
changed some of the logic.

Thanks!

https://codereview.chromium.org/1033603007/diff/100001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1033603007/diff/100001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:2205: IntRect
FrameView::unobscuredRectForFrameOwner(const HTMLFrameOwnerElement*
ownerElement) const
On 2015/03/26 20:41:51, Julien Chaffraix - PST wrote:
> This looks awfully close to windowClipRectForFrameOwner, can't we share code
> with it instead of completely duplicating code?

Done.

https://codereview.chromium.org/1033603007/diff/100001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:2216: DisableCompositingQueryAsserts disabler;
On 2015/03/26 20:41:51, Julien Chaffraix - PST wrote:
> We shouldn't be adding more of these :(

Done.

[Julien - ping for review, 2015-03-27 21:26:07]

lgtm with comments

https://codereview.chromium.org/1033603007/diff/140001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1033603007/diff/140001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:2175: void
FrameView::getClipRectsForFrameOwner(const HTMLFrameOwnerElement* ownerElement,
IntRect* clipRect, IntRect* unobscuredRect) const
In Blink, we don't prefix getters with get so it should be
clipRectsForFrameOwner.

Also it seems like we should just return |clipRect| instead of having it a
non-optional parameter.

https://codereview.chromium.org/1033603007/diff/140001/Source/core/frame/Fram...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/1033603007/diff/140001/Source/core/frame/Fram...
Source/core/frame/FrameView.h:157: // |clipRect| may not be nullptr.
|unobscuredRect| is the clip rect that is not clipped to the root window. It may
be nullptr.
I assume this should read: |clipRect| must not be nullptr (you have an ASSERT
for that)

https://codereview.chromium.org/1033603007/diff/140001/Source/web/WebPluginCo...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/1033603007/diff/140001/Source/web/WebPluginCo...
Source/web/WebPluginContainerImpl.cpp:982: IntRect elementWindowClipRect,
elementUnobscuredRect;
We prefer to split these 2 into their own lines in Blink.

[tommycli, 2015-03-27 22:42:51]

jchaffraix: I took your suggestions below.

Thanks! These make this change look almost trivial, despite its tortured
history.

https://codereview.chromium.org/1033603007/diff/140001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1033603007/diff/140001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:2175: void
FrameView::getClipRectsForFrameOwner(const HTMLFrameOwnerElement* ownerElement,
IntRect* clipRect, IntRect* unobscuredRect) const
On 2015/03/27 21:26:07, Julien Chaffraix - PST wrote:
> In Blink, we don't prefix getters with get so it should be
> clipRectsForFrameOwner.
> 
> Also it seems like we should just return |clipRect| instead of having it a
> non-optional parameter.

Done.

https://codereview.chromium.org/1033603007/diff/140001/Source/core/frame/Fram...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/1033603007/diff/140001/Source/core/frame/Fram...
Source/core/frame/FrameView.h:157: // |clipRect| may not be nullptr.
|unobscuredRect| is the clip rect that is not clipped to the root window. It may
be nullptr.
On 2015/03/27 21:26:07, Julien Chaffraix - PST wrote:
> I assume this should read: |clipRect| must not be nullptr (you have an ASSERT
> for that)

Done.

https://codereview.chromium.org/1033603007/diff/140001/Source/web/WebPluginCo...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/1033603007/diff/140001/Source/web/WebPluginCo...
Source/web/WebPluginContainerImpl.cpp:982: IntRect elementWindowClipRect,
elementUnobscuredRect;
On 2015/03/27 21:26:07, Julien Chaffraix - PST wrote:
> We prefer to split these 2 into their own lines in Blink.

Done.

[Julien - ping for review, 2015-03-28 00:18:40]

The CQ bit was checked by jchaffraix@chromium.org

[Julien - ping for review, 2015-03-28 00:18:41]

lgtm, thanks for following-up!

[Julien - ping for review, 2015-03-28 00:18:41]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1033603007/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-03-28 00:18:48]

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

[commit-bot: I haz the power, 2015-03-28 01:40:16]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192708