Reland attempt 2: Improve <webview> autosize:

a. Expand/shrink <webview> element when 'sizechange' event fires, to
match with the new view size (in shim).
b. For SW mode, fix a bug where damage buffer would remain smaller than
the view size and would result in crash. Added test for this case.

BUG=173238, 282116
Test=WebViewTest.AutoSize.*, <webview>.autosize=true now should autosize
webview container within the constraints
(minwidth/maxwidth/minheight/maxheight).

Review URL: https://chromiumcodereview.appspot.com/23691039

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@221402 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/apps/web_view_browsertest.cc

@@ -20,6 +20,7 @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents_delegate.h"
+#include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/fake_speech_recognition_manager.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -258,6 +259,17 @@ class WebViewTest : public extensions::PlatformAppBrowserTest {
     }
   }
 
+  virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
+    const testing::TestInfo* const test_info =
+        testing::UnitTest::GetInstance()->current_test_info();
+
+    // Force SW rendering to check autosize bug.
+    if (!strncmp(test_info->name(), "AutoSizeSW", strlen("AutosizeSW")))
+      command_line->AppendSwitch(switches::kDisableForceCompositingMode);
+
+    extensions::PlatformAppBrowserTest::SetUpCommandLine(command_line);
+  }
+
   // This method is responsible for initializing a packaged app, which contains
   // multiple webview tags. The tags have different partition identifiers and
   // their WebContent objects are returned as output. The method also verifies
@@ -567,6 +579,20 @@ IN_PROC_BROWSER_TEST_F(WebViewTest,
       << message_;
 }
 
+IN_PROC_BROWSER_TEST_F(WebViewTest, AutoSize) {
+  ASSERT_TRUE(RunPlatformAppTest("platform_apps/web_view/autosize"))
+      << message_;
+}
+
+#if !defined(OS_CHROMEOS)
+// This test ensures <webview> doesn't crash in SW rendering when autosize is
+// turned on.
+IN_PROC_BROWSER_TEST_F(WebViewTest, AutoSizeSW) {
+  ASSERT_TRUE(RunPlatformAppTest("platform_apps/web_view/autosize"))
+      << message_;
+}
+#endif
+
 IN_PROC_BROWSER_TEST_F(WebViewTest, Shim_TestAutosizeAfterNavigation) {
   TestHelper("testAutosizeAfterNavigation",
              "DoneShimTest.PASSED",
@@ -587,9 +613,8 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, Shim_TestAutosizeRemoveAttributes) {
              "web_view/shim");
 }
 
-// This test is flaky. crbug.com/282116
 IN_PROC_BROWSER_TEST_F(WebViewTest,
-                       DISABLED_Shim_TestAutosizeWithPartialAttributes) {
+                       Shim_TestAutosizeWithPartialAttributes) {
   TestHelper("testAutosizeWithPartialAttributes",
              "DoneShimTest.PASSED",
              "DoneShimTest.FAILED",

chrome/renderer/resources/extensions/web_view.js

@@ -24,9 +24,21 @@ var WebView = require('binding').Binding.create('webview').generate();
 // API can access it and not external developers.
 var secret = {};
 
+var WEB_VIEW_ATTRIBUTE_MAXHEIGHT = 'maxheight';
+var WEB_VIEW_ATTRIBUTE_MAXWIDTH = 'maxwidth';
+var WEB_VIEW_ATTRIBUTE_MINHEIGHT = 'minheight';
+var WEB_VIEW_ATTRIBUTE_MINWIDTH = 'minwidth';
+
 /** @type {Array.<string>} */
-var WEB_VIEW_ATTRIBUTES = ['name', 'partition', 'autosize', 'minheight',
-    'minwidth', 'maxheight', 'maxwidth'];
+var WEB_VIEW_ATTRIBUTES = [
+    'name',
+    'partition',
+    'autosize',
+    WEB_VIEW_ATTRIBUTE_MINHEIGHT,
+    WEB_VIEW_ATTRIBUTE_MINWIDTH,
+    WEB_VIEW_ATTRIBUTE_MAXHEIGHT,
+    WEB_VIEW_ATTRIBUTE_MAXWIDTH
+];
 
 var webViewInstanceIdCounter = 0;
 
@@ -113,6 +125,9 @@ var WEB_VIEW_EVENTS = {
   },
   'sizechanged': {
     evt: CreateEvent('webview.onSizeChanged'),
+    customHandler: function(webViewInternal, event, webViewEvent) {
+      webViewInternal.handleSizeChangedEvent_(event, webViewEvent);
+    },
     fields: ['oldHeight', 'oldWidth', 'newHeight', 'newWidth']
   },
   'unresponsive': {
@@ -475,6 +490,62 @@ WebViewInternal.prototype.getEvents_ = function() {
   return WEB_VIEW_EVENTS;
 };
 
+WebViewInternal.prototype.handleSizeChangedEvent_ =
+    function(event, webViewEvent) {
+  var node = this.webviewNode_;
+
+  var width = node.offsetWidth;
+  var height = node.offsetHeight;
+
+  // Check the current bounds to make sure we do not resize <webview>
+  // outside of current constraints.
+  var maxWidth;
+  if (node.hasAttribute(WEB_VIEW_ATTRIBUTE_MAXWIDTH) &&
+      node[WEB_VIEW_ATTRIBUTE_MAXWIDTH]) {
+    maxWidth = node[WEB_VIEW_ATTRIBUTE_MAXWIDTH];
+  } else {
+    maxWidth = width;
+  }
+
+  var minWidth;
+  if (node.hasAttribute(WEB_VIEW_ATTRIBUTE_MINWIDTH) &&
+      node[WEB_VIEW_ATTRIBUTE_MINWIDTH]) {
+    minWidth = node[WEB_VIEW_ATTRIBUTE_MINWIDTH];
+  } else {
+    minWidth = width;
+  }
+  if (minWidth > maxWidth) {
+    minWidth = maxWidth;
+  }
+
+  var maxHeight;
+  if (node.hasAttribute(WEB_VIEW_ATTRIBUTE_MAXHEIGHT) &&
+      node[WEB_VIEW_ATTRIBUTE_MAXHEIGHT]) {
+    maxHeight = node[WEB_VIEW_ATTRIBUTE_MAXHEIGHT];
+  } else {
+    maxHeight = height;
+  }
+  var minHeight;
+  if (node.hasAttribute(WEB_VIEW_ATTRIBUTE_MINHEIGHT) &&
+      node[WEB_VIEW_ATTRIBUTE_MINHEIGHT]) {
+    minHeight = node[WEB_VIEW_ATTRIBUTE_MINHEIGHT];
+  } else {
+    minHeight = height;
+  }
+  if (minHeight > maxHeight) {
+    minHeight = maxHeight;
+  }
+
+  if (webViewEvent.newWidth >= minWidth &&
+      webViewEvent.newWidth <= maxWidth &&
+      webViewEvent.newHeight >= minHeight &&
+      webViewEvent.newHeight <= maxHeight) {
+    node.style.width = webViewEvent.newWidth + 'px';
+    node.style.height = webViewEvent.newHeight + 'px';
+  }
+  node.dispatchEvent(webViewEvent);
+};
+
 /**
  * @private
  */

chrome/test/data/extensions/platform_apps/web_view/autosize/main.html

+<!--
+ * Copyright 2013 The Chromium Authors. All rights reserved.  Use of this
+ * source code is governed by a BSD-style license that can be found in the
+ * LICENSE file.
+-->
+<html>
+<body>
+  <webview style="width: 300px; height: 200px"></webview>
+  <script src="main.js"></script>
+</body>
+</html>

chrome/test/data/extensions/platform_apps/web_view/autosize/main.js

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+function testAutoSize() {
+  var webview = document.querySelector('webview');
+
+  var minw = 1600;
+  var maxw = 3000;
+  var minh = 1600;
+  var maxh = 3000;
+
+  webview.addEventListener('loadstop', function(e) {
+    window.console.log('guest.loadstop');
+    // Note that we set the autosize attribute first to check incorrect
+    // damage buffer allocation.
+    //
+    // Consider a case where:
+    // Initially, <webview>.width = 300 (default width).
+    // Then we set
+    // <webview>.autosize = true
+    // And then we set
+    // <webview>.minwidth = 200
+    // and
+    // <webview>.maxwidth = 800
+    // If the autosize logic decided to set the width to 400 (something > 300),
+    // then we won't have enough memory in damage buffer to render.
+    // When tried manually, this would cause garbage to be rendered (around the
+    // bottom portion of plugin). In test we set the value to high so increase
+    // the chance of invalid memory access.
+    webview.autosize = true;
+
+    // Ask for a larger size so we can expose memory access bug.
+    webview.maxwidth = maxw;
+    webview.maxheight = maxh;
+    webview.minwidth = minw;
+    webview.minheight = minh;
+  });
+
+  webview.addEventListener('sizechanged', function(e) {
+    window.console.log('sizechagned: dimension: ' + e.newWidth + ' X ' +
+                       e.newHeight);
+    chrome.test.assertTrue(e.newWidth >= minw);
+    chrome.test.assertTrue(e.newWidth <= maxw);
+    chrome.test.assertTrue(e.newHeight >= minh);
+    chrome.test.assertTrue(e.newHeight <= maxh);
+
+    chrome.test.succeed();
+  });
+
+  var longString = 'a b c d e f g h i j k l m o p q r s t u v w x y z';
+  for (var i = 0; i < 4; ++i) {
+    longString += longString;
+  }
+  webview.setAttribute('src',
+      'data:text/html,<body bgcolor="red">' +
+      '<div style="width: 400px; height: 200px;">' + longString + '</div>' +
+      longString + '</body>');
+}
+
+onload = function() {
+  chrome.test.runTests([testAutoSize]);
+};

chrome/test/data/extensions/platform_apps/web_view/autosize/manifest.json

+{
+  "name": "Platform App Test: <webview>",
+  "description": "Checks autosize on <webview>",
+  "version": "1",
+  "permissions": [
+    "webview"
+  ],
+  "app": {
+    "background": {
+      "scripts": ["test.js"]
+    }
+  }
+}

chrome/test/data/extensions/platform_apps/web_view/autosize/test.js

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+chrome.app.runtime.onLaunched.addListener(function() {
+  chrome.app.window.create('main.html', {}, function () {});
+});

chrome/test/data/extensions/platform_apps/web_view/shim/main.js

@@ -170,36 +170,30 @@ function testAutosizeWithPartialAttributes() {
 
   var step = 1;
   var sizeChangeHandler = function(e) {
+    window.console.log('sizeChangeHandler, new: ' +
+                       e.newWidth + ' X ' + e.newHeight);
     switch (step) {
       case 1:
         // Expect 300x200.
         embedder.test.assertEq(300, e.newWidth);
         embedder.test.assertEq(200, e.newHeight);
 
-        // Remove an autosize attribute and verify that it causes relayout.
-        webview.minwidth = null;
+        // Change the min size to cause a relayout.
+        webview.minwidth = 500;
         break;
       case 2:
-        // Expect 640x?
-        embedder.test.assertEq(640, e.newWidth);
+        embedder.test.assertTrue(e.newWidth >= webview.minwidth);
+        embedder.test.assertTrue(e.newWidth <= webview.maxwidth);
 
         // Tests when minwidth > maxwidth, minwidth = maxwidth.
-        webview.maxheight = null;
+        // i.e. minwidth is essentially 700.
         webview.minwidth = 800;
-        webview.minheight = 800;
         break;
       case 3:
-        // Expect 700X480
+        // Expect 700X?
         embedder.test.assertEq(700, e.newWidth);
-        embedder.test.assertEq(480, e.newHeight);
-
-        // Remove maxwidth and make sure the size returns to plugin size.
-        webview.removeAttribute('maxwidth');
-        break;
-      case 4:
-        // Expect original size, 640x480.
-        embedder.test.assertEq(640, e.newWidth);
-        embedder.test.assertEq(480, e.newHeight);
+        embedder.test.assertTrue(e.newHeight >= 200);
+        embedder.test.assertTrue(e.newHeight <= 600);
 
         embedder.test.succeed();
         break;
@@ -244,18 +238,16 @@ function testAutosizeRemoveAttributes() {
         webview.removeAttribute('maxwidth');
         webview.removeAttribute('minheight');
         webview.removeAttribute('maxheight');
+        webview.removeAttribute('autosize');
+
+        // We'd get one more sizechanged event after we turn off
+        // autosize.
+        webview.style.width = '500px';
+        webview.style.height = '500px';
         break;
       case 2:
-        // Expect plugin to return to its original size.
-        embedder.test.assertEq(640, e.newWidth);
-        embedder.test.assertEq(480, e.newHeight);
-
         embedder.test.succeed();
         break;
-      default:
-        window.console.log('Unexpected sizechanged event, step = ' + step);
-        embedder.test.fail();
-        break;
     }
 
     ++step;

content/browser/browser_plugin/browser_plugin_guest.cc

@@ -1399,6 +1399,7 @@ void BrowserPluginGuest::OnSetSize(
   if (auto_size_enabled_ && (!old_auto_size_enabled ||
                              (old_max_size != max_auto_size_) ||
                              (old_min_size != min_auto_size_))) {
+    RecordAction(UserMetricsAction("BrowserPlugin.Guest.EnableAutoResize"));
     GetWebContents()->GetRenderViewHost()->EnableAutoResize(
         min_auto_size_, max_auto_size_);
     // TODO(fsamuel): If we're changing autosize parameters, then we force

content/renderer/browser_plugin/browser_plugin.h

@@ -252,20 +252,22 @@ class CONTENT_EXPORT BrowserPlugin :
   // allocates a new |pending_damage_buffer_| if in software rendering mode.
   void PopulateResizeGuestParameters(
       BrowserPluginHostMsg_ResizeGuest_Params* params,
-      const gfx::Rect& view_size);
+      const gfx::Rect& view_size,
+      bool needs_repaint);
 
   // Populates BrowserPluginHostMsg_AutoSize_Params object with autosize state.
   void PopulateAutoSizeParameters(
-      BrowserPluginHostMsg_AutoSize_Params* params, bool current_auto_size);
+      BrowserPluginHostMsg_AutoSize_Params* params, bool auto_size_enabled);
 
   // Populates both AutoSize and ResizeGuest parameters based on the current
   // autosize state.
   void GetDamageBufferWithSizeParams(
       BrowserPluginHostMsg_AutoSize_Params* auto_size_params,
-      BrowserPluginHostMsg_ResizeGuest_Params* resize_guest_params);
+      BrowserPluginHostMsg_ResizeGuest_Params* resize_guest_params,
+      bool needs_repaint);
 
   // Informs the guest of an updated autosize state.
-  void UpdateGuestAutoSizeState(bool current_auto_size);
+  void UpdateGuestAutoSizeState(bool auto_size_enabled);
 
   // Indicates whether a damage buffer was used by the guest process for the
   // provided |params|.
@@ -312,14 +314,16 @@ class CONTENT_EXPORT BrowserPlugin :
   scoped_ptr<base::SharedMemory> current_damage_buffer_;
   scoped_ptr<base::SharedMemory> pending_damage_buffer_;
   uint32 damage_buffer_sequence_id_;
-  bool resize_ack_received_;
+  bool paint_ack_received_;
   gfx::Rect plugin_rect_;
   float last_device_scale_factor_;
   // Bitmap for crashed plugin. Lazily initialized, non-owning pointer.
   SkBitmap* sad_guest_;
   bool guest_crashed_;
   scoped_ptr<BrowserPluginHostMsg_ResizeGuest_Params> pending_resize_params_;
-  bool auto_size_ack_pending_;
+  bool is_auto_size_state_dirty_;
+  // Maximum size constraint for autosize.
+  gfx::Size max_auto_size_;
   std::string storage_partition_id_;
   bool persist_storage_;
   bool valid_partition_id_;