Change the FF CEEE to create its chrome frame instance initially hidden, and

make it visible only once the content is done loading.  This avoids any flicker
at startup, and fixes a bug where the chrome frame instance is removed after
a print preview.

Also added support to chrome frame to send an uninitialized ready state
when its being torn down.  Did some cleanup in the chrome frame tests which
were broke the first time this change was committed.

BUG=62979
TEST=Make sure that there is no UI flicker on chrome frame startup, and make
sure that the extenions works correct after showing print preview and closing
it.

Review URL: http://codereview.chromium.org/5022003

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

7 files changed, 122 insertions, 127 deletions

diff --git a/ceee/firefox/content/cf.js b/ceee/firefox/content/cf.js
index 48c0328..e8dd4f79 100644
--- a/ceee/firefox/content/cf.js
+++ b/ceee/firefox/content/cf.js
@@ -168,10 +168,11 @@ CfHelper.prototype.create = function(parent, idValue, onCfReady, onCfMessage) {
                         '--enable-experimental-extension-apis');
   // TODO(joi@chromium.org) Remove this once the "*" default is removed from CF.
   this.cf_.setAttribute('chrome_functions_automated', '');
+  this.cf_.style.visibility = 'hidden';
 
   // Add ChromeFrame to the DOM.  This *must* be done before setting the
   // event listeners or things will just silently fail.
-  parent.insertBefore(this.cf_, parent.firstChild);
+  parent.appendChild(this.cf_);
 
   // Setup all event handlers.
   this.cf_.onprivatemessage = onCfMessage;
@@ -192,7 +193,9 @@ CfHelper.prototype.create = function(parent, idValue, onCfReady, onCfMessage) {
 
   var impl = this;
   this.cf_.addEventListener('readystatechanged',
-      function() {impl.onReadyStateChange_(onCfReady);}, false);
+      function() {impl.onReadyStateChange_();}, false);
+  this.cf_.addEventListener('load',
+      function() {impl.onCFContentLoad_(onCfReady);}, false);
 
   return this.cf_;
 };
@@ -230,13 +233,14 @@ var READY_STATE_COMPLETED = 4;
 
 /**
  * Handle 'embed' element ready state events.
- * @param {!function()} onCfReady A callback for ChromeFrame becomes ready.
  * @private
  */
-CfHelper.prototype.onReadyStateChange_ = function(onCfReady) {
+CfHelper.prototype.onReadyStateChange_ = function() {
   this.ceee_.logInfo('CfHelper.readystatechange: state=' +
       this.cf_.readystate);
-  if (this.cf_.readystate == READY_STATE_COMPLETED) {
+  if (this.cf_.readystate == READY_STATE_UNINITIALIZED) {
+    this.cf_.style.visibility = 'hidden';
+  } else if (this.cf_.readystate == READY_STATE_COMPLETED) {
     // Do this before we even load the extension at the other end so
     // that extension automation is set up before any background pages
     // in the extension load.
@@ -249,7 +253,6 @@ CfHelper.prototype.onReadyStateChange_ = function(onCfReady) {
     //if (!loadExtensions) {
     //  this.isReady_ = true;
     //  this.postPendingMessages_();
-    //  onCfReady();
     //  return;
     //}
     var lastInstallPath =
@@ -270,14 +273,14 @@ CfHelper.prototype.onReadyStateChange_ = function(onCfReady) {
       // Install and save the path and file time.
       this.cf_.installExtension(extensionPath, function(res) {
         impl.cf_.getEnabledExtensions(function(extensionDirs) {
-          impl.onGetEnabledExtensionsComplete_(extensionDirs, onCfReady);
+          impl.onGetEnabledExtensionsComplete_(extensionDirs);
         });
       });
     } else {
       // We are not installing a CRX file.  Before deciding what we should do,
       // we need to find out what extension may already be installed.
       this.cf_.getEnabledExtensions(function(extensionDirs) {
-        impl.onGetEnabledExtensionsComplete_(extensionDirs, onCfReady);
+        impl.onGetEnabledExtensionsComplete_(extensionDirs);
       });
     }
   }
@@ -287,11 +290,9 @@ CfHelper.prototype.onReadyStateChange_ = function(onCfReady) {
  * Process response from 'getEnabledExtensions'.
  * @param {string} extensionDirs tab-separate list of extension dirs. Only first
  *     entry is handled.
- * @param {function()} onCfReady A callback for ChromeFrame becomes ready.
  * @private
  */
-CfHelper.prototype.onGetEnabledExtensionsComplete_ = function (extensionDirs,
-                                                               onCfReady) {
+CfHelper.prototype.onGetEnabledExtensionsComplete_ = function(extensionDirs) {
   this.ceee_.logInfo('OnGetEnabledExtensions: ' + extensionDirs);
 
   var extensions = extensionDirs.split('\t');
@@ -306,24 +307,24 @@ CfHelper.prototype.onGetEnabledExtensionsComplete_ = function (extensionDirs,
           this.LAST_INSTALL_TIME,
           extensionFile.lastModifiedTime.toString());
     }
-    this.onLoadExtension_(extensions[0], onCfReady);
+    this.onLoadExtension_(extensions[0]);
   } else if (extensionFile && !this.ceee_.isCrx(extensionFile.path)) {
     this.ceee_.logInfo('Loading exploded path:' + extensionFile.path);
     this.cf_.loadExtension(extensionFile.path, function() {
-      impl.onLoadExtension_(extensionFile.path, onCfReady);
+      impl.onLoadExtension_(extensionFile.path);
     });
   } else if (!this.alreadyTriedInstalling_ && extensionFile) {
     this.ceee_.logInfo('Attempting to do first-time .crx install.');
     this.alreadyTriedInstalling_ = true;
     this.cf_.installExtension(extensionFile.path, function(res) {
       impl.cf_.getEnabledExtensions(function(extensionDirs) {
-        impl.onGetEnabledExtensionsComplete_(extensionDirs, onCfReady);
+        impl.onGetEnabledExtensionsComplete_(extensionDirs);
       });
     });
   } else {
     // Remove the toolbar entirely. The direct parent is the toolbaritem
     // (check overlay.xul for more information), so we to remove our parentNode.
-    var toolbar = this.parent_.parentNode.parentNode;
+    var toolbar = this.parent_.parentNode;
     toolbar.parentNode.removeChild(toolbar);
     this.ceee_.logInfo('No extension installed.');
   }
@@ -332,10 +333,9 @@ CfHelper.prototype.onGetEnabledExtensionsComplete_ = function (extensionDirs,
 /**
  * Process response from 'loadExtension' call.
  * @param {string} baseDir Extension home dir.
- * @param {!function()} onCfReady A callback for ChromeFrame becomes ready.
  * @private
  */
-CfHelper.prototype.onLoadExtension_ = function (baseDir, onCfReady) {
+CfHelper.prototype.onLoadExtension_ = function (baseDir) {
   var extensions = [baseDir];
   this.ceee_.logInfo('OnLoadExtension: ' + baseDir);
   this.ceee_.initExtensions(extensions);
@@ -349,19 +349,22 @@ CfHelper.prototype.onLoadExtension_ = function (baseDir, onCfReady) {
   } else {
     // Remove the toolbar entirely. The direct parent is the toolbaritem
     // (check overlay.xul for more information), so we to remove our parentNode.
-    var toolbar = this.parent_.parentNode.parentNode;
+    var toolbar = this.parent_.parentNode;
     toolbar.parentNode.removeChild(toolbar);
     this.ceee_.logInfo('No extension URL');
   }
+};
 
+/**
+ * Called once the content of the ChromeFrame is loaded and ready.
+ * @param {!function()} onCfReady A callback for ChromeFrame becomes ready.
+ * @private
+ */
+CfHelper.prototype.onCFContentLoad_ = function(onCfReady) {
+  this.cf_.style.visibility = 'visible';
   this.isReady_ = true;
   this.postPendingMessages_();
   onCfReady();
-  var parent = this.parent_;
-  setTimeout(function(){
-    parent.selectedIndex = 0;
-    parent.removeChild(parent.lastChild);
-  }, 0);
 };
 
 // Make the constructor visible outside this anonymous block.
diff --git a/ceee/firefox/content/overlay.js b/ceee/firefox/content/overlay.js
index 482f99d..23dbd8e 100644
--- a/ceee/firefox/content/overlay.js
+++ b/ceee/firefox/content/overlay.js
@@ -306,7 +306,7 @@ CEEE_Class.prototype.createChromeFrame_ = function() {
   var onCfMessage = function(evt, target) {impl.onCfMessage_(evt, target);};
 
   this.cfHelper_ = new CEEE_CfHelper(this);
-  var parent = document.getElementById('ceee-frame-deck');
+  var parent = document.getElementById('ceee-browser-item');
   var cf = this.cfHelper_.create(parent, this.cfHelper_.CHROME_FRAME_ID,
       onCfReady, onCfMessage);
 };
diff --git a/ceee/firefox/content/overlay.xul b/ceee/firefox/content/overlay.xul
index 883d5ba..c70ab62 100644
--- a/ceee/firefox/content/overlay.xul
+++ b/ceee/firefox/content/overlay.xul
@@ -47,12 +47,7 @@
            toolbaritem to appear correctly. -->
       <toolbaritem id="ceee-browser-item" title="&ceee.browser.label;"
           flex="1" tooltiptext="&ceee.browser.tooltip;">
-        <deck id="ceee-frame-deck" flex="1" selectedIndex="1">
-          <!-- The ChromeFrame will be placed as a child of this element
-               just before the following div element in the overlay.js file. -->
-          <div xmlns="http://www.w3.org/1999/xhtml"
-               style="background-color:white"/>
-        </deck>
+        <!-- The ChromeFrame will be placed as a child of this element. -->
       </toolbaritem>
     </toolbar>
   </toolbox>
@@ -69,15 +64,13 @@
       <label id="ceee-sidebar-label"
              value="My title"
              style="font-size:14pt;"/>
-      <!-- The ChromeFrame will be placed as a child of this element
-               just before the following div element in the overlay.js file. -->
+      <!-- The ChromeFrame will be placed as a child of this element. -->
     </vbox>
   </hbox>
 
   <window id="main-window">
     <vbox id="ceee-sidebar-icon-box">
-      <!-- The ChromeFrame will be placed as a child of this element
-               just before the following div element in the overlay.js file. -->
+      <!-- The ChromeFrame will be placed as a child of this element. -->
     </vbox>
   </window>
 
diff --git a/ceee/firefox/content/userscript_api.js b/ceee/firefox/content/userscript_api.js
index aefc66cc..87522d9 100644
--- a/ceee/firefox/content/userscript_api.js
+++ b/ceee/firefox/content/userscript_api.js
@@ -651,9 +651,6 @@ UserScriptManager.prototype.loadUserScripts = function(manifest) {
         pathPattern: new RegExp(pathPattern)
       };
       patterns.push(pattern);
-
-      ceee.logInfo('loadUserScripts: url=' + spec +
-                   ' pattern=' + CEEE_json.encode(pattern));
     }
 
     script.patterns = patterns;
diff --git a/chrome_frame/chrome_frame_npapi.cc b/chrome_frame/chrome_frame_npapi.cc
index ea7217f..f3144b1 100644
--- a/chrome_frame/chrome_frame_npapi.cc
+++ b/chrome_frame/chrome_frame_npapi.cc
@@ -290,9 +290,8 @@ bool ChromeFrameNPAPI::Initialize(NPMIMEType mime_type, NPP instance,
 }
 
 void ChromeFrameNPAPI::Uninitialize() {
-  // Don't call SetReadyState as it will end up calling FireEvent.
-  // We are in the context of NPP_DESTROY.
-  ready_state_ = READYSTATE_UNINITIALIZED;
+  if (ready_state_ != READYSTATE_UNINITIALIZED)
+    SetReadyState(READYSTATE_UNINITIALIZED);
 
   UnsubscribeFromFocusEvents();
 
@@ -851,7 +850,7 @@ void ChromeFrameNPAPI::OnMessageFromChromeFrame(int tab_handle,
     }
     DLOG_IF(WARNING, !invoke) << "InvokeDefault failed";
   } else {
-    NOTREACHED() << "CreateMessageEvent";
+    DLOG(WARNING) << "CreateMessageEvent failed, probably exiting";
   }
 }
 
@@ -1055,7 +1054,7 @@ void ChromeFrameNPAPI::DispatchEvent(NPObject* event) {
         npapi::GetStringIdentifier("dispatchEvent"), &param, 1, &result);
     DLOG_IF(WARNING, !invoke) << "dispatchEvent failed";
   } else {
-    NOTREACHED() << "NPNVPluginElementNPObject";
+    DLOG(WARNING) << "ChromeFrameNPAPI::DispatchEvent failed, probably exiting";
   }
 }
 
@@ -1440,7 +1439,7 @@ NpProxyService* ChromeFrameNPAPI::CreatePrefService() {
 }
 
 NPObject* ChromeFrameNPAPI::GetWindowObject() const {
-  if (!window_object_.get()) {
+  if (!window_object_.get() && instance_) {
     NPError ret = npapi::GetValue(instance_, NPNVWindowNPObject,
         window_object_.Receive());
     DLOG_IF(ERROR, ret != NPERR_NO_ERROR) << "NPNVWindowNPObject failed";
diff --git a/chrome_frame/chrome_frame_npapi_unittest.cc b/chrome_frame/chrome_frame_npapi_unittest.cc
index 34194440..f209a86 100644
--- a/chrome_frame/chrome_frame_npapi_unittest.cc
+++ b/chrome_frame/chrome_frame_npapi_unittest.cc
@@ -68,9 +68,6 @@ class MockNPAPI: public ChromeFrameNPAPI {
     ChromeFrameNPAPI::OnAutomationServerReady();
   }
 
-  // Neuter this (or it dchecks during testing).
-  void SetReadyState(READYSTATE new_state) {}
-
   ChromeFrameAutomationClient* CreateAutomationClient() {
     return mock_automation_client_;
   }
@@ -100,13 +97,88 @@ MATCHER_P4(LaunchParamEq, version_check, extra, incognito, widget,
          arg->incognito() == incognito,
          arg->widget_mode() == widget;
 }
+
+
+static const NPIdentifier kOnPrivateMessageId =
+    reinterpret_cast<NPIdentifier>(0x100);
+static const NPIdentifier kPostPrivateMessageId =
+    reinterpret_cast<NPIdentifier>(0x100);
 }
 
+class MockNetscapeFuncs {
+ public:
+  MockNetscapeFuncs() {
+    CHECK(NULL == current_);
+    current_ = this;
+  }
+
+  ~MockNetscapeFuncs() {
+    CHECK(this == current_);
+    current_ = NULL;
+  }
+
+  MOCK_METHOD3(GetValue, NPError(NPP, NPNVariable, void *));
+  MOCK_METHOD3(GetStringIdentifiers, void(const NPUTF8 **,
+                                          int32_t,
+                                          NPIdentifier *));  // NOLINT
+  MOCK_METHOD1(RetainObject, NPObject*(NPObject*));  // NOLINT
+  MOCK_METHOD1(ReleaseObject, void(NPObject*));  // NOLINT
+
+
+  void GetPrivilegedStringIdentifiers(const NPUTF8 **names,
+                                      int32_t name_count,
+                                      NPIdentifier *identifiers) {
+    for (int32_t i = 0; i < name_count; ++i) {
+      if (0 == strcmp(names[i], "onprivatemessage")) {
+        identifiers[i] = kOnPrivateMessageId;
+      } else if (0 == strcmp(names[i], "postPrivateMessage")) {
+        identifiers[i] = kPostPrivateMessageId;
+      } else {
+        identifiers[i] = 0;
+      }
+    }
+  }
+
+  static const NPNetscapeFuncs* netscape_funcs() {
+    return &netscape_funcs_;
+  }
+
+ private:
+  static NPError MockGetValue(NPP instance,
+                              NPNVariable variable,
+                              void *ret_value) {
+    DCHECK(current_);
+    return current_->GetValue(instance, variable, ret_value);
+  }
+
+  static void MockGetStringIdentifiers(const NPUTF8 **names,
+                                       int32_t name_count,
+                                       NPIdentifier *identifiers) {
+    DCHECK(current_);
+    return current_->GetStringIdentifiers(names, name_count, identifiers);
+  }
+
+  static NPObject* MockRetainObject(NPObject* obj) {
+    DCHECK(current_);
+    return current_->RetainObject(obj);
+  }
+
+  static void MockReleaseObject(NPObject* obj) {
+    DCHECK(current_);
+    current_->ReleaseObject(obj);
+  }
+
+  static MockNetscapeFuncs* current_;
+  static NPNetscapeFuncs netscape_funcs_;
+};
+
 // Test fixture to allow testing the privileged NPAPI APIs
 class TestNPAPIPrivilegedApi: public ::testing::Test {
  public:
   virtual void SetUp() {
     memset(&instance, 0, sizeof(instance));
+    npapi::InitializeBrowserFunctions(
+        const_cast<NPNetscapeFuncs*>(mock_funcs.netscape_funcs()));
 
     // Gets owned & destroyed by mock_api (in the
     // ChromeFramePlugin<T>::Uninitialize() function).
@@ -119,6 +191,11 @@ class TestNPAPIPrivilegedApi: public ::testing::Test {
   }
 
   virtual void TearDown() {
+    // Make sure to uninitialize the mock NPAPI before we uninitialize the
+    // browser function, otherwise we will get a DCHECK in the NPAPI dtor
+    // when it tries to perform the uninitialize there.
+    mock_api.Uninitialize();
+    npapi::UninitializeBrowserFunctions();
   }
 
   void SetupPrivilegeTest(bool is_incognito,
@@ -154,6 +231,7 @@ class TestNPAPIPrivilegedApi: public ::testing::Test {
   }
 
  public:
+  MockNetscapeFuncs mock_funcs;
   MockNPAPI mock_api;
   MockAutomationClient* mock_automation;
   MockProxyService* mock_proxy;
@@ -264,79 +342,6 @@ TEST_F(TestNPAPIPrivilegedApi, PrivilegedAllowsArgsAndProfile) {
 
 namespace {
 
-static const NPIdentifier kOnPrivateMessageId =
-    reinterpret_cast<NPIdentifier>(0x100);
-static const NPIdentifier kPostPrivateMessageId =
-    reinterpret_cast<NPIdentifier>(0x100);
-
-
-class MockNetscapeFuncs {
- public:
-  MockNetscapeFuncs() {
-    CHECK(NULL == current_);
-    current_ = this;
-  }
-
-  ~MockNetscapeFuncs() {
-    CHECK(this == current_);
-    current_ = NULL;
-  }
-
-  MOCK_METHOD3(GetValue, NPError(NPP, NPNVariable, void *));
-  MOCK_METHOD3(GetStringIdentifiers, void(const NPUTF8 **,
-                                          int32_t,
-                                          NPIdentifier *));  // NOLINT
-  MOCK_METHOD1(RetainObject, NPObject*(NPObject*));  // NOLINT
-  MOCK_METHOD1(ReleaseObject, void(NPObject*));  // NOLINT
-
-
-  void GetPrivilegedStringIdentifiers(const NPUTF8 **names,
-                                      int32_t name_count,
-                                      NPIdentifier *identifiers) {
-    for (int32_t i = 0; i < name_count; ++i) {
-      if (0 == strcmp(names[i], "onprivatemessage")) {
-        identifiers[i] = kOnPrivateMessageId;
-      } else if (0 == strcmp(names[i], "postPrivateMessage")) {
-        identifiers[i] = kPostPrivateMessageId;
-      } else {
-        identifiers[i] = 0;
-      }
-    }
-  }
-
-  static const NPNetscapeFuncs* netscape_funcs() {
-    return &netscape_funcs_;
-  }
-
- private:
-  static NPError MockGetValue(NPP instance,
-                              NPNVariable variable,
-                              void *ret_value) {
-    DCHECK(current_);
-    return current_->GetValue(instance, variable, ret_value);
-  }
-
-  static void MockGetStringIdentifiers(const NPUTF8 **names,
-                                       int32_t name_count,
-                                       NPIdentifier *identifiers) {
-    DCHECK(current_);
-    return current_->GetStringIdentifiers(names, name_count, identifiers);
-  }
-
-  static NPObject* MockRetainObject(NPObject* obj) {
-    DCHECK(current_);
-    return current_->RetainObject(obj);
-  }
-
-  static void MockReleaseObject(NPObject* obj) {
-    DCHECK(current_);
-    current_->ReleaseObject(obj);
-  }
-
-  static MockNetscapeFuncs* current_;
-  static NPNetscapeFuncs netscape_funcs_;
-};
-
 MockNetscapeFuncs* MockNetscapeFuncs::current_ = NULL;
 NPNetscapeFuncs MockNetscapeFuncs::netscape_funcs_ = {
   0,   // size
@@ -394,8 +399,6 @@ class TestNPAPIPrivilegedProperty: public TestNPAPIPrivilegedApi {
  public:
   virtual void SetUp() {
     TestNPAPIPrivilegedApi::SetUp();
-    npapi::InitializeBrowserFunctions(
-        const_cast<NPNetscapeFuncs*>(mock_funcs.netscape_funcs()));
 
     // Expect calls to release and retain objects.
     EXPECT_CALL(mock_funcs, RetainObject(kMockNPObject))
@@ -417,12 +420,8 @@ class TestNPAPIPrivilegedProperty: public TestNPAPIPrivilegedApi {
   }
 
   virtual void TearDown() {
-    npapi::UninitializeBrowserFunctions();
     TestNPAPIPrivilegedApi::TearDown();
   }
-
- public:
-  MockNetscapeFuncs mock_funcs;
 };
 
 
diff --git a/chrome_frame/np_event_listener.cc b/chrome_frame/np_event_listener.cc
index 937ca1d..cfbaded 100644
--- a/chrome_frame/np_event_listener.cc
+++ b/chrome_frame/np_event_listener.cc
@@ -172,9 +172,13 @@ NPObject* NPObjectEventListener::GetObjectElement(NPP instance) {
         // We've got the <embed> element but we really want
         // the <object> element.
         if (npapi::GetProperty(instance, object, ids[PARENT_ELEMENT], &var)) {
-          DCHECK(NPVARIANT_IS_OBJECT(var));
           npapi::ReleaseObject(object);
-          object = NPVARIANT_TO_OBJECT(var);
+          object = NULL;
+          if (NPVARIANT_IS_OBJECT(var)) {
+            object = NPVARIANT_TO_OBJECT(var);
+          } else {
+            DVLOG(1) << __FUNCTION__ << " Could not find our parent";
+          }
         }
       } else {
         DVLOG(1) << __FUNCTION__ << " got " << tag;