Simplify computation of the invalidation rect for a frame.

Simplify computation of the invalidation rect for scrolling, and clip
rects for plugins.

BUG=549788
TBR=bbudge@chromium.org,mseaborn@chromium.org

Committed: https://crrev.com/c0fa9443c9cb93708b6b743663bc3c49ea079900
Cr-Commit-Position: refs/heads/master@{#358262}

[chrishtr, 2015-10-30 23:12:05]

Description was changed from

==========
none

BUG=
==========

to

==========
Simplify computation of the invalidation rect for a frame.

BUG=549788
==========

[chrishtr, 2015-11-03 19:18:12]

Description was changed from

==========
Simplify computation of the invalidation rect for a frame.

BUG=549788
==========

to

==========
Simplify computation of the invalidation rect for scrolling, and clip
rects for plugins.

BUG=549788
==========

[chrishtr, 2015-11-03 19:22:54]

chrishtr@chromium.org changed reviewers:
+ skobes@chromium.org, tommycli@chromium.org, wangxianzhu@chromium.org

[chrishtr, 2015-11-03 19:22:55]

The purpose of this CL is to:

1. Reduce dependency on PaintLayerClipper, which is a source of complexity and
sadness that needs to be addressed.

2. Reduce transform and clip code special to FrameView.

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:953:
clippedAbsoluteRect = IntRect();
I changed the code to just return an empty rect in scenarios where the plugin is
not attached to the layout tree properly. I think this will be fine.

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:966:
enclosingLayer->layoutObject()->mapRectToPaintInvalidationBacking(rootView,
unclippedAbsoluteRect, nullptr);
What this does: maps the box rect for the plugin container to the space of the
root view of the webpage, taking into account all transforms and clips along the
way. IOW it's the bounding box of the actual pixels the plugin can affect.

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebPluginContainerImpl.h (right):

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPluginContainerImpl.h:168: // Sets
|clippedAbsoluteRect| to the absolute visible rect for the plugin, clipped to
the visible screen of the root frame.
This is my understanding of what is intended after reading the old code. Is it
correct?

i.e. we want to compute the pixels in the root view of the document, bot clipped
to the visible viewport, and not clipped to it.

[chrishtr, 2015-11-03 19:23:24]

This CL currently fails a few PPAPI tests. Are those tests actually not obsolete
& deleted? Why not? Thanks.

[chrishtr, 2015-11-03 19:24:15]

test:

1. Reduce dependency on PaintLayerClipper, which is a source of complexity and
sadness that needs to be addressed.

[tommycli, 2015-11-03 20:10:22]

Generally speaking LGTM from my perspective, assuming this change doesn't break
the PPS tests.

I'm not an expert in painting though, so you will probably want the other
reviewers opinions also.

Does this fix the issue described in:

https://code.google.com/p/chromium/issues/detail?id=343769#c4

The plugins have always received multiple (sometimes incorrect) reports of their
geometry from the rendering engine. There's a hack that just rechecks it after
250ms, hoping that the size has "settled".

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:175:
DisableCompositingQueryAsserts disabler;
Previous usage of this had this comment:

// FIXME: childrenClipRect relies on compositingState, which is not necessarily
up to date.	
// https://code.google.com/p/chromium/issues/detail?id=343769

Should probably preserve, since that comment helped me when I was tweaking that
code.

Why does this method suddenly need this btw?

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:953:
clippedAbsoluteRect = IntRect();
On 2015/11/03 19:22:55, chrishtr wrote:
> I changed the code to just return an empty rect in scenarios where the plugin
is
> not attached to the layout tree properly. I think this will be fine.

Acknowledged.

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:971: if
(unclippedIntAbsoluteRect)
nit: Seems like you're doing the enclosingIntRect op twice. Can you just make a
copy from clippedAbsoluteRect to unclipped before you do the intersection?

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebPluginContainerImpl.h (right):

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPluginContainerImpl.h:168: // Sets
|clippedAbsoluteRect| to the absolute visible rect for the plugin, clipped to
the visible screen of the root frame.
On 2015/11/03 19:22:55, chrishtr wrote:
> This is my understanding of what is intended after reading the old code. Is it
> correct?
> 
> i.e. we want to compute the pixels in the root view of the document, bot
clipped
> to the visible viewport, and not clipped to it.

Exactly. One is clipped to the visible viewport, the other pretends that the
user's monitor can show the whole webpage.

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPluginContainerImpl.h:169: // If not null, sets
|unclippedAbsoluteRect| tp the absolute visible rect for the plugin (but without
also clipping to the screen).
Tiny typo.

[chrishtr, 2015-11-03 21:23:55]

On 2015/11/03 at 20:10:22, tommycli wrote:
> Generally speaking LGTM from my perspective, assuming this change doesn't
break the PPS tests.
> 
> I'm not an expert in painting though, so you will probably want the other
reviewers opinions also.
> 
> Does this fix the issue described in:
> 
> https://code.google.com/p/chromium/issues/detail?id=343769#c4
> 
> The plugins have always received multiple (sometimes incorrect) reports of
their geometry from the rendering engine. There's a hack that just rechecks it
after 250ms, hoping that the size has "settled".

Not sure, maybe. The dependency on compositor state is now gone, which might fix
the issue. I'll look into it.

> 
>
https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):
> 
>
https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:175:
DisableCompositingQueryAsserts disabler;
> Previous usage of this had this comment:
> 
> // FIXME: childrenClipRect relies on compositingState, which is not
necessarily up to date.	
> // https://code.google.com/p/chromium/issues/detail?id=343769
> 
> Should probably preserve, since that comment helped me when I was tweaking
that code.
> 
> Why does this method suddenly need this btw?
> 
>
https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right):
> 
>
https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:953:
clippedAbsoluteRect = IntRect();
> On 2015/11/03 19:22:55, chrishtr wrote:
> > I changed the code to just return an empty rect in scenarios where the
plugin is
> > not attached to the layout tree properly. I think this will be fine.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:971: if
(unclippedIntAbsoluteRect)
> nit: Seems like you're doing the enclosingIntRect op twice. Can you just make
a copy from clippedAbsoluteRect to unclipped before you do the intersection?
> 
>
https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/WebPluginContainerImpl.h (right):
> 
>
https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/WebPluginContainerImpl.h:168: // Sets
|clippedAbsoluteRect| to the absolute visible rect for the plugin, clipped to
the visible screen of the root frame.
> On 2015/11/03 19:22:55, chrishtr wrote:
> > This is my understanding of what is intended after reading the old code. Is
it
> > correct?
> > 
> > i.e. we want to compute the pixels in the root view of the document, bot
clipped
> > to the visible viewport, and not clipped to it.
> 
> Exactly. One is clipped to the visible viewport, the other pretends that the
user's monitor can show the whole webpage.
> 
>
https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/WebPluginContainerImpl.h:169: // If not null,
sets |unclippedAbsoluteRect| tp the absolute visible rect for the plugin (but
without also clipping to the screen).
> Tiny typo.

[chrishtr, 2015-11-03 21:24:57]

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:175:
DisableCompositingQueryAsserts disabler;
On 2015/11/03 at 20:10:22, tommycli wrote:
> Previous usage of this had this comment:
> 
> // FIXME: childrenClipRect relies on compositingState, which is not
necessarily up to date.	
> // https://code.google.com/p/chromium/issues/detail?id=343769
> 
> Should probably preserve, since that comment helped me when I was tweaking
that code.
> 
> Why does this method suddenly need this btw?

Forgot to update in the latest patchset. This method can actually be deleted
now.

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:971: if
(unclippedIntAbsoluteRect)
On 2015/11/03 at 20:10:22, tommycli wrote:
> nit: Seems like you're doing the enclosingIntRect op twice. Can you just make
a copy from clippedAbsoluteRect to unclipped before you do the intersection?

Done.

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebPluginContainerImpl.h (right):

https://codereview.chromium.org/1413523007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPluginContainerImpl.h:169: // If not null, sets
|unclippedAbsoluteRect| tp the absolute visible rect for the plugin (but without
also clipping to the screen).
On 2015/11/03 at 20:10:22, tommycli wrote:
> Tiny typo.

Fixed.

[skobes, 2015-11-04 04:02:07]

lgtm, although I am not at all familiar with the plugin code

[chrishtr, 2015-11-05 03:44:07]

Updated to simplify the code in WebPluginContainerImpl even further, and fix
PPPI tests.
The tests were broken because:

1. The clip rects are supposed to be for the contentBoxRect
(excluding padding, margin and border). Fixed that.
2. The windowRect was computed with a different method call that differed w.r.t.
scrolling,
and there was extra logic to compensate for that by subtracting.
I fixed the method to unify and removed the extra logic.

[chrishtr, 2015-11-05 03:44:28]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-11-05 03:44:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/1413523007/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-11-05 03:45:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413523007/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413523007/220001

[commit-bot: I haz the power, 2015-11-05 04:30:24]

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

[commit-bot: I haz the power, 2015-11-05 04:30:24]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[chrishtr, 2015-11-05 21:40:48]

chrishtr@chromium.org changed reviewers:
+ bbudge@chromium.org

[chrishtr, 2015-11-05 21:40:49]

+bbudge for ppapi/tests/test_view.cc change. AFAICT the object really is at a
1px offset.

[chrishtr, 2015-11-06 00:53:12]

Description was changed from

==========
Simplify computation of the invalidation rect for scrolling, and clip
rects for plugins.

BUG=549788
==========

to

==========
Simplify computation of the invalidation rect for scrolling, and clip
rects for plugins.

BUG=549788
TBR=bbudge@chromium.org,mseaborn@chromium.org
==========

[chrishtr, 2015-11-06 00:53:17]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-11-06 00:53:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/1413523007/#ps320001
(title: " ")

[commit-bot: I haz the power, 2015-11-06 00:54:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413523007/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413523007/320001

[chrishtr, 2015-11-06 00:54:10]

On 2015/11/05 at 21:40:49, chrishtr wrote:
> +bbudge for ppapi/tests/test_view.cc change. AFAICT the object really is at a
1px offset.

The position is actually fractional between the root LayoutBoxFlow and
LayoutBody. This causes the enclosing int rects for clip to be a pixel off in
some cases due to enclosingIntRect().

[commit-bot: I haz the power, 2015-11-06 02:28:00]

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

[commit-bot: I haz the power, 2015-11-06 02:28:01]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2015-11-06 03:40:55]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-11-06 03:40:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/1413523007/#ps340001
(title: " ")

[commit-bot: I haz the power, 2015-11-06 03:41:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413523007/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413523007/340001

[commit-bot: I haz the power, 2015-11-06 05:03:29]

Committed patchset #19 (id:340001)

[commit-bot: I haz the power, 2015-11-06 05:04:20]

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

[bbudge, 2015-11-06 13:24:54]

ppapi/tests/test_view.cc LGTM