NOT FOR REVIEW - preliminary bg attachment fixed work

NOT FOR REVIEW - preliminary bg attachment fixed work


BUG=

[hartmanng, 2013-03-20 20:54:27]

Is the png really needed?

Just asking because I make these by accident and don't notice sometimes.

https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
File Source/WebCore/rendering/RenderLayerBacking.cpp (right):

https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderLayerBacking.cpp:871: #if PLATFORM(CHROMIUM)
A comment would be good here, I think. In particular, in the else if, why do you
need to call that remove*() function if that wasn't necessary before this patch?

https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):

https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderLayerCompositor.cpp:2290: if (Settings* settings
= m_renderView->document()->settings())
could just be combined into one if

[vollick, 2013-03-20 21:08:28]

Yep, the png was on purpose.

https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
File Source/WebCore/rendering/RenderLayerBacking.cpp (right):

https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderLayerBacking.cpp:871: #if PLATFORM(CHROMIUM)
On 2013/03/20 20:54:27, hartmanng wrote:
> A comment would be good here, I think. In particular, in the else if, why do
you
> need to call that remove*() function if that wasn't necessary before this
patch?

Done.

https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):

https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderLayerCompositor.cpp:2290: if (Settings* settings
= m_renderView->document()->settings())
On 2013/03/20 20:54:27, hartmanng wrote:
> could just be combined into one if

Done. Combined it into 0 ifs. :)

[hartmanng, 2013-03-20 21:09:07]

lgtm

[danakj, 2013-03-20 21:54:54]

yum, plumbing. i'm a noob so i have questions.

https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
File Source/WebCore/rendering/RenderLayerBacking.cpp (right):

https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerBacking.cpp:871: #if PLATFORM(CHROMIUM)
how come mac doesn't need this?

https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerBacking.cpp:882:
frameView->addViewportConstrainedObject(renderer());
is it ok to add the same renderer() more than once? would that happen?

https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):

https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerCompositor.cpp:2290: return
renderViewBacking &&
Can you explain why false if !renderViewBacking, other than safari would be?

https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerCompositor.cpp:2291:
renderView->document()->settings() &&
m_renderView?

[vollick, 2013-03-21 00:43:51]

Thanks for the review!

https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
File Source/WebCore/rendering/RenderLayerBacking.cpp (right):

https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerBacking.cpp:871: #if PLATFORM(CHROMIUM)
On 2013/03/20 21:54:54, danakj wrote:
> how come mac doesn't need this?

Good question. Mac does its registration in
RenderLayerCompositor::fixedRootBackgroundLayerChanged (which gets called from
the RenderLayerBacking when the background layer is created). That makes the
assumption that the background layer is only ever going to be used for fixed
root backgrounds (currently true). I've moved this code to the same spot for
symmetry.

https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerBacking.cpp:882:
frameView->addViewportConstrainedObject(renderer());
On 2013/03/20 21:54:54, danakj wrote:
> is it ok to add the same renderer() more than once? would that happen?
Yep, it's ok. It's a set.

https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):

https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerCompositor.cpp:2290: return
renderViewBacking &&
On 2013/03/20 21:54:54, danakj wrote:
> Can you explain why false if !renderViewBacking, other than safari would be?
You have to have a backing to have a composited fixed root bg layer -- it's what
manages its creation, insertion into the mini hierarchy, positioning, etc.

https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerCompositor.cpp:2291:
renderView->document()->settings() &&
On 2013/03/20 21:54:54, danakj wrote:
> m_renderView?

This had me confused. I wondered "how could this ever compile? I must have the
#PLATFORM thing wrong." But no, it just didn't compile. Apparently I.. I
uploaded without even compiling. So much shame.