Fix painting of fixed background images in composited layers

This is a merge of http://trac.webkit.org/changeset/151623 by Simon Fraser <simon.fraser@apple.com>;.

It was a mostly-mechanical merge. The reftest fixed-backgrounds.html required some tweaking,
since it relied on composited and non-composited output being identical. This required changing
the shadow from a soft shadow to a hard shadow.

BUG=330852
--

Source/WebCore: Painting of fixed background images is wrong in composited layers
​https://bugs.webkit.org/show_bug.cgi?id=65793
Reviewed by Sam Weinig.
The code that computed background image geometry for background-attachment:fixed
images was unaware of compositing, so often painting the image at the wrong location.
Fix by having RenderBoxModelObject::calculateBackgroundImageGeometry() do the correct
math for fixed backgrounds in composited layer by offsetting the viewport rect by
the paint container's absolute position.
Tests: compositing/backgrounds/fixed-background-on-descendant.html
compositing/backgrounds/fixed-backgrounds.html
rendering/RenderBox.cpp:
(WebCore::RenderBox::getBackgroundPaintedExtent): Now returns a bool indicating
whether it is returning a reliable extent rect. It can return false in the case where
a background is fixed, since computing the correct extent would require finding
the appropriate composited ancestor to pass to calculateBackgroundImageGeometry().
This is OK since this function is used for "background opaque" optimizations.
(WebCore::RenderBox::computeBackgroundIsKnownToBeObscured): If getBackgroundPaintedExtent()
returns false, return false.
(WebCore::RenderBox::maskClipRect): We removed mask-attachment, so we never need to
compute the composited ancestor here and can pass null.
(WebCore::RenderBox::repaintLayerRectsForImage): Unwrap a comment.
If the changed image is related to a fixed background, geometry.hasNonLocalGeometry()
will be true. In that cause, just repaint the entire renderer rather than groveling
around for a composited ancestor.
rendering/RenderBox.h: Changed name and signature of backgroundPaintedExtent.
rendering/RenderBoxModelObject.cpp:
(WebCore::RenderBoxModelObject::paintFillLayerExtended): calculateBackgroundImageGeometry()
now needs to know the painting container.
(WebCore::RenderBoxModelObject::calculateBackgroundImageGeometry): Now takes a painting
container, that is required to correctly compute the viewport-relative offset for fixed
backgrounds. geometry.setHasNonLocalGeometry() is set for fixed backgrounds to indicate
to callers that, if they didn't pass a paint container, the destRect is not accurate.
The main bug fix is also here: we move the viewportRect by the absolute location of
paint container, which is equivalent to the composited layer offset.
(WebCore::RenderBoxModelObject::getGeometryForBackgroundImage): calculateBackgroundImageGeometry()
takes a paint container.
rendering/RenderBoxModelObject.h:
(WebCore::RenderBoxModelObject::BackgroundImageGeometry::BackgroundImageGeometry):
(WebCore::RenderBoxModelObject::BackgroundImageGeometry::setHasNonLocalGeometry):
(WebCore::RenderBoxModelObject::BackgroundImageGeometry::hasNonLocalGeometry):
rendering/RenderImage.cpp:
(WebCore::RenderImage::computeBackgroundIsKnownToBeObscured): If getBackgroundPaintedExtent()
can't cheaply give an accurate answer, return false.
rendering/RenderLayerBacking.cpp:
(WebCore::RenderLayerBacking::updateDirectlyCompositedBackgroundImage): Pass the paint container,
which is our own renderer.
LayoutTests: Fixed background images behave strangely with webkit transitions
​https://bugs.webkit.org/show_bug.cgi?id=65793
Reviewed by Sam Weinig.
Ref tests that compare fixed background rendering after a scroll, with and
without compositing, with a couple of layer configurations.
compositing/backgrounds/fixed-background-on-descendant-expected.html: Added.
compositing/backgrounds/fixed-background-on-descendant.html: Added.
compositing/backgrounds/fixed-backgrounds-expected.html: Added.
compositing/backgrounds/fixed-backgrounds.html: Added.

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

[Stephen White, 2014-01-02 22:49:04]

Shawn, could you take a look? I'm not an expert in this code, but this merge
seemed pretty straightforward. Let me know if there's someone else I should loop
in.

[esprehn, 2014-01-03 16:41:33]

You should add a title and change description for what this patch actually does.
There may come a time when trac.webkit.org doesn't exist, or someone might not
be online when looking at git logs.

[Stephen White, 2014-01-03 17:43:43]

On 2014/01/03 16:41:33, esprehn wrote:
> You should add a title and change description for what this patch actually
does.
> There may come a time when http://trac.webkit.org doesn't exist, or someone
might not
> be online when looking at git logs.

Done. Also +enne, for more compositor-fu.

[Stephen White, 2014-01-03 17:49:06]

+enne for compositor-fu (for realz this time).

[shawnsingh, 2014-01-05 18:48:17]

Superficially this looks OK to me, but I'm not familiar enough with this side of
the code.  I think jchaffraix or esprehn are better suited to review this, or
perhaps eseidel.

[Stephen White, 2014-01-07 20:59:35]

esprehn@: ping?

[esprehn, 2014-01-08 01:53:54]

https://codereview.chromium.org/102123013/diff/1/LayoutTests/compositing/back...
File
LayoutTests/compositing/backgrounds/fixed-background-on-descendant-expected.html
(right):

https://codereview.chromium.org/102123013/diff/1/LayoutTests/compositing/back...
LayoutTests/compositing/backgrounds/fixed-background-on-descendant-expected.html:3:
<html>
We usually leave off the <html>, <head> and <body>.

https://codereview.chromium.org/102123013/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/102123013/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1353: return
!geometry.hasNonLocalGeometry();
Instead of assigning the values of the layout rect even if we're going to return
false, can we early return and avoid assigning the values we know are probably
wrong?

if (geometry.hasNonLocalGeometry())
  return false;
paintExtent = ...;
return true;

https://codereview.chromium.org/102123013/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1690: layerRenderer->repaint();
I suspect this will be far worse sometimes too, a huge raster on mobile is worse
I think?

https://codereview.chromium.org/102123013/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBoxModelObject.h (right):

https://codereview.chromium.org/102123013/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBoxModelObject.h:205: { }
nl after }

https://codereview.chromium.org/102123013/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBoxModelObject.h:259: bool m_hasNonLocalGeometry; //
Has background-attachment: fixed. Implies that we can't always cheaply compute
destRect.
Don't stick a bool between the IntSizes, that causes nasty alignment from the
compiler which uses more memory. It should be last.

[esprehn, 2014-03-27 09:48:24]

What happened to this patch?

[Stephen White, 2014-03-27 14:43:34]

On 2014/03/27 09:48:24, esprehn wrote:
> What happened to this patch?

Deprioritized. I'll dust it off and take a look.

[Stephen White, 2014-03-27 15:07:12]

New patch up.

https://codereview.chromium.org/102123013/diff/1/LayoutTests/compositing/back...
File
LayoutTests/compositing/backgrounds/fixed-background-on-descendant-expected.html
(right):

https://codereview.chromium.org/102123013/diff/1/LayoutTests/compositing/back...
LayoutTests/compositing/backgrounds/fixed-background-on-descendant-expected.html:3:
<html>
On 2014/01/08 01:53:55, esprehn wrote:
> We usually leave off the <html>, <head> and <body>.

Done (the tests were copied verbatim from the WebKit patch).

https://codereview.chromium.org/102123013/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/102123013/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1353: return
!geometry.hasNonLocalGeometry();
On 2014/01/08 01:53:55, esprehn wrote:
> Instead of assigning the values of the layout rect even if we're going to
return
> false, can we early return and avoid assigning the values we know are probably
> wrong?
> 
> if (geometry.hasNonLocalGeometry())
>   return false;
> paintExtent = ...;
> return true;

Done.

https://codereview.chromium.org/102123013/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1690: layerRenderer->repaint();
On 2014/01/08 01:53:55, esprehn wrote:
> I suspect this will be far worse sometimes too, a huge raster on mobile is
worse
> I think?

I defer to someone more knowledgeable. I'm just cargo culting this patch because
it fixes my bug. If there's a better way, someone else should probably step in.

https://codereview.chromium.org/102123013/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBoxModelObject.h (right):

https://codereview.chromium.org/102123013/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBoxModelObject.h:205: { }
On 2014/01/08 01:53:55, esprehn wrote:
> nl after }

Done.

https://codereview.chromium.org/102123013/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBoxModelObject.h:259: bool m_hasNonLocalGeometry; //
Has background-attachment: fixed. Implies that we can't always cheaply compute
destRect.
On 2014/01/08 01:53:55, esprehn wrote:
> Don't stick a bool between the IntSizes, that causes nasty alignment from the
> compiler which uses more memory. It should be last.

Done.

[esprehn, 2014-04-01 22:00:12]

lgtm

https://codereview.chromium.org/102123013/diff/130001/LayoutTests/compositing...
File
LayoutTests/compositing/backgrounds/fixed-background-on-descendant-expected.html
(right):

https://codereview.chromium.org/102123013/diff/130001/LayoutTests/compositing...
LayoutTests/compositing/backgrounds/fixed-background-on-descendant-expected.html:1:
<style>
same

https://codereview.chromium.org/102123013/diff/130001/LayoutTests/compositing...
File LayoutTests/compositing/backgrounds/fixed-background-on-descendant.html
(right):

https://codereview.chromium.org/102123013/diff/130001/LayoutTests/compositing...
LayoutTests/compositing/backgrounds/fixed-background-on-descendant.html:1:
<style>
<!DOCTYPE html>

https://codereview.chromium.org/102123013/diff/130001/LayoutTests/compositing...
File LayoutTests/compositing/backgrounds/fixed-backgrounds-expected.html
(right):

https://codereview.chromium.org/102123013/diff/130001/LayoutTests/compositing...
LayoutTests/compositing/backgrounds/fixed-backgrounds-expected.html:1: <style>
same

https://codereview.chromium.org/102123013/diff/130001/LayoutTests/compositing...
File LayoutTests/compositing/backgrounds/fixed-backgrounds.html (right):

https://codereview.chromium.org/102123013/diff/130001/LayoutTests/compositing...
LayoutTests/compositing/backgrounds/fixed-backgrounds.html:1: <style>
same

[Stephen White, 2014-04-01 22:45:41]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2014-04-01 22:45:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/senorblanco@chromium.org/102123013/170001

[commit-bot: I haz the power, 2014-04-02 01:08:23]

Change committed as 170618