Remove NonCompositedContentHost -- Take 2

Remove NonCompositedContentHost -- Take 2

This patch was previously reverted for crbug.com/242626
The new version fixed the issue with deferred commit never get resumed,
and the issue with transparent WebView.

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=151453

[trchen, 2013-05-25 01:43:37]

Hey James,
This patch fixed all the known issues. Can we try to land it again?

[jamesr, 2013-05-28 18:27:50]

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

https://codereview.chromium.org/15973002/diff/7001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebViewImpl.cpp:3616: // If we receive an
invalidation from WebKit while in deferred commit mode,
update the comment since this function isn't about invalidations

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

https://codereview.chromium.org/15973002/diff/7001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayerBacking.cpp:199:
m_graphicsLayer->setContentsOpaque(toRenderView(renderer())->frameView()->hasOpaqueBackground());
what updates this bit if WebView::setIsTransparent() is called after
createPrimaryGraphicsLayer()?

[trchen, 2013-05-28 22:52:33]

Uploaded a new version. PTAL!

On 2013/05/28 18:27:50, jamesr wrote:
>
https://codereview.chromium.org/15973002/diff/7001/Source/WebKit/chromium/src...
> File Source/WebKit/chromium/src/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/15973002/diff/7001/Source/WebKit/chromium/src...
> Source/WebKit/chromium/src/WebViewImpl.cpp:3616: // If we receive an
> invalidation from WebKit while in deferred commit mode,
> update the comment since this function isn't about invalidations

Done

>
https://codereview.chromium.org/15973002/diff/7001/Source/core/rendering/Rend...
> File Source/core/rendering/RenderLayerBacking.cpp (right):
> 
>
https://codereview.chromium.org/15973002/diff/7001/Source/core/rendering/Rend...
> Source/core/rendering/RenderLayerBacking.cpp:199:
>
m_graphicsLayer->setContentsOpaque(toRenderView(renderer())->frameView()->hasOpaqueBackground());
> what updates this bit if WebView::setIsTransparent() is called after
> createPrimaryGraphicsLayer()?

FrameView::didChangeBackgroundOpaqueness added.

[jamesr, 2013-05-28 23:03:52]

lgtm except for one thing

https://codereview.chromium.org/15973002/diff/16001/Source/core/page/FrameVie...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/15973002/diff/16001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:2080: void
FrameView::didChangeBackgroundOpaqueness()
I think this function would be clearer if this just called to its
RenderLayerBacking (if it had one) and the RLB figured out if it was the root
layer and if so set the appropriate thing on its GraphicsLayer.

[trchen, 2013-05-28 23:17:28]

On 2013/05/28 23:03:52, jamesr wrote:
> lgtm except for one thing
> 
>
https://codereview.chromium.org/15973002/diff/16001/Source/core/page/FrameVie...
> File Source/core/page/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/15973002/diff/16001/Source/core/page/FrameVie...
> Source/core/page/FrameView.cpp:2080: void
> FrameView::didChangeBackgroundOpaqueness()
> I think this function would be clearer if this just called to its
> RenderLayerBacking (if it had one) and the RLB figured out if it was the root
> layer and if so set the appropriate thing on its GraphicsLayer.

I was thinking the same thing, but it is a bit awkward.
Do we just name it RLB::didChangeBackgroundOpaqueness()? Someone later can be
surprised to find out the function is only called on frames. Do you think adding
a comment saying that it will be only called on RLBs owned by frames would
suffice?

[jamesr, 2013-05-28 23:22:04]

On 2013/05/28 23:17:28, trchen wrote:
> On 2013/05/28 23:03:52, jamesr wrote:
> > lgtm except for one thing
> > 
> >
>
https://codereview.chromium.org/15973002/diff/16001/Source/core/page/FrameVie...
> > File Source/core/page/FrameView.cpp (right):
> > 
> >
>
https://codereview.chromium.org/15973002/diff/16001/Source/core/page/FrameVie...
> > Source/core/page/FrameView.cpp:2080: void
> > FrameView::didChangeBackgroundOpaqueness()
> > I think this function would be clearer if this just called to its
> > RenderLayerBacking (if it had one) and the RLB figured out if it was the
root
> > layer and if so set the appropriate thing on its GraphicsLayer.
> 
> I was thinking the same thing, but it is a bit awkward.
> Do we just name it RLB::didChangeBackgroundOpaqueness()? Someone later can be
> surprised to find out the function is only called on frames. Do you think
adding
> a comment saying that it will be only called on RLBs owned by frames would
> suffice?

Well, I don't think you would want to call it
RLB::didChangeBackgroundOpaqueness().  Why not have FV::setIsTransparent call
through to RLB::setIsTransparent() and have that map through to the appropriate
GraphicsLayer?

[trchen, 2013-05-29 03:14:18]

On 2013/05/28 23:22:04, jamesr wrote:
> On 2013/05/28 23:17:28, trchen wrote:
> > On 2013/05/28 23:03:52, jamesr wrote:
> > > lgtm except for one thing
> > > 
> > >
> >
>
https://codereview.chromium.org/15973002/diff/16001/Source/core/page/FrameVie...
> > > File Source/core/page/FrameView.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/15973002/diff/16001/Source/core/page/FrameVie...
> > > Source/core/page/FrameView.cpp:2080: void
> > > FrameView::didChangeBackgroundOpaqueness()
> > > I think this function would be clearer if this just called to its
> > > RenderLayerBacking (if it had one) and the RLB figured out if it was the
> root
> > > layer and if so set the appropriate thing on its GraphicsLayer.
> > 
> > I was thinking the same thing, but it is a bit awkward.
> > Do we just name it RLB::didChangeBackgroundOpaqueness()? Someone later can
be
> > surprised to find out the function is only called on frames. Do you think
> adding
> > a comment saying that it will be only called on RLBs owned by frames would
> > suffice?
> 
> Well, I don't think you would want to call it
> RLB::didChangeBackgroundOpaqueness().  Why not have FV::setIsTransparent call
> through to RLB::setIsTransparent() and have that map through to the
appropriate
> GraphicsLayer?

Patchset 5/6/7 added RenderView::backgroundIsKnownToBeOpaqueInRect() as
discussed on chat. Does it still LGTY?

[jamesr, 2013-05-29 04:59:47]

Yup, looks good % the annotations

https://codereview.chromium.org/15973002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.h (left):

https://codereview.chromium.org/15973002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.h:52: virtual bool
backgroundIsKnownToBeOpaqueInRect(const LayoutRect& localRect) const OVERRIDE
FINAL;
why are you removing the OVERRIDE? FINAL doesn't apply any more but OVERRIDE
still should, right?

https://codereview.chromium.org/15973002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderView.h (right):

https://codereview.chromium.org/15973002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderView.h:199: virtual bool
backgroundIsKnownToBeOpaqueInRect(const LayoutRect& localRect) const;
this should be OVERRIDE FINAL, right?

[trchen, 2013-05-29 22:32:29]

Oh I was confused whether it is encouraged to remove OVERRIDE/FINAL. Will add
those back.

[commit-bot: I haz the power, 2013-05-29 23:02:52]

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

[commit-bot: I haz the power, 2013-05-30 04:04:15]

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

[commit-bot: I haz the power, 2013-05-30 06:04:15]

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

[commit-bot: I haz the power, 2013-05-30 07:06:27]

Change committed as 151453