Make RenderWidget responsible for the composited view's lifetime

Make RenderWidget responsible for the composited view's lifetime

Chromium side of https://bugs.webkit.org/show_bug.cgi?id=105071.
content::RenderWidget constructs a WebLayerTreeView when WebKit
asks it and then retains ownership. This has to be destroyed
before the WebWidget is closed since the WLTVClient is (currently)
a WebKit object.


BUG=156175
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175303

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175827

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176271

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176707

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178256

[jamesr, 2012-12-16 02:01:15]

I've moved a number of calls from WebViewImpl to RenderWidget - after this lands
I'll remove the WebViewImpl versions and (where possible) cut them out of the
WebLayerTreeView interface completely.

More importantly, this changes the type of RenderWidget::web_layer_tree_view_ to
WebLayerTreeViewImpl, so we can directly access cc::LayerTreeHost.

[enne (OOO), 2012-12-16 20:02:59]

https://codereview.chromium.org/11575049/diff/4006/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11575049/diff/4006/content/renderer/render_wi...
content/renderer/render_widget.cc:855:
web_layer_tree_view_->updateAnimations(0.0);
Why is this zero? It seems like if you're hoisting this up into render widget
that you need to start passing a real time.

[jamesr, 2012-12-17 01:02:16]

On 2012/12/16 20:02:59, enne wrote:
>
https://codereview.chromium.org/11575049/diff/4006/content/renderer/render_wi...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/11575049/diff/4006/content/renderer/render_wi...
> content/renderer/render_widget.cc:855:
> web_layer_tree_view_->updateAnimations(0.0);
> Why is this zero? It seems like if you're hoisting this up into render widget
> that you need to start passing a real time.

Good catch! Fixed.  I punched straight through to cc::LayerTreeHost to avoid
doing a base::TimeTicks -> double -> base::TimeTicks conversion.

Also important to know - this compiles but doesn't work correctly without
https://bugs.webkit.org/show_bug.cgi?id=105071

[enne (OOO), 2012-12-17 17:48:34]

lgtm

[commit-bot: I haz the power, 2012-12-18 03:47:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/18001

[commit-bot: I haz the power, 2012-12-20 07:40:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/39002

[commit-bot: I haz the power, 2012-12-20 07:40:23]

Presubmit check for 11575049-39002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content

Presubmit checks took 1.7s to calculate.

[jamesr, 2012-12-20 19:41:45]

Antoine - could you OWNERS and CQ if you get a chance? Thanks

[commit-bot: I haz the power, 2012-12-20 19:46:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/39002

[jamesr, 2012-12-20 19:46:18]

On 2012/12/20 19:41:45, jamesr wrote:
> Antoine - could you OWNERS and CQ if you get a chance? Thanks

Eh, you're on vacation too.  Will TBR :)

[commit-bot: I haz the power, 2012-12-20 21:25:01]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-20 22:02:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/39002

[commit-bot: I haz the power, 2012-12-20 22:02:03]

Failed to apply patch for
webkit/compositor_bindings/web_layer_tree_view_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file webkit/compositor_bindings/web_layer_tree_view_impl.cc
  Hunk #2 FAILED at 18.
  1 out of 2 hunks FAILED -- saving rejects to file
webkit/compositor_bindings/web_layer_tree_view_impl.cc.rej

Patch:       webkit/compositor_bindings/web_layer_tree_view_impl.cc
Index: webkit/compositor_bindings/web_layer_tree_view_impl.cc
diff --git a/webkit/compositor_bindings/web_layer_tree_view_impl.cc
b/webkit/compositor_bindings/web_layer_tree_view_impl.cc
index
312913251615aefecead807b00abf0a0a5cfcdc7..5184773e496c18171f2671039045fd1172cf0c39
100644
--- a/webkit/compositor_bindings/web_layer_tree_view_impl.cc
+++ b/webkit/compositor_bindings/web_layer_tree_view_impl.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "web_layer_tree_view_impl.h"
+#include "webkit/compositor_bindings/web_layer_tree_view_impl.h"
 
 #include "base/command_line.h"
 #include "cc/font_atlas.h"
@@ -18,272 +18,258 @@
 #include
"third_party/WebKit/Source/Platform/chromium/public/WebLayerTreeView.h"
 #include
"third_party/WebKit/Source/Platform/chromium/public/WebRenderingStats.h"
 #include "third_party/WebKit/Source/Platform/chromium/public/WebSize.h"
-#include "web_layer_impl.h"
-#include "web_to_ccinput_handler_adapter.h"
+#include "webkit/compositor_bindings/web_layer_impl.h"
 #include "webkit/compositor_bindings/web_rendering_stats_impl.h"
+#include "webkit/compositor_bindings/web_to_ccinput_handler_adapter.h"
 
-using namespace cc;
 
 namespace WebKit {
 
 WebLayerTreeViewImpl::WebLayerTreeViewImpl(WebLayerTreeViewClient* client)
-    : m_client(client)
-{
+    : client_(client) {
 }
 
-WebLayerTreeViewImpl::~WebLayerTreeViewImpl()
-{
+WebLayerTreeViewImpl::~WebLayerTreeViewImpl() {
 }
 
-bool WebLayerTreeViewImpl::initialize(const WebLayerTreeView::Settings&
webSettings, scoped_ptr<Thread> implThread)
-{
-    LayerTreeSettings settings;
-    settings.acceleratePainting = webSettings.acceleratePainting;
-    settings.renderVSyncEnabled = webSettings.renderVSyncEnabled;
-    settings.perTilePaintingEnabled = webSettings.perTilePaintingEnabled;
-    settings.acceleratedAnimationEnabled =
webSettings.acceleratedAnimationEnabled;
-    settings.pageScalePinchZoomEnabled = webSettings.pageScalePinchZoomEnabled;
-    settings.refreshRate = webSettings.refreshRate;
-    settings.defaultTileSize = webSettings.defaultTileSize;
-    settings.maxUntiledLayerSize = webSettings.maxUntiledLayerSize;
-    settings.initialDebugState.showFPSCounter = webSettings.showFPSCounter;
-    settings.initialDebugState.showPaintRects = webSettings.showPaintRects;
-    settings.initialDebugState.showPlatformLayerTree =
webSettings.showPlatformLayerTree;
-    settings.initialDebugState.showDebugBorders = webSettings.showDebugBorders;
-    m_layerTreeHost = LayerTreeHost::create(this, settings, implThread.Pass());
-    if (!m_layerTreeHost.get())
-        return false;
-    return true;
+bool WebLayerTreeViewImpl::initialize(
+    const WebLayerTreeView::Settings& web_settings,
+    scoped_ptr<cc::Thread> impl_thread) {
+  cc::LayerTreeSettings settings;
+  settings.acceleratePainting = web_settings.acceleratePainting;
+  settings.renderVSyncEnabled = web_settings.renderVSyncEnabled;
+  settings.perTilePaintingEnabled = web_settings.perTilePaintingEnabled;
+  settings.acceleratedAnimationEnabled =
+      web_settings.acceleratedAnimationEnabled;
+  settings.pageScalePinchZoomEnabled = web_settings.pageScalePinchZoomEnabled;
+  settings.refreshRate = web_settings.refreshRate;
+  settings.defaultTileSize = web_settings.defaultTileSize;
+  settings.maxUntiledLayerSize = web_settings.maxUntiledLayerSize;
+  settings.initialDebugState.showFPSCounter = web_settings.showFPSCounter;
+  settings.initialDebugState.showPaintRects = web_settings.showPaintRects;
+  settings.initialDebugState.showPlatformLayerTree =
+      web_settings.showPlatformLayerTree;
+  settings.initialDebugState.showDebugBorders = web_settings.showDebugBorders;
+  layer_tree_host_ = cc::LayerTreeHost::create(this,
+                                               settings,
+                                               impl_thread.Pass());
+  if (!layer_tree_host_)
+    return false;
+  return true;
 }
 
-void WebLayerTreeViewImpl::setSurfaceReady()
-{
-    m_layerTreeHost->setSurfaceReady();
+cc::LayerTreeHost* WebLayerTreeViewImpl::layer_tree_host() const {
+  return layer_tree_host_.get();
 }
 
-void WebLayerTreeViewImpl::setRootLayer(const WebLayer& root)
-{
-    m_layerTreeHost->setRootLayer(static_cast<const
WebLayerImpl*>(&root)->layer());
+void WebLayerTreeViewImpl::setSurfaceReady() {
+  layer_tree_host_->setSurfaceReady();
 }
 
-void WebLayerTreeViewImpl::clearRootLayer()
-{
-    m_layerTreeHost->setRootLayer(scoped_refptr<Layer>());
+void WebLayerTreeViewImpl::setRootLayer(const WebLayer& root) {
+  layer_tree_host_->setRootLayer(
+      static_cast<const WebLayerImpl*>(&root)->layer());
 }
 
-void WebLayerTreeViewImpl::setViewportSize(const WebSize& layoutViewportSize,
const WebSize& deviceViewportSize)
-{
-    if (!deviceViewportSize.isEmpty())
-        m_layerTreeHost->setViewportSize(layoutViewportSize,
deviceViewportSize);
-    else
-        m_layerTreeHost->setViewportSize(layoutViewportSize,
layoutViewportSize);
+void WebLayerTreeViewImpl::clearRootLayer() {
+  layer_tree_host_->setRootLayer(scoped_refptr<cc::Layer>());
 }
 
-WebSize WebLayerTreeViewImpl::layoutViewportSize() const
-{
-    return m_layerTreeHost->layoutViewportSize();
+void WebLayerTreeViewImpl::setViewportSize(
+    const WebSize& layout_viewport_size, const WebSize& device_viewport_size) {
+  if (!device_viewport_size.isEmpty()) {
+    layer_tree_host_->setViewportSize(layout_viewport_size,
+                                      device_viewport_size);
+  } else {
+    layer_tree_host_->setViewportSize(layout_viewport_size,
+                                      layout_viewport_size);
+  }
 }
 
-WebSize WebLayerTreeViewImpl::deviceViewportSize() const
-{
-    return m_layerTreeHost->deviceViewportSize();
+WebSize WebLayerTreeViewImpl::layoutViewportSize() const {
+  return layer_tree_host_->layoutViewportSize();
 }
 
-WebFloatPoint WebLayerTreeViewImpl::adjustEventPointForPinchZoom(const
WebFloatPoint& point) const
-{
-    return m_layerTreeHost->adjustEventPointForPinchZoom(point);
+WebSize WebLayerTreeViewImpl::deviceViewportSize() const {
+  return layer_tree_host_->deviceViewportSize();
 }
 
-void WebLayerTreeViewImpl::setDeviceScaleFactor(const float deviceScaleFactor)
-{
-    m_layerTreeHost->setDeviceScaleFactor(deviceScaleFactor);
+WebFloatPoint WebLayerTreeViewImpl::adjustEventPointForPinchZoom(
+    const WebFloatPoint& point) const {
+  return layer_tree_host_->adjustEventPointForPinchZoom(point);
 }
 
-float WebLayerTreeViewImpl::deviceScaleFactor() const
-{
-    return m_layerTreeHost->deviceScaleFactor();
+void WebLayerTreeViewImpl::setDeviceScaleFactor(float device_scale_factor) {
+  layer_tree_host_->setDeviceScaleFactor(device_scale_factor);
 }
 
-void WebLayerTreeViewImpl::setBackgroundColor(WebColor color)
-{
-    m_layerTreeHost->setBackgroundColor(color);
+float WebLayerTreeViewImpl::deviceScaleFactor() const {
+  return layer_tree_host_->deviceScaleFactor();
 }
 
-void WebLayerTreeViewImpl::setHasTransparentBackground(bool transparent)
-{
-    m_layerTreeHost->setHasTransparentBackground(transparent);
+void WebLayerTreeViewImpl::setBackgroundColor(WebColor color) {
+  layer_tree_host_->setBackgroundColor(color);
 }
 
-void WebLayerTreeViewImpl::setVisible(bool visible)
-{
-    m_layerTreeHost->setVisible(visible);
+void WebLayerTreeViewImpl::setHasTransparentBackground(bool transparent) {
+  layer_tree_host_->setHasTransparentBackground(transparent);
 }
 
-void WebLayerTreeViewImpl::setPageScaleFactorAndLimits(float pageScaleFactor,
float minimum, float maximum)
-{
-    m_layerTreeHost->setPageScaleFactorAndLimits(pageScaleFactor, minimum,
maximum);
+void WebLayerTreeViewImpl::setVisible(bool visible) {
+  layer_tree_host_->setVisible(visible);
 }
 
-void WebLayerTreeViewImpl::startPageScaleAnimation(const WebPoint& scroll, bool
useAnchor, float newPageScale, double durationSec)
-{
-    base::TimeDelta duration = base::TimeDelta::FromMicroseconds(durationSec *
base::Time::kMicrosecondsPerSecond);
-    m_layerTreeHost->startPageScaleAnimation(gfx::Vector2d(scroll.x, scroll.y),
useAnchor, newPageScale, duration);
+void WebLayerTreeViewImpl::setPageScaleFactorAndLimits(float page_scale_factor,
+                                                       float minimum,
+                                                       float maximum) {
+  layer_tree_host_->setPageScaleFactorAndLimits(page_scale_factor,
+                                                minimum, maximum);
 }
 
-void WebLayerTreeViewImpl::setNeedsAnimate()
-{
-    m_layerTreeHost->setNeedsAnimate();
+void WebLayerTreeViewImpl::startPageScaleAnimation(const WebPoint& scroll,
+                                                   bool use_anchor,
+                                                   float new_page_scale,
+                                                   double duration_sec) {
+  int64 duration_us = duration_sec * base::Time::kMicrosecondsPerSecond;
+  base::TimeDelta duration = base::TimeDelta::FromMicroseconds(duration_us);
+  layer_tree_host_->startPageScaleAnimation(gfx::Vector2d(scroll.x, scroll.y),
+                                            use_anchor,
+                                            new_page_scale,
+                                            duration);
 }
 
-void WebLayerTreeViewImpl::setNeedsRedraw()
-{
-    m_layerTreeHost->setNeedsRedraw();
+void WebLayerTreeViewImpl::setNeedsAnimate() {
+  layer_tree_host_->setNeedsAnimate();
 }
 
-bool WebLa…
(message too large)

[Leandro Graciá Gil, 2013-01-04 17:23:49]

Any updates on this? I have a patch that depends on this one.

[jamesr, 2013-01-04 18:07:53]

On 2013/01/04 17:23:49, Leandro Graciá Gil wrote:
> Any updates on this? I have a patch that depends on this one.

With luck, will land today.  Just needed a rebase

[commit-bot: I haz the power, 2013-01-04 18:29:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/52001

[jamesr, 2013-01-04 19:24:48]

https://codereview.chromium.org/11575049/diff/52001/content/renderer/render_w...
File content/renderer/render_widget.cc (left):

https://codereview.chromium.org/11575049/diff/52001/content/renderer/render_w...
content/renderer/render_widget.cc:1072: webwidget_->composite(false);
oh snap, this change might break render_widget_fullscreen_pepper

[jamesr, 2013-01-04 21:20:39]

This patch does make some bad assumptions about when compositing is active -
with a RenderWidgetFullscreenPepper behind the WebWidget interface, we can have
is_accelerated_compositing_active_ set and have no WebLayerTreeView.

[jamesr, 2013-01-04 21:45:50]

With render_widget_fullscreen_pepper's PepperWidget, we can have RenderWidgets
that are composited but that do not have  web_layer_tree_view_s or WebViewImpls
behind the WebWidget interface.  This means we have to null-check
web_layer_tree_view_ even when accelerated compositing is active and go through
the WebWidget interface for things like composite().

+cc brettw as an FYI.  I've verified that this works for fullscreen youtube
(with --ignore-gpu-blacklist so it goes down the GPU path).

[commit-bot: I haz the power, 2013-01-04 22:32:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/63001

[commit-bot: I haz the power, 2013-01-04 22:32:07]

Presubmit check for 11575049-63001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content

Presubmit checks took 1.5s to calculate.

[commit-bot: I haz the power, 2013-01-04 22:42:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/63001

[commit-bot: I haz the power, 2013-01-05 01:23:06]

Retried try job too often on win_rel for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2013-01-05 01:25:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/63001

[commit-bot: I haz the power, 2013-01-05 03:41:57]

Retried try job too often on win_rel for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2013-01-05 19:20:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/63001

[commit-bot: I haz the power, 2013-01-05 19:44:17]

Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests,
unit_tests

[commit-bot: I haz the power, 2013-01-05 19:49:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/63001

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

Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests,
unit_tests

[commit-bot: I haz the power, 2013-01-06 03:14:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/63001

[commit-bot: I haz the power, 2013-01-06 03:44:55]

Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests,
unit_tests

[jamesr, 2013-01-06 22:59:36]

Turns out the mac_asan compile failure is real, but it only happens when using
the 10.6 SDK and apparently the mac/mac_rel bots use 10.7.  Fix here:
https://codereview.chromium.org/11794019

[Sami, 2013-01-07 14:42:49]

https://codereview.chromium.org/11575049/diff/63001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11575049/diff/63001/content/renderer/render_w...
content/renderer/render_widget.cc:1639: if (is_accelerated_compositing_active_/*
&& web_layer_tree_view_*/) {
Drive by nit: comment left in by mistake?

[piman, 2013-01-07 18:34:54]

lgtm

[jamesr, 2013-01-08 19:10:16]

On 2013/01/07 14:42:49, Sami wrote:
>
https://codereview.chromium.org/11575049/diff/63001/content/renderer/render_w...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/11575049/diff/63001/content/renderer/render_w...
> content/renderer/render_widget.cc:1639: if
(is_accelerated_compositing_active_/*
> && web_layer_tree_view_*/) {
> Drive by nit: comment left in by mistake?

Good catch!  This should just be removed

[commit-bot: I haz the power, 2013-01-08 19:12:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/75004

[commit-bot: I haz the power, 2013-01-08 19:13:56]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-08 19:16:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/75004

[commit-bot: I haz the power, 2013-01-08 19:17:48]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-08 20:14:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/75004

[commit-bot: I haz the power, 2013-01-09 00:57:53]

Retried try job too often on win_rel for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2013-01-09 08:08:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/75004

[commit-bot: I haz the power, 2013-01-09 09:59:35]

Retried try job too often on win_rel for step(s) interactive_ui_tests

[jamesr, 2013-01-09 17:53:35]

On 2013/01/09 09:59:35, I haz the power (commit-bot) wrote:
> Retried try job too often on win_rel for step(s) interactive_ui_tests

The interactive_ui_tests case that keeps failing is
ExtensionApiTest.NotificationsHasPermissionManifest, which is timing out.  This
test times out on the bots (all bots) a fair amount of the time and takes a long
time every time, so I think I'm gonna claim innocence.

[jamesr, 2013-01-09 22:58:58]

https://codereview.chromium.org/11575049/diff/75004/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11575049/diff/75004/content/renderer/render_w...
content/renderer/render_widget.cc:1486: web_layer_tree_view_.reset();
aha, this is causing crashes in threaded mode when closing a render widget
that's not the last widget in the process.  The problem is this deletes the
WebLayerTreeView, but webwidget_->close() will try to dereference this pointer
when in threaded mode as a side effect of shutting down.  I can't just kill the
widget first since it is the WebLayerTreeViewClient, and the WLTV interface is
that the client has to outlive the WLTV (same as with other interfaces).

https://bugs.webkit.org/show_bug.cgi?id=106495 adds API that will be needed to
make it through this shutdown cleanly.

[commit-bot: I haz the power, 2013-01-10 23:44:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11575049/97001

[Leandro Graciá Gil, 2013-01-17 18:18:24]

On 2013/01/10 23:44:14, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/jamesr%40chromium.org/11575049/97001

Just wondering, is there a new issue for this or maybe this one should not be
marked as closed? Just trying to keep track of the status.