Remove NonCompositedContentHost

Remove NonCompositedContentHost

Safari uses a hybrid scrolling approach. For contents that require
compositing, they will create compositing layers and will use layer-based
scrolling. For non-composited contents, they will be painted to a native
window and will be scrolled with a OS-provided backing.

Chromium doesn't use this native window based scrolling. Instead,
NonCompositedContentHost was introduced to provide a replacement for
the OS-provided backing.

Now we no longer need to maintain compatibility with the native window
approach. This patch removes RenderLayerBacking::paintsIntoWindow().
Main frame contents will be painted onto its RenderView layer (which was
always created but only used as container for child layer), and
NonCompositedContentHost becomes redundant which gets removed too.

R=jamesr@chromium.org

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

[trchen, 2013-04-23 04:08:10]

Hey James,
I think it is not too difficult to remove NonCompositedContentHost. So far I
only see text failures due to (drawsContent 1) on the main RenderView layer. Do
you know if I overlooked anything?

Not ready for submit yet. LayerTreeHostImpl::CapturePicture probably will be
broken.

[jamesr, 2013-04-23 06:11:50]

Interesting.  Here are a few things that NonCompositedContentHost knows how to
do that (afaik) RenderLayerCompositor doesn't:

1.) Marks itself as needing LCD text
2.) Sets NCCH::m_layerAdjust to deal with RTL root layer scrolling
3.) Maps root scroll position into appropriate layers

What do you envision doing these things?

[trchen, 2013-04-23 21:13:01]

On 2013/04/23 06:11:50, jamesr wrote:
> Interesting.  Here are a few things that NonCompositedContentHost knows how to
> do that (afaik) RenderLayerCompositor doesn't:
> 
> 1.) Marks itself as needing LCD text

I think it is CC's responsibility to decide whether LCD text should be enabled.
As it requires that both target space transform and screen space transform to be
pixel-snapped. Only CC knows those information.

However aelias@ mentioned that we also need the layer to be opaque. I believe it
is true when background-attachment:scroll(default) is used. I'm not sure what is
the current behavior for other values. vollick@ probably knows more than I do.

> 2.) Sets NCCH::m_layerAdjust to deal with RTL root layer scrolling

RTL frame issue was fixed in http://webk.it/112088
There is some problem with pinch zoom though. Will get that solved before
submitting this.

> 3.) Maps root scroll position into appropriate layers

It is done in the same way composited subframes are scrolled. Enabled in the
first hunk in RenderLayerCompositor.cpp.

> What do you envision doing these things?

[trchen, 2013-04-23 22:05:57]

As discussed with jamesr@ offline, we need to enable LCD text for main frame
explicitly until CC become able to determine it automatically.

Doing this in RenderLayerBacking::createPrimaryGraphicsLayer(), since it is
where other main frame layer property gets initialized.

As a side note for vollick@, it seems like main frame primary layer is always
opaque. You might want to change that when there is fixed-background layer.

[jamesr, 2013-04-24 03:16:35]

I'm not sure I follow the LinkHighlight changes.  Other than that, this looks
great.

https://codereview.chromium.org/13959008/diff/6002/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/13959008/diff/6002/Source/core/rendering/Rend...
Source/core/rendering/RenderLayerBacking.cpp:202: WebKit::WebContentLayer* layer
=
static_cast<WebCore::GraphicsLayerChromium*>(m_graphicsLayer.get())->contentLayer();
we need to fold GraphicsLayerChromium into GraphicsLayer already

[trchen, 2013-04-24 03:30:08]

On 2013/04/24 03:16:35, jamesr wrote:
> I'm not sure I follow the LinkHighlight changes.  Other than that, this looks
> great.

Because RenderView layer didn't actually paint contents, highlights that belong
to RenderView layer were redirected to NCCH. This patch  reverts that.

>
https://codereview.chromium.org/13959008/diff/6002/Source/core/rendering/Rend...
> File Source/core/rendering/RenderLayerBacking.cpp (right):
> 
>
https://codereview.chromium.org/13959008/diff/6002/Source/core/rendering/Rend...
> Source/core/rendering/RenderLayerBacking.cpp:202: WebKit::WebContentLayer*
layer
> =
>
static_cast<WebCore::GraphicsLayerChromium*>(m_graphicsLayer.get())->contentLayer();
> we need to fold GraphicsLayerChromium into GraphicsLayer already

Yep, too many things need to be done. Let's add that to my todo list.

[enne (OOO), 2013-04-24 14:00:24]

https://codereview.chromium.org/13959008/diff/6002/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/13959008/diff/6002/Source/core/rendering/Rend...
Source/core/rendering/RenderLayerBacking.cpp:207: // FIXME: Remove LCD text
setting after it is implemented in chromium.
I suspect but am not positive that this could be removed at this point.  As long
as the graphics layer gets its background color set to opaque (which it should),
we should automatically turn on LCD text.

Can you verify that with this change that this layer is still marked as contents
opaque so that we turn off blending?

[trchen, 2013-04-24 21:16:51]

https://codereview.chromium.org/13959008/diff/6002/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayerBacking.cpp (left):

https://codereview.chromium.org/13959008/diff/6002/Source/core/rendering/Rend...
Source/core/rendering/RenderLayerBacking.cpp:197:
m_graphicsLayer->setContentsOpaque(true);
Here, look at me.

https://codereview.chromium.org/13959008/diff/6002/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/13959008/diff/6002/Source/core/rendering/Rend...
Source/core/rendering/RenderLayerBacking.cpp:207: // FIXME: Remove LCD text
setting after it is implemented in chromium.
On 2013/04/24 14:00:24, enne wrote:
> I suspect but am not positive that this could be removed at this point.  As
long
> as the graphics layer gets its background color set to opaque (which it
should),
> we should automatically turn on LCD text.

I'm positive. Just found that setUseLCDText is an empty stub in
WebContentLayerImpl. lol
Will remove this hunk along with the API.

> Can you verify that with this change that this layer is still marked as
contents
> opaque so that we turn off blending?

Yes see my comments above. Is it the sufficient condition?

[enne (OOO), 2013-04-24 22:46:53]

On 2013/04/24 21:16:51, trchen wrote:

> Yes see my comments above. Is it the sufficient condition?

Thanks for pointing that out! That seems sufficient.  

I don't have any other concerns around lcd text.  This lgtm so long as it still
passes the RTL layout tests.

[aelias_OOO_until_Jul13, 2013-04-24 22:49:45]

https://codereview.chromium.org/13959008/diff/6002/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/13959008/diff/6002/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebViewImpl.cpp:4060: void
WebViewImpl::updateLayerTreeViewport()
Looks like this method isn't needed anymore.  Please delete it and call
setPageScaleFactorAndLimits from deviceOrPageScaleFactorChanged.

[enne (OOO), 2013-05-09 22:22:06]

Ping.  Are you still planning to land this?  I see other folks making changes to
NCCH, e.g. https://codereview.chromium.org/13462003/.

[trchen, 2013-05-09 22:24:54]

On 2013/05/09 22:22:06, enne wrote:
> Ping.  Are you still planning to land this?  I see other folks making changes
to
> NCCH, e.g. https://codereview.chromium.org/13462003/.

I still have problems with RTL layout tests.
I'm working on this one first: https://codereview.chromium.org/14807003/
Hopefully it will solve most of the problems.

[Ian Vollick, 2013-05-10 15:10:38]

On 2013/05/09 22:24:54, trchen wrote:
> On 2013/05/09 22:22:06, enne wrote:
> > Ping.  Are you still planning to land this?  I see other folks making
changes
> to
> > NCCH, e.g. https://codereview.chromium.org/13462003/.
> 
> I still have problems with RTL layout tests.
> I'm working on this one first: https://codereview.chromium.org/14807003/
> Hopefully it will solve most of the problems.

This patch will conflict with https://codereview.chromium.org/13462003/ a
little, but I don't think it will be too bad. The bit of plumbing in that patch
that's used to hide the soon-to-be-deleted NCCH will just have to be updated so
we affect the right graphics layer in the new world order. I would be happy to
help with this or upload a patch.

[trchen, 2013-05-15 04:42:47]

Hello reviewers, I rebased the patch to the latest tree and rebaselined all
affected tests. The RTL issue was solved in another patch I submitted yesterday.
I think this patch is almost complete. PTAL!

To summarize what are changed:
1. The main frame RenderView layer will have (drawsContent 1) since the
originally non-composited contents will be painted on it.
2. Likewise, the repaint rects for the native window are now moved to the main
frame RenderView layer.
3. The previous patch forgot to set background color on LayerTreeHost. It was
originally done in WebViewImpl::paintRootLayer during painting NCCH. Now it will
be done in WebViewImpl::layout.

[aelias_OOO_until_Jul13, 2013-05-15 08:18:28]

On 2013/05/15 04:42:47, trchen wrote:
> 3. The previous patch forgot to set background color on LayerTreeHost. It was
> originally done in WebViewImpl::paintRootLayer during painting NCCH. Now it
will
> be done in WebViewImpl::layout.

WebViewImpl::layout is just a minor side call by which the embedder can force a
layout, there's no guarantee it will ever get called.

[trchen, 2013-05-15 15:27:02]

On 2013/05/15 08:18:28, aelias wrote:
> On 2013/05/15 04:42:47, trchen wrote:
> > 3. The previous patch forgot to set background color on LayerTreeHost. It
was
> > originally done in WebViewImpl::paintRootLayer during painting NCCH. Now it
> will
> > be done in WebViewImpl::layout.
> 
> WebViewImpl::layout is just a minor side call by which the embedder can force
a
> layout, there's no guarantee it will ever get called.

It will be called during every commit:
ThreadProxy::BeginFrame() -> Layout() -> LayerTreeHost::Layout() ->
RenderWidgetCompositor::Layout() -> WebViewImpl::layout()
I agree with you that it feels like a weak assumption though. I think adding a
WebViewImpl::willCommit() would make it more explicit.

[danakj, 2013-05-15 16:03:46]

On Wed, May 15, 2013 at 11:27 AM, <trchen@chromium.org> wrote:

> On 2013/05/15 08:18:28, aelias wrote:
>
>> On 2013/05/15 04:42:47, trchen wrote:
>> > 3. The previous patch forgot to set background color on LayerTreeHost.
>> It
>>
> was
>
>> > originally done in WebViewImpl::paintRootLayer during painting NCCH.
>> Now it
>> will
>> > be done in WebViewImpl::layout.
>>
>
>  WebViewImpl::layout is just a minor side call by which the embedder can
>> force
>>
> a
>
>> layout, there's no guarantee it will ever get called.
>>
>
> It will be called during every commit:
> ThreadProxy::BeginFrame() -> Layout() -> LayerTreeHost::Layout() ->
> RenderWidgetCompositor::**Layout() -> WebViewImpl::layout()
> I agree with you that it feels like a weak assumption though. I think
> adding a
> WebViewImpl::willCommit() would make it more explicit.
>

Make sure you consider the single thread case as well. It's called from
RenderWidget, but ordering can matter. I also made this change but it was
reverted due to asserts saying it wasn't sufficient in some case.

See the CLs here: https://code.google.com/p/chromium/issues/detail?id=231925



>
>
https://codereview.chromium.**org/13959008/<https://codereview.chromium.org/1...
>

[trchen, 2013-05-16 04:44:43]

On 2013/05/15 16:03:46, danakj wrote:
> On Wed, May 15, 2013 at 11:27 AM, <mailto:trchen@chromium.org> wrote:
> 
> > On 2013/05/15 08:18:28, aelias wrote:
> >
> >> On 2013/05/15 04:42:47, trchen wrote:
> >> > 3. The previous patch forgot to set background color on LayerTreeHost.
> >> It
> >>
> > was
> >
> >> > originally done in WebViewImpl::paintRootLayer during painting NCCH.
> >> Now it
> >> will
> >> > be done in WebViewImpl::layout.
> >>
> >
> >  WebViewImpl::layout is just a minor side call by which the embedder can
> >> force
> >>
> > a
> >
> >> layout, there's no guarantee it will ever get called.
> >>
> >
> > It will be called during every commit:
> > ThreadProxy::BeginFrame() -> Layout() -> LayerTreeHost::Layout() ->
> > RenderWidgetCompositor::**Layout() -> WebViewImpl::layout()
> > I agree with you that it feels like a weak assumption though. I think
> > adding a
> > WebViewImpl::willCommit() would make it more explicit.
> >
> 
> Make sure you consider the single thread case as well. It's called from
> RenderWidget, but ordering can matter. I also made this change but it was
> reverted due to asserts saying it wasn't sufficient in some case.
> 
> See the CLs here: https://code.google.com/p/chromium/issues/detail?id=231925

I figured out your assertion failure. It was due to inconsistent handling with
invalid colors. See my report in the thread:
https://code.google.com/p/chromium/issues/detail?id=231925#c14

However I agree you make a point that SingleThreadProxy doesn't call layout() at
all. Do you suggest any extra places to setBackgroundColor() besides
enterForceCompositingMode() and layout()?

[trchen, 2013-05-16 04:52:33]

PTAL. :)
I added setBackgroundColor to setIsAcceleratedCompositingActive. IMO it makes
more sense than enterForceCompositingMode.

On 2013/05/16 04:44:43, trchen wrote:
> On 2013/05/15 16:03:46, danakj wrote:
> > On Wed, May 15, 2013 at 11:27 AM, <mailto:trchen@chromium.org> wrote:
> > 
> > > On 2013/05/15 08:18:28, aelias wrote:
> > >
> > >> On 2013/05/15 04:42:47, trchen wrote:
> > >> > 3. The previous patch forgot to set background color on LayerTreeHost.
> > >> It
> > >>
> > > was
> > >
> > >> > originally done in WebViewImpl::paintRootLayer during painting NCCH.
> > >> Now it
> > >> will
> > >> > be done in WebViewImpl::layout.
> > >>
> > >
> > >  WebViewImpl::layout is just a minor side call by which the embedder can
> > >> force
> > >>
> > > a
> > >
> > >> layout, there's no guarantee it will ever get called.
> > >>
> > >
> > > It will be called during every commit:
> > > ThreadProxy::BeginFrame() -> Layout() -> LayerTreeHost::Layout() ->
> > > RenderWidgetCompositor::**Layout() -> WebViewImpl::layout()
> > > I agree with you that it feels like a weak assumption though. I think
> > > adding a
> > > WebViewImpl::willCommit() would make it more explicit.
> > >
> > 
> > Make sure you consider the single thread case as well. It's called from
> > RenderWidget, but ordering can matter. I also made this change but it was
> > reverted due to asserts saying it wasn't sufficient in some case.
> > 
> > See the CLs here: https://code.google.com/p/chromium/issues/detail?id=231925
> 
> I figured out your assertion failure. It was due to inconsistent handling with
> invalid colors. See my report in the thread:
> https://code.google.com/p/chromium/issues/detail?id=231925#c14
> 
> However I agree you make a point that SingleThreadProxy doesn't call layout()
at
> all. Do you suggest any extra places to setBackgroundColor() besides
> enterForceCompositingMode() and layout()?

[enne (OOO), 2013-05-16 18:08:12]

https://codereview.chromium.org/13959008/diff/33001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/13959008/diff/33001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.cpp:3974:
m_layerTreeView->setBackgroundColor(backgroundColor());
If layout() doesn't get called with the single thread proxy, how does the this
get updated when the background color changes when accelerated compositing is
active?

[danakj, 2013-05-16 18:09:44]

https://codereview.chromium.org/13959008/diff/33001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/13959008/diff/33001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.cpp:3974:
m_layerTreeView->setBackgroundColor(backgroundColor());
On 2013/05/16 18:08:12, enne wrote:
> If layout() doesn't get called with the single thread proxy, how does the this
> get updated when the background color changes when accelerated compositing is
> active?

It comes from RenderWidget instead.

RenderWidget::DoDeferredUpdate
RenderWidget::OnPaintAtSize
RenderWidget::OnSnapshotHelper

All call this.

[enne (OOO), 2013-05-16 20:48:43]

Ok, thanks for the explanation.

My only remaining question is that I don't understand what's causing the changes
to the sizes in
LayoutTests/compositing/layer-creation/fixed-position-out-of-view-scaled-expected.txt
and the corresponding Mac version?

[trchen, 2013-05-16 21:23:16]

On 2013/05/16 20:48:43, enne wrote:
> Ok, thanks for the explanation.
> 
> My only remaining question is that I don't understand what's causing the
changes
> to the sizes in
>
LayoutTests/compositing/layer-creation/fixed-position-out-of-view-scaled-expected.txt
> and the corresponding Mac version?

I bet you mean
LayoutTests/compositing/visibility/visibility-image-layers-dynamic.html
It is because it dumps the layer tree 3 times and each time it appends the text
results to the DOM tree. The text results are one line longer than before, so
for the second and third iteration the layer become one line larger.

[enne (OOO), 2013-05-16 21:26:04]

Oh, layout tests.  :(

Thanks for the explanation.  This patch lgtm (again).

[commit-bot: I haz the power, 2013-05-17 21:53:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/13959008/45001

[commit-bot: I haz the power, 2013-05-17 21:54:50]

Failed to apply patch for Source/Platform/chromium/public/WebContentLayer.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/Platform/chromium/public/WebContentLayer.h
  Hunk #1 FAILED at 48.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/Platform/chromium/public/WebContentLayer.h.rej

Patch:       Source/Platform/chromium/public/WebContentLayer.h
Index: Source/Platform/chromium/public/WebContentLayer.h
diff --git a/Source/Platform/chromium/public/WebContentLayer.h
b/Source/Platform/chromium/public/WebContentLayer.h
index
36a23fc8ed8a729f1432ef6a92c59ec1d89bee76..0b13fa1d0f8b52b7a4805fee37084051356d83e6
100644
--- a/Source/Platform/chromium/public/WebContentLayer.h
+++ b/Source/Platform/chromium/public/WebContentLayer.h
@@ -48,11 +48,6 @@ public:
     // to false.
     virtual void setAutomaticallyComputeRasterScale(bool) { }
 
-    // Set to render text in this layer with LCD antialiasing. Only set if you
know that this layer will be
-    // drawn in a way where this makes sense - i.e. opaque background, not
rotated or scaled, etc.
-    // Defaults to false;
-    virtual void setUseLCDText(bool) = 0;
-
     // Set to draw a system-defined checkerboard if the compositor would
otherwise draw a tile in this layer
     // and the actual contents are unavailable. If false, the compositor will
draw the layer's background color
     // for these tiles.

[commit-bot: I haz the power, 2013-05-17 22:12:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/13959008/50001

[commit-bot: I haz the power, 2013-05-17 22:39:47]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[jamesr, 2013-05-17 23:14:03]

lgtm

[commit-bot: I haz the power, 2013-05-17 23:16:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/13959008/50001

[commit-bot: I haz the power, 2013-05-18 00:15:39]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-20 22:36:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/13959008/72001

[commit-bot: I haz the power, 2013-05-20 23:43:18]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-21 00:55:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/13959008/83001

[commit-bot: I haz the power, 2013-05-21 01:53:53]

Change committed as 150733