Fix display:none issue for <webview>.

Fix display:none issue for <webview>.

After we load a <webview>, setting its display to "none" removes the
plugin <object> from the render tree. If we set the display back to "",
we need to properly detect the case and allocate new instance id
and call attach to make things work.

The problem is briefly described here:
https://docs.google.com/a/chromium.org/document/d/1ITPOxS97DsG1HkbTAlpH0YcaTXKZNYUs84p1MppaHg0/view

BUG=399060
Test=Load a <webview> in a chrome app. See it render.
Set <webview>.style.display = 'none', see it go away.
Set <webview>.stlye.display = '', see it render properly.

Committed: https://crrev.com/cee3dc262eb5e002fc8be52cd23506172087c53d
Cr-Commit-Position: refs/heads/master@{#291990}

[lazyboy, 2014-08-22 22:49:27]

https://chromiumcodereview.appspot.com/440463002/diff/100001/chrome/renderer/...
File chrome/renderer/resources/extensions/web_view.js (right):

https://chromiumcodereview.appspot.com/440463002/diff/100001/chrome/renderer/...
chrome/renderer/resources/extensions/web_view.js:119:
shadowRoot.appendChild(this.browserPluginNode);
This reshuffle is needed, otherwise "internalinstanceid" mutation would appear
before
we have this.partition set up.

[Fady Samuel, 2014-08-23 11:06:16]

https://codereview.chromium.org/440463002/diff/120001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/440463002/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:743: this.reset();
What happens if you try to set the src attribute while the guest is in a
display:none state?

[lazyboy, 2014-08-23 16:10:18]

https://codereview.chromium.org/440463002/diff/120001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/440463002/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:743: this.reset();
On 2014/08/23 11:06:16, Fady Samuel wrote:
> What happens if you try to set the src attribute while the guest is in a
> display:none state?

The next setting of display = '' will navigate the webview.
There's a test for that WebViewTest.Shim_TestDisplayNoneWebviewLoad

[Fady Samuel, 2014-08-25 18:55:34]

lgtm with nits.

https://codereview.chromium.org/440463002/diff/160001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/440463002/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:578: this.parseAttributes();
Early exit preferred for readability:

if (!this.deferredAttachState) {
  this.parseAttributes();
  return;
}

https://codereview.chromium.org/440463002/diff/160001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view_events.js (right):

https://codereview.chromium.org/440463002/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view_events.js:218: };
I'm trying to switch away from self to bind(this). Ben Kalman prefers that style
and extension_options.js has been using that style and so I think we should
adopt that style for consistency.

  PluginDestroyedEvent.addListener(function(e) {
    this.webViewInternal.onPluginDestroyed();
  }.bind(this), {instanceId: this.viewInstanceId});

[lazyboy, 2014-08-25 19:16:44]

Address comments in patchest #10

https://chromiumcodereview.appspot.com/440463002/diff/160001/chrome/renderer/...
File chrome/renderer/resources/extensions/web_view.js (right):

https://chromiumcodereview.appspot.com/440463002/diff/160001/chrome/renderer/...
chrome/renderer/resources/extensions/web_view.js:578: this.parseAttributes();
On 2014/08/25 18:55:34, Fady Samuel wrote:
> Early exit preferred for readability:
> 
> if (!this.deferredAttachState) {
>   this.parseAttributes();
>   return;
> }

Done.

https://chromiumcodereview.appspot.com/440463002/diff/160001/chrome/renderer/...
File chrome/renderer/resources/extensions/web_view_events.js (right):

https://chromiumcodereview.appspot.com/440463002/diff/160001/chrome/renderer/...
chrome/renderer/resources/extensions/web_view_events.js:218: };
On 2014/08/25 18:55:34, Fady Samuel wrote:
> I'm trying to switch away from self to bind(this). Ben Kalman prefers that
style
> and extension_options.js has been using that style and so I think we should
> adopt that style for consistency.
> 
>   PluginDestroyedEvent.addListener(function(e) {
>     this.webViewInternal.onPluginDestroyed();
>   }.bind(this), {instanceId: this.viewInstanceId});

Done.

[lazyboy, 2014-08-25 22:58:32]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-08-25 23:02:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lazyboy@chromium.org/440463002/200001

[commit-bot: I haz the power, 2014-08-26 00:07:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-26 00:07:37]

Failed to commit the patch.

[Fady Samuel, 2014-08-26 15:48:11]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-08-26 15:49:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lazyboy@chromium.org/440463002/200001

[commit-bot: I haz the power, 2014-08-26 15:49:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-26 15:49:39]

Failed to apply patch for chrome/browser/apps/web_view_browsertest.cc:
While running git apply --index -p1;
  error: patch failed: chrome/browser/apps/web_view_browsertest.cc:116
  error: chrome/browser/apps/web_view_browsertest.cc: patch does not apply

Patch:       chrome/browser/apps/web_view_browsertest.cc
Index: chrome/browser/apps/web_view_browsertest.cc
diff --git a/chrome/browser/apps/web_view_browsertest.cc
b/chrome/browser/apps/web_view_browsertest.cc
index
1f2477e0e10b6991efa8689aa24e4c7c5c40978e..ad4d1e63cae091cf21708f6b0bd6d8005c340909
100644
--- a/chrome/browser/apps/web_view_browsertest.cc
+++ b/chrome/browser/apps/web_view_browsertest.cc
@@ -98,17 +98,26 @@ class TestGuestViewManager : public
extensions::GuestViewManager {
  public:
   explicit TestGuestViewManager(content::BrowserContext* context) :
       GuestViewManager(context),
+      seen_guest_removed_(false),
       web_contents_(NULL) {}
 
   content::WebContents* WaitForGuestCreated() {
     if (web_contents_)
       return web_contents_;
 
-    message_loop_runner_ = new content::MessageLoopRunner;
-    message_loop_runner_->Run();
+    created_message_loop_runner_ = new content::MessageLoopRunner;
+    created_message_loop_runner_->Run();
     return web_contents_;
   }
 
+  void WaitForGuestDeleted() {
+    if (seen_guest_removed_)
+      return;
+
+    deleted_message_loop_runner_ = new content::MessageLoopRunner;
+    deleted_message_loop_runner_->Run();
+  }
+
  private:
   // GuestViewManager override:
   virtual void AddGuest(int guest_instance_id,
@@ -116,13 +125,25 @@ class TestGuestViewManager : public
extensions::GuestViewManager {
     extensions::GuestViewManager::AddGuest(
         guest_instance_id, guest_web_contents);
     web_contents_ = guest_web_contents;
+    seen_guest_removed_ = false;
 
-    if (message_loop_runner_)
-      message_loop_runner_->Quit();
+    if (created_message_loop_runner_)
+      created_message_loop_runner_->Quit();
+  }
+
+  virtual void RemoveGuest(int guest_instance_id) OVERRIDE {
+    extensions::GuestViewManager::RemoveGuest(guest_instance_id);
+    web_contents_ = NULL;
+    seen_guest_removed_ = true;
+
+    if (deleted_message_loop_runner_)
+      deleted_message_loop_runner_->Quit();
   }
 
+  bool seen_guest_removed_;
   content::WebContents* web_contents_;
-  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+  scoped_refptr<content::MessageLoopRunner> created_message_loop_runner_;
+  scoped_refptr<content::MessageLoopRunner> deleted_message_loop_runner_;
 };
 
 // Test factory for creating test instances of GuestViewManager.
@@ -679,7 +700,6 @@ class WebViewTest : public
extensions::PlatformAppBrowserTest {
   }
 
   void LoadAppWithGuest(const std::string& app_path) {
-
     ExtensionTestMessageListener launched_listener("WebViewTest.LAUNCHED",
                                                    false);
     launched_listener.set_failure_message("WebViewTest.FAILURE");
@@ -842,6 +862,33 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, AutoSize) {
       << message_;
 }
 
+// Tests that a <webview> that is set to "display: none" after load and then
+// setting "display: block" re-renders the plugin properly.
+//
+// Initially after loading the <webview> and the test sets <webview> to
+// "display: none".
+// This causes the browser plugin to be destroyed, we then set the
+// style.display of the <webview> to block again and check that loadstop
+// fires properly.
+IN_PROC_BROWSER_TEST_F(WebViewTest, DisplayNoneAndBack) {
+  LoadAppWithGuest("web_view/display_none_and_back");
+
+  scoped_refptr<content::MessageLoopRunner> loop_runner(
+      new content::MessageLoopRunner);
+  WebContentsHiddenObserver observer(GetGuestWebContents(),
+                                     loop_runner->QuitClosure());
+
+  // Handled in platform_apps/web_view/display_none_and_back/main.js
+  SendMessageToEmbedder("hide-guest");
+  GetGuestViewManager()->WaitForGuestDeleted();
+  ExtensionTestMessageListener test_passed_listener("WebViewTest.PASSED",
+                                                    false);
+
+  SendMessageToEmbedder("show-guest");
+  GetGuestViewManager()->WaitForGuestCreated();
+  EXPECT_TRUE(test_passed_listener.WaitUntilSatisfied());
+}
+
 // http://crbug.com/326332
 IN_PROC_BROWSER_TEST_F(WebViewTest, DISABLED_Shim_TestAutosizeAfterNavigation)
{
   TestHelper("testAutosizeAfterNavigation", "web_view/shim", NO_TEST_SERVER);

[lazyboy, 2014-08-26 16:44:56]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-08-26 16:45:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lazyboy@chromium.org/440463002/220001

[commit-bot: I haz the power, 2014-08-26 18:10:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-26 19:05:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-26 19:05:40]

Failed to apply patch for
chrome/browser/guest_view/web_view/web_view_constants.cc:
While running git apply --index -p1;
  error: chrome/browser/guest_view/web_view/web_view_constants.cc: does not
exist in index

Patch:       chrome/browser/guest_view/web_view/web_view_constants.cc
Index: chrome/browser/guest_view/web_view/web_view_constants.cc
diff --git a/chrome/browser/guest_view/web_view/web_view_constants.cc
b/chrome/browser/guest_view/web_view/web_view_constants.cc
index
a93e363f4d83ad62be8af6fa2f4c41707f339fdb..28950ac1dd4f3b516f2e9599b735a8e90515ea16
100644
--- a/chrome/browser/guest_view/web_view/web_view_constants.cc
+++ b/chrome/browser/guest_view/web_view/web_view_constants.cc
@@ -34,6 +34,7 @@ const char kEventLoadStop[] = "webViewInternal.onLoadStop";
 const char kEventMessage[] = "webViewInternal.onMessage";
 const char kEventNewWindow[] = "webViewInternal.onNewWindow";
 const char kEventPermissionRequest[] = "webViewInternal.onPermissionRequest";
+const char kEventPluginDestroyed[] = "webViewInternal.onPluginDestroyed";
 const char kEventResponsive[] = "webViewInternal.onResponsive";
 const char kEventSizeChanged[] = "webViewInternal.onSizeChanged";
 const char kEventUnresponsive[] = "webViewInternal.onUnresponsive";

[lazyboy, 2014-08-26 19:36:45]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-08-26 19:38:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lazyboy@chromium.org/440463002/240001

[commit-bot: I haz the power, 2014-08-26 20:43:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-26 20:58:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-26 20:58:08]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[lazyboy, 2014-08-26 22:00:43]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-08-26 22:02:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lazyboy@chromium.org/440463002/240001

[commit-bot: I haz the power, 2014-08-26 22:06:59]

Committed patchset #13 (240001) as e9391fa563657cbec02a0a1a254bf5f44b119dc3

[commit-bot: I haz the power, 2014-09-10 02:45:54]

Patchset 13 (id:??) landed as
https://crrev.com/cee3dc262eb5e002fc8be52cd23506172087c53d
Cr-Commit-Position: refs/heads/master@{#291990}