Currently we only composite fixed position elements if we are on High DPI devices. This fix allows …

Currently we only composite fixed position elements if we are on High DPI devices. This fix allows these elements to be composited if they are completely opaque (since we will not lose LCD text) OR on High DPI devices

BUG=510580, 482229

[trchen, 2015-08-14 21:02:17]

trchen@chromium.org changed reviewers:
+ trchen@chromium.org

[trchen, 2015-08-14 21:02:17]

https://codereview.chromium.org/1298493003/diff/1/Source/core/layout/composit...
File Source/core/layout/compositing/CompositingReasonFinder.cpp (right):

https://codereview.chromium.org/1298493003/diff/1/Source/core/layout/composit...
Source/core/layout/compositing/CompositingReasonFinder.cpp:184: if
(!(m_compositingTriggers & ViewportConstrainedPositionedTrigger)) {
Please swap the branches. Avoid negative predicates and short branch goes first.
:)

https://codereview.chromium.org/1298493003/diff/1/Source/core/layout/composit...
Source/core/layout/compositing/CompositingReasonFinder.cpp:186: LayoutRect
bounds = layer->layoutBox()->contentBoxRect();
This doesn't look right because texts can overflow. Better simply look at the
entire visual overflow.

[davidfox, 2015-08-15 04:47:47]

The CQ bit was checked by david@davidjfox.com

[davidfox, 2015-08-15 04:47:48]

The CQ bit was unchecked by david@davidjfox.com

[trchen, 2015-08-17 22:31:52]

trchen@chromium.org changed reviewers:
+ vollick@chromium.org

[trchen, 2015-08-20 22:24:05]

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

[commit-bot: I haz the power, 2015-08-20 22:24:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1298493003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1298493003/60001

[commit-bot: I haz the power, 2015-08-20 23:32:16]

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

[commit-bot: I haz the power, 2015-08-20 23:32:16]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Ian Vollick, 2015-08-21 14:40:43]

Hey! Sorry for the slow replies. Have you manually confirmed that the test
virtual/prefer_compositing_to_lcd_text/compositing/overflow/fixed-scroll-in-empty-root-layer.html
is producing correct results? (I assume it's just differences due to the slight
transparency you've added).

If so, please add the new expectation for one platform and set the others to
NeedsRebaseline in TestExpectations.

https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
File LayoutTests/compositing/layer-creation/fixed-position-under-transform.html
(right):

https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
LayoutTests/compositing/layer-creation/fixed-position-under-transform.html:19:
background-color: rgba(255, 0, 0, 0.99);
Just to be sure I'm understanding you; you're changing these fixed pos elements
to not be opaque so that they test the same old behavior?

https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
File LayoutTests/compositing/rtl/rtl-fixed-overflow-scrolled.html (left):

https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
LayoutTests/compositing/rtl/rtl-fixed-overflow-scrolled.html:24:
transform:translateZ(0);
ditto for removing the translateZ?

[davidfox, 2015-08-21 14:50:35]

Regarding
virtual/prefer_compositing_to_lcd_text/compositing/overflow/fixed-scroll-in-empty-root-layer.html
:
The only difference is the contents are no longer opaque due to the transparency
(to prevent the layer from getting promoted)
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...

https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
File LayoutTests/compositing/layer-creation/fixed-position-under-transform.html
(right):

https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
LayoutTests/compositing/layer-creation/fixed-position-under-transform.html:19:
background-color: rgba(255, 0, 0, 0.99);
On 2015/08/21 14:40:43, vollick wrote:
> Just to be sure I'm understanding you; you're changing these fixed pos
elements
> to not be opaque so that they test the same old behavior?

That's exactly correct.

https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
File LayoutTests/compositing/rtl/rtl-fixed-overflow-scrolled.html (left):

https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
LayoutTests/compositing/rtl/rtl-fixed-overflow-scrolled.html:24:
transform:translateZ(0);
On 2015/08/21 14:40:43, vollick wrote:
> ditto for removing the translateZ?

Yeah. This is no longer needed... The layer will automatically be promoted to
its own layer.

[davidfox, 2015-08-21 14:53:52]

On 2015/08/21 14:50:35, obto wrote:
> Regarding
>
virtual/prefer_compositing_to_lcd_text/compositing/overflow/fixed-scroll-in-empty-root-layer.html
> :
> The only difference is the contents are no longer opaque due to the
transparency
> (to prevent the layer from getting promoted)
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
> 
>
https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
> File
LayoutTests/compositing/layer-creation/fixed-position-under-transform.html
> (right):
> 
>
https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
> LayoutTests/compositing/layer-creation/fixed-position-under-transform.html:19:
> background-color: rgba(255, 0, 0, 0.99);
> On 2015/08/21 14:40:43, vollick wrote:
> > Just to be sure I'm understanding you; you're changing these fixed pos
> elements
> > to not be opaque so that they test the same old behavior?
> 
> That's exactly correct.
> 
>
https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
> File LayoutTests/compositing/rtl/rtl-fixed-overflow-scrolled.html (left):
> 
>
https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
> LayoutTests/compositing/rtl/rtl-fixed-overflow-scrolled.html:24:
> transform:translateZ(0);
> On 2015/08/21 14:40:43, vollick wrote:
> > ditto for removing the translateZ?
> 
> Yeah. This is no longer needed... The layer will automatically be promoted to
> its own layer.

What do you suggest doing regarding the failed interactive_ui_tests?

Looks like I'm seeing the same bug mentioned here but for linux?:
http://crbug.com/518106 ;
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

[Ian Vollick, 2015-08-21 14:56:17]

On 2015/08/21 14:53:52, obto wrote:
> On 2015/08/21 14:50:35, obto wrote:
> > Regarding
> >
>
virtual/prefer_compositing_to_lcd_text/compositing/overflow/fixed-scroll-in-empty-root-layer.html
> > :
> > The only difference is the contents are no longer opaque due to the
> transparency
> > (to prevent the layer from getting promoted)
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
> > 
> >
>
https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
> > File
> LayoutTests/compositing/layer-creation/fixed-position-under-transform.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
> >
LayoutTests/compositing/layer-creation/fixed-position-under-transform.html:19:
> > background-color: rgba(255, 0, 0, 0.99);
> > On 2015/08/21 14:40:43, vollick wrote:
> > > Just to be sure I'm understanding you; you're changing these fixed pos
> > elements
> > > to not be opaque so that they test the same old behavior?
> > 
> > That's exactly correct.
> > 
> >
>
https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
> > File LayoutTests/compositing/rtl/rtl-fixed-overflow-scrolled.html (left):
> > 
> >
>
https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
> > LayoutTests/compositing/rtl/rtl-fixed-overflow-scrolled.html:24:
> > transform:translateZ(0);
> > On 2015/08/21 14:40:43, vollick wrote:
> > > ditto for removing the translateZ?
> > 
> > Yeah. This is no longer needed... The layer will automatically be promoted
to
> > its own layer.
> 
> What do you suggest doing regarding the failed interactive_ui_tests?
> 
> Looks like I'm seeing the same bug mentioned here but for linux?:
> http://crbug.com/518106 ;
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

Unless I'm missing something, these failures seem unrelated to your patch.

[davidfox, 2015-08-21 15:39:49]

On 2015/08/21 14:56:17, vollick wrote:
> On 2015/08/21 14:53:52, obto wrote:
> > On 2015/08/21 14:50:35, obto wrote:
> > > Regarding
> > >
> >
>
virtual/prefer_compositing_to_lcd_text/compositing/overflow/fixed-scroll-in-empty-root-layer.html
> > > :
> > > The only difference is the contents are no longer opaque due to the
> > transparency
> > > (to prevent the layer from getting promoted)
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
> > > 
> > >
> >
>
https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
> > > File
> > LayoutTests/compositing/layer-creation/fixed-position-under-transform.html
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
> > >
> LayoutTests/compositing/layer-creation/fixed-position-under-transform.html:19:
> > > background-color: rgba(255, 0, 0, 0.99);
> > > On 2015/08/21 14:40:43, vollick wrote:
> > > > Just to be sure I'm understanding you; you're changing these fixed pos
> > > elements
> > > > to not be opaque so that they test the same old behavior?
> > > 
> > > That's exactly correct.
> > > 
> > >
> >
>
https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
> > > File LayoutTests/compositing/rtl/rtl-fixed-overflow-scrolled.html (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/1298493003/diff/60001/LayoutTests/compositing...
> > > LayoutTests/compositing/rtl/rtl-fixed-overflow-scrolled.html:24:
> > > transform:translateZ(0);
> > > On 2015/08/21 14:40:43, vollick wrote:
> > > > ditto for removing the translateZ?
> > > 
> > > Yeah. This is no longer needed... The layer will automatically be promoted
> to
> > > its own layer.
> > 
> > What do you suggest doing regarding the failed interactive_ui_tests?
> > 
> > Looks like I'm seeing the same bug mentioned here but for linux?:
> > http://crbug.com/518106 ;
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> 
> Unless I'm missing something, these failures seem unrelated to your patch.

I believe that should fix the issues from all the try bots. Let me know if I'm
missing anything :)

[Ian Vollick, 2015-08-24 16:29:14]

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

[commit-bot: I haz the power, 2015-08-24 16:29:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1298493003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1298493003/80001

[commit-bot: I haz the power, 2015-08-24 17:25:51]

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

[commit-bot: I haz the power, 2015-08-24 17:25:52]

Dry run: 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_...)

[trchen, 2015-08-27 00:15:52]

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

[commit-bot: I haz the power, 2015-08-27 00:16:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1298493003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1298493003/80001

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

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

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

Dry run: This issue passed the CQ dry run.

[trchen, 2015-08-27 23:10:58]

https://codereview.chromium.org/1298493003/diff/80001/Source/core/layout/comp...
File Source/core/layout/compositing/CompositingReasonFinder.cpp (right):

https://codereview.chromium.org/1298493003/diff/80001/Source/core/layout/comp...
Source/core/layout/compositing/CompositingReasonFinder.cpp:188: LayoutRect
bounds = layer->layoutBox()->visualOverflowRect();
I looked at the semantics of visualOverflowRect(), it seems that it only
includes the visual overflow from non-stacking-context descendants. (See
LayoutBox::addOverflowFromChild)

I suspect the logic is going to work for this example:
<div style="position:fixed; z-index:0; width:1px; height:1px;
background:white;">
  <div style="position:relative; z-index:0;">
    The text is probably not counted towards visualOverflowRect of the fixed-pos
parent.
  </div>
</div>

This is an awkward situation because in CompositedDeprecatedPaintLayerMapping we
use backgroundIsKnownToBeOpaqueInRect(compositedBounds()) to decide whether a
layer is opaque, but the compositedBounds() (i.e. The union of visual overflow
rect of non-composited descendants.) can't be calculated at this point because
compositing decision hasn't been made yet.

I think the best thing you can do is compute the union of layer bounding box of
all descendant DPLs, ignoring compositing status. You can refer to the code in
DeprecatedPaintLayer::physicalBoundingBoxIncludingReflectionAndStackingChildren().
Perhaps add a enum option to it to ignore compositing status?

[davidfox, 2015-08-30 22:00:04]

https://codereview.chromium.org/1298493003/diff/80001/Source/core/layout/comp...
File Source/core/layout/compositing/CompositingReasonFinder.cpp (right):

https://codereview.chromium.org/1298493003/diff/80001/Source/core/layout/comp...
Source/core/layout/compositing/CompositingReasonFinder.cpp:188: LayoutRect
bounds = layer->layoutBox()->visualOverflowRect();
On 2015/08/27 23:10:58, trchen wrote:
> I looked at the semantics of visualOverflowRect(), it seems that it only
> includes the visual overflow from non-stacking-context descendants. (See
> LayoutBox::addOverflowFromChild)
> 
> I suspect the logic is going to work for this example:
> <div style="position:fixed; z-index:0; width:1px; height:1px;
> background:white;">
>   <div style="position:relative; z-index:0;">
>     The text is probably not counted towards visualOverflowRect of the
fixed-pos
> parent.
>   </div>
> </div>
> 
> This is an awkward situation because in CompositedDeprecatedPaintLayerMapping
we
> use backgroundIsKnownToBeOpaqueInRect(compositedBounds()) to decide whether a
> layer is opaque, but the compositedBounds() (i.e. The union of visual overflow
> rect of non-composited descendants.) can't be calculated at this point because
> compositing decision hasn't been made yet.
> 
> I think the best thing you can do is compute the union of layer bounding box
of
> all descendant DPLs, ignoring compositing status. You can refer to the code in
>
DeprecatedPaintLayer::physicalBoundingBoxIncludingReflectionAndStackingChildren().
> Perhaps add a enum option to it to ignore compositing status?

Wouldn't it be possible to call DeprecatedPaintLayer::boundingBoxForCompositing
directly?

[trchen, 2015-08-31 21:22:29]

On 2015/08/30 22:00:04, davidfox wrote:
>
https://codereview.chromium.org/1298493003/diff/80001/Source/core/layout/comp...
> File Source/core/layout/compositing/CompositingReasonFinder.cpp (right):
> 
>
https://codereview.chromium.org/1298493003/diff/80001/Source/core/layout/comp...
> Source/core/layout/compositing/CompositingReasonFinder.cpp:188: LayoutRect
> bounds = layer->layoutBox()->visualOverflowRect();
> On 2015/08/27 23:10:58, trchen wrote:
> > I looked at the semantics of visualOverflowRect(), it seems that it only
> > includes the visual overflow from non-stacking-context descendants. (See
> > LayoutBox::addOverflowFromChild)
> > 
> > I suspect the logic is going to work for this example:
> > <div style="position:fixed; z-index:0; width:1px; height:1px;
> > background:white;">
> >   <div style="position:relative; z-index:0;">
> >     The text is probably not counted towards visualOverflowRect of the
> fixed-pos
> > parent.
> >   </div>
> > </div>
> > 
> > This is an awkward situation because in
CompositedDeprecatedPaintLayerMapping
> we
> > use backgroundIsKnownToBeOpaqueInRect(compositedBounds()) to decide whether
a
> > layer is opaque, but the compositedBounds() (i.e. The union of visual
overflow
> > rect of non-composited descendants.) can't be calculated at this point
because
> > compositing decision hasn't been made yet.
> > 
> > I think the best thing you can do is compute the union of layer bounding box
> of
> > all descendant DPLs, ignoring compositing status. You can refer to the code
in
> >
>
DeprecatedPaintLayer::physicalBoundingBoxIncludingReflectionAndStackingChildren().
> > Perhaps add a enum option to it to ignore compositing status?
> 
> Wouldn't it be possible to call
DeprecatedPaintLayer::boundingBoxForCompositing
> directly?

Unfortunately no. DPL::boundingBoxForCompositing depends on the compositing
decision to figure out the bounding box of all non-compositing descendants,
however at this point we are yet making compositing decision! The easiest way to
make it work is to compute the visual overflow of all descendants of the current
layer we are making decision for. This may be too conservative, but I don't see
other easy way for it to work.

Sorry that this seemingly simple change takes so long to review and revise. This
is very usual because Blink and HTML/CSS are huge and complex. There are always
some corner cases that can be overlooked. Polishing a CL does take some
perseverance. :)

[davidfox, 2015-08-31 22:14:38]

Oh I'm quite enjoying learning about all these intricacies, was just
curious if that method would be a better way :)

On Mon, Aug 31, 2015 at 4:22 PM <trchen@chromium.org> wrote:

> On 2015/08/30 22:00:04, davidfox wrote:
>
>
>
https://codereview.chromium.org/1298493003/diff/80001/Source/core/layout/comp...
> > File Source/core/layout/compositing/CompositingReasonFinder.cpp (right):
>
>
>
>
https://codereview.chromium.org/1298493003/diff/80001/Source/core/layout/comp...
> > Source/core/layout/compositing/CompositingReasonFinder.cpp:188:
> LayoutRect
> > bounds = layer->layoutBox()->visualOverflowRect();
> > On 2015/08/27 23:10:58, trchen wrote:
> > > I looked at the semantics of visualOverflowRect(), it seems that it
> only
> > > includes the visual overflow from non-stacking-context descendants.
> (See
> > > LayoutBox::addOverflowFromChild)
> > >
> > > I suspect the logic is going to work for this example:
> > > <div style="position:fixed; z-index:0; width:1px; height:1px;
> > > background:white;">
> > >   <div style="position:relative; z-index:0;">
> > >     The text is probably not counted towards visualOverflowRect of the
> > fixed-pos
> > > parent.
> > >   </div>
> > > </div>
> > >
> > > This is an awkward situation because in
> CompositedDeprecatedPaintLayerMapping
> > we
> > > use backgroundIsKnownToBeOpaqueInRect(compositedBounds()) to decide
> > whether
> a
> > > layer is opaque, but the compositedBounds() (i.e. The union of visual
> overflow
> > > rect of non-composited descendants.) can't be calculated at this point
> because
> > > compositing decision hasn't been made yet.
> > >
> > > I think the best thing you can do is compute the union of layer
> > bounding box
> > of
> > > all descendant DPLs, ignoring compositing status. You can refer to the
> > code
> in
> > >
>
>
>
DeprecatedPaintLayer::physicalBoundingBoxIncludingReflectionAndStackingChildren().
> > > Perhaps add a enum option to it to ignore compositing status?
>
> > Wouldn't it be possible to call
> DeprecatedPaintLayer::boundingBoxForCompositing
> > directly?
>
> Unfortunately no. DPL::boundingBoxForCompositing depends on the compositing
> decision to figure out the bounding box of all non-compositing descendants,
> however at this point we are yet making compositing decision! The easiest
> way to
> make it work is to compute the visual overflow of all descendants of the
> current
> layer we are making decision for. This may be too conservative, but I don't
> see
> other easy way for it to work.
>
> Sorry that this seemingly simple change takes so long to review and revise.
> This
> is very usual because Blink and HTML/CSS are huge and complex. There are
> always
> some corner cases that can be overlooked. Polishing a CL does take some
> perseverance. :)
>
> https://codereview.chromium.org/1298493003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[davidfox, 2015-09-09 20:22:13]

The CQ bit was checked by david@davidjfox.com

[davidfox, 2015-09-09 20:22:14]

The CQ bit was unchecked by david@davidjfox.com

[davidfox, 2015-09-09 20:25:30]

The CQ bit was checked by david@davidjfox.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-09 20:25:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1298493003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1298493003/100001

[commit-bot: I haz the power, 2015-09-09 20:25:43]

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

[commit-bot: I haz the power, 2015-09-09 20:25:44]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[trchen, 2015-09-17 21:40:33]

Hey vollick, I'm a bit worried about the extra time complexity. Do you have some
idea how can we accelerate this?

https://codereview.chromium.org/1298493003/diff/140001/Source/core/layout/com...
File Source/core/layout/compositing/CompositingReasonFinder.cpp (right):

https://codereview.chromium.org/1298493003/diff/140001/Source/core/layout/com...
Source/core/layout/compositing/CompositingReasonFinder.cpp:197: }
A few problem here:
1. This only adds the overflow from direct children, but we want to take every
descendant layer into consideration.
2. Need to translate from local coordinate of the descandant to the current
layer before uniting the rects.

You may refer to
DPL::physicalBoundingBoxIncludingReflectionAndStackingChildren() to how it's
done. Perhaps adding an enum option for it to ignore compositing state then it
would be good to go.

I'm a little bit worried though, because it will increase the time complexity of
compositing decision from O(1) to O(N) per layer, where N is the number of layer
in the subtree. You may want a second opinion from a compositing expert.

[Ian Vollick, 2015-09-18 02:09:50]

https://codereview.chromium.org/1298493003/diff/140001/Source/core/layout/com...
File Source/core/layout/compositing/CompositingReasonFinder.cpp (right):

https://codereview.chromium.org/1298493003/diff/140001/Source/core/layout/com...
Source/core/layout/compositing/CompositingReasonFinder.cpp:197: }
On 2015/09/17 21:40:33, trchen wrote:
> A few problem here:
> 1. This only adds the overflow from direct children, but we want to take every
> descendant layer into consideration.
> 2. Need to translate from local coordinate of the descandant to the current
> layer before uniting the rects.
> 
> You may refer to
> DPL::physicalBoundingBoxIncludingReflectionAndStackingChildren() to how it's
> done. Perhaps adding an enum option for it to ignore compositing state then it
> would be good to go.
> 
> I'm a little bit worried though, because it will increase the time complexity
of
> compositing decision from O(1) to O(N) per layer, where N is the number of
layer
> in the subtree. You may want a second opinion from a compositing expert.

Yes, this is indeed too slow. If you look at
DeprecatedPaintLayer::backgroundIsKnownToBeOpaqueInRect, there's a hidden O(N)
in there, too, if the queried rect is outside its "local" bounds (though this
shouldn't happen too often).

I think we may be able to piggyback on some existing code to get this rect.
Correct me if I'm wrong, but  what I think we're after here is the composited
bounds (see CompositedDeprecatedPaintLayerMapping::compositedBounds). We
_almost_ compute this value anyhow here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

..except we clip it away somewhat. Perhaps we could cache the background rect we
compute here before we clip it and use it for our opaqueness test.

[davidfox, 2015-09-18 02:50:37]

1. When and how often is CompositingInputsUpdater::updateRecursive invoked?
Is it only called for layers decided to be composited?

2. Are you envisioning it working something like this?:

   1. Add a method to DepreatedPaintLayer (or where best fit) to calculate
   its full background rect and cache the result.
   2. Update CompositingInputsUpdater::updateRecursive to clip the result
   from the above method, instead of calculating it on its own. This would be
   the primary way the cache is kept up to date.
   3. Add a simple getter to retrieve the cached rect. To be used in
   CompositingReasonFinder::requiresCompositingForPositionFixed



On Thu, Sep 17, 2015 at 9:09 PM <vollick@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1298493003/diff/140001/Source/core/layout/com...
> File Source/core/layout/compositing/CompositingReasonFinder.cpp (right):
>
>
>
https://codereview.chromium.org/1298493003/diff/140001/Source/core/layout/com...
> Source/core/layout/compositing/CompositingReasonFinder.cpp:197
>
<https://codereview.chromium.org/1298493003/diff/140001/Source/core/layout/com...>:
> }
> On 2015/09/17 21:40:33, trchen wrote:
> > A few problem here:
> > 1. This only adds the overflow from direct children, but we want to
> take every
> > descendant layer into consideration.
> > 2. Need to translate from local coordinate of the descandant to the
> current
> > layer before uniting the rects.
>
> > You may refer to
> > DPL::physicalBoundingBoxIncludingReflectionAndStackingChildren() to
> how it's
> > done. Perhaps adding an enum option for it to ignore compositing state
> then it
> > would be good to go.
>
> > I'm a little bit worried though, because it will increase the time
> complexity of
> > compositing decision from O(1) to O(N) per layer, where N is the
> number of layer
> > in the subtree. You may want a second opinion from a compositing
> expert.
>
> Yes, this is indeed too slow. If you look at
> DeprecatedPaintLayer::backgroundIsKnownToBeOpaqueInRect, there's a
> hidden O(N) in there, too, if the queried rect is outside its "local"
> bounds (though this shouldn't happen too often).
>
> I think we may be able to piggyback on some existing code to get this
> rect. Correct me if I'm wrong, but  what I think we're after here is the
> composited bounds (see
> CompositedDeprecatedPaintLayerMapping::compositedBounds). We _almost_
> compute this value anyhow here:
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
>
> ..except we clip it away somewhat. Perhaps we could cache the background
> rect we compute here before we clip it and use it for our opaqueness
> test.
>
> https://codereview.chromium.org/1298493003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[davidfox, 2015-09-18 02:55:40]

https://codereview.chromium.org/1298493003/diff/140001/Source/core/layout/com...
File Source/core/layout/compositing/CompositingReasonFinder.cpp (right):

https://codereview.chromium.org/1298493003/diff/140001/Source/core/layout/com...
Source/core/layout/compositing/CompositingReasonFinder.cpp:197: }
On 2015/09/18 02:09:50, vollick wrote:
> On 2015/09/17 21:40:33, trchen wrote:
> > A few problem here:
> > 1. This only adds the overflow from direct children, but we want to take
every
> > descendant layer into consideration.
> > 2. Need to translate from local coordinate of the descandant to the current
> > layer before uniting the rects.
> > 
> > You may refer to
> > DPL::physicalBoundingBoxIncludingReflectionAndStackingChildren() to how it's
> > done. Perhaps adding an enum option for it to ignore compositing state then
it
> > would be good to go.
> > 
> > I'm a little bit worried though, because it will increase the time
complexity
> of
> > compositing decision from O(1) to O(N) per layer, where N is the number of
> layer
> > in the subtree. You may want a second opinion from a compositing expert.
> 
> Yes, this is indeed too slow. If you look at
> DeprecatedPaintLayer::backgroundIsKnownToBeOpaqueInRect, there's a hidden O(N)
> in there, too, if the queried rect is outside its "local" bounds (though this
> shouldn't happen too often).
> 
> I think we may be able to piggyback on some existing code to get this rect.
> Correct me if I'm wrong, but  what I think we're after here is the composited
> bounds (see CompositedDeprecatedPaintLayerMapping::compositedBounds). We
> _almost_ compute this value anyhow here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> ..except we clip it away somewhat. Perhaps we could cache the background rect
we
> compute here before we clip it and use it for our opaqueness test.

Sorry for the "double" post. Initially replied to this via my mobile email haha

1. When and how often is CompositingInputsUpdater::updateRecursive invoked? Is
it only called for layers decided to be composited?

2. Are you envisioning it working something like this?:
- Add a method to DepreatedPaintLayer (or where best fit) to calculate its full
background rect and cache the result.

- Update CompositingInputsUpdater::updateRecursive to clip the result from the
above method, instead of calculating it on its own. This would be the primary
way the cache is kept up to date.

- Add a simple getter to retrieve the cached rect. To be used in
CompositingReasonFinder::requiresCompositingForPositionFixed

[Ian Vollick, 2015-09-18 13:45:14]

On 2015/09/18 02:50:37, davidfox wrote:
> 1. When and how often is CompositingInputsUpdater::updateRecursive invoked?
> Is it only called for layers decided to be composited?

Nope, it's called before we make compositing decisions. It's worth doing a
little digging to learn about this machinery, but the inputs are updated
whenever DeprecatedPaintLayer::setNeedsCompositingInputsUpdate is called. You'll
want to check that the input are set to update appropriately.
 
> 2. Are you envisioning it working something like this?:
> 
>    1. Add a method to DepreatedPaintLayer (or where best fit) to calculate
>    its full background rect and cache the result.
>    2. Update CompositingInputsUpdater::updateRecursive to clip the result
>    from the above method, instead of calculating it on its own. This would be
>    the primary way the cache is kept up to date.
>    3. Add a simple getter to retrieve the cached rect. To be used in
>    CompositingReasonFinder::requiresCompositingForPositionFixed

I don't think that's quite what I meant. My thoughts are (and you'll need to
check if this is actually feasible):

1. Ensure that the compositing inputs are updated before we do our opaqueness
check.
2. When updating the compositing inputs cache the background rect we'll need for
the opaqueness check if it doesn't already exist (and add the appropriate getter
on DPL).
3. Use the getter in requiresCompositingForPositionFixed.
> 
> 
> On Thu, Sep 17, 2015 at 9:09 PM <mailto:vollick@chromium.org> wrote:
> 
> >
> >
> >
>
https://codereview.chromium.org/1298493003/diff/140001/Source/core/layout/com...
> > File Source/core/layout/compositing/CompositingReasonFinder.cpp (right):
> >
> >
> >
>
https://codereview.chromium.org/1298493003/diff/140001/Source/core/layout/com...
> > Source/core/layout/compositing/CompositingReasonFinder.cpp:197
> >
>
<https://codereview.chromium.org/1298493003/diff/140001/Source/core/layout/com...>:
> > }
> > On 2015/09/17 21:40:33, trchen wrote:
> > > A few problem here:
> > > 1. This only adds the overflow from direct children, but we want to
> > take every
> > > descendant layer into consideration.
> > > 2. Need to translate from local coordinate of the descandant to the
> > current
> > > layer before uniting the rects.
> >
> > > You may refer to
> > > DPL::physicalBoundingBoxIncludingReflectionAndStackingChildren() to
> > how it's
> > > done. Perhaps adding an enum option for it to ignore compositing state
> > then it
> > > would be good to go.
> >
> > > I'm a little bit worried though, because it will increase the time
> > complexity of
> > > compositing decision from O(1) to O(N) per layer, where N is the
> > number of layer
> > > in the subtree. You may want a second opinion from a compositing
> > expert.
> >
> > Yes, this is indeed too slow. If you look at
> > DeprecatedPaintLayer::backgroundIsKnownToBeOpaqueInRect, there's a
> > hidden O(N) in there, too, if the queried rect is outside its "local"
> > bounds (though this shouldn't happen too often).
> >
> > I think we may be able to piggyback on some existing code to get this
> > rect. Correct me if I'm wrong, but  what I think we're after here is the
> > composited bounds (see
> > CompositedDeprecatedPaintLayerMapping::compositedBounds). We _almost_
> > compute this value anyhow here:
> >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> >
> > ..except we clip it away somewhat. Perhaps we could cache the background
> > rect we compute here before we clip it and use it for our opaqueness
> > test.
> >
> > https://codereview.chromium.org/1298493003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

[davidfox, 2015-10-13 02:40:11]

The CQ bit was checked by david@davidjfox.com

[davidfox, 2015-10-13 02:40:11]

The CQ bit was unchecked by david@davidjfox.com