Several minor visual fixes to toolstrips:

* Layout toolstrips on the left of the bookmark bar so that
  something always obviously happens when you install one.
  This is in response to some feedback I've gotten demoing
  toolstrips for various people.

* Open the NTP on install if the bookmarkbar isn't visible.
  Again, I'm trying to make something happen when you
  install an extension. This won't be the permanent behavior
  but it seems like doing nothing if you have your bookmark
  bar detached is likely to confuse people.

* Fix a bug where we kept trying to resize the toolstrip
  back to the width it had when it was first made visible.

* Fix a bug where we didn't always repaint the bookmarkbar
  after the toolstrip preferred width changed.

This also seems to fix the issue where the buildbot sample
shows up overlayed on top of tab contents, though I didn't
mean to fix that with this CL.

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

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

6 files changed, 58 insertions, 47 deletions

diff --git a/chrome/browser/extensions/extension_view.cc b/chrome/browser/extensions/extension_view.cc
index 5678e08..3f602d0 100755
--- a/chrome/browser/extensions/extension_view.cc
+++ b/chrome/browser/extensions/extension_view.cc
@@ -50,7 +50,7 @@ void ExtensionView::DidChangeBounds(const gfx::Rect& previous,
 void ExtensionView::ShowIfCompletelyLoaded() {
   // We wait to show the ExtensionView until it has loaded and our parent has
   // given us a background. These can happen in different orders.
-  if (host_->did_stop_loading() && render_view_host()->view() &&
+  if (!IsVisible() && host_->did_stop_loading() && render_view_host()->view() &&
       !render_view_host()->view()->background().empty()) {
     SetVisible(true);
     DidContentsPreferredWidthChange(pending_preferred_width_);
@@ -71,7 +71,7 @@ void ExtensionView::DidContentsPreferredWidthChange(const int pref_width) {
   // Size changes will not be honored by lower layers while we are hidden.
   if (!IsVisible()) {
     pending_preferred_width_ = pref_width;
-  } else if (pref_width > 0) {
+  } else if (pref_width > 0 && pref_width != GetPreferredSize().width()) {
     set_preferred_size(gfx::Size(pref_width, height()));
     SizeToPreferredSize();
 
@@ -81,9 +81,8 @@ void ExtensionView::DidContentsPreferredWidthChange(const int pref_width) {
     // containment hierarchy.
     if (GetParent() != NULL && GetParent()->GetParent() != NULL) {
       GetParent()->GetParent()->Layout();
+      GetParent()->GetParent()->SchedulePaint();
     }
-
-    SchedulePaint();
   }
 }
 
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index 38fb703..5e9c221 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -13,6 +13,8 @@
 #include "base/thread.h"
 #include "base/values.h"
 #include "net/base/file_stream.h"
+#include "chrome/browser/browser.h"
+#include "chrome/browser/browser_list.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browsing_instance.h"
 #include "chrome/browser/extensions/extension.h"
@@ -27,8 +29,7 @@
 #include "chrome/common/json_value_serializer.h"
 #include "chrome/common/notification_service.h"
 #include "chrome/common/unzip.h"
-
-#include "chrome/browser/browser_list.h"
+#include "chrome/common/url_constants.h"
 
 #if defined(OS_WIN)
 #include "base/registry.h"
@@ -190,13 +191,22 @@ void ExtensionsService::OnExtensionsLoaded(ExtensionList* new_extensions) {
   delete new_extensions;
 }
 
-void ExtensionsService::OnExtensionInstalled(FilePath path, bool update) {
+void ExtensionsService::OnExtensionInstalled(Extension* extension,
+                                             bool update) {
   NotificationService::current()->Notify(
       NotificationType::EXTENSION_INSTALLED,
       NotificationService::AllSources(),
-      Details<FilePath>(&path));
-
-  // TODO(erikkay): Update UI if appropriate.
+      Details<Extension>(extension));
+
+  // We open the NTP if the extension has a toolstrip and the bookmark bar is
+  // detached. We noticed that people got confused if something didn't obviously
+  // happen when installing an extension.
+  Browser* browser = BrowserList::GetLastActive();
+  if (browser && browser->window() &&
+      !browser->window()->IsBookmarkBarVisible() &&
+      !extension->toolstrips().empty())
+    browser->AddTabWithURL(GURL(chrome::kChromeUINewTabURL), GURL(),
+                           PageTransition::LINK, true, -1, NULL);
 }
 
 ExtensionView* ExtensionsService::CreateView(Extension* extension,
@@ -755,24 +765,23 @@ void ExtensionsServiceBackend::ReportExtensionInstallError(
 
 void ExtensionsServiceBackend::ReportExtensionInstalled(
     const FilePath& path, bool update) {
+  // After it's installed, load it right away with the same settings.
+  Extension* extension = LoadExtensionCurrentVersion(path);
+  CHECK(extension);
+
   frontend_->GetMessageLoop()->PostTask(FROM_HERE, NewRunnableMethod(
       frontend_,
       &ExtensionsServiceFrontendInterface::OnExtensionInstalled,
-      path,
+      extension,
       update));
 
-  // After it's installed, load it right away with the same settings.
-  LOG(INFO) << "Loading extension " << path.value();
-  Extension* extension = LoadExtensionCurrentVersion(path);
-  if (extension) {
-    // Only one extension, but ReportExtensionsLoaded can handle multiple,
-    // so we need to construct a list.
-    scoped_ptr<ExtensionList> extensions(new ExtensionList);
-    extensions->push_back(extension);
-    LOG(INFO) << "Done.";
-    // Hand off ownership of the loaded extensions to the frontend.
-    ReportExtensionsLoaded(extensions.release());
-  }
+  // Only one extension, but ReportExtensionsLoaded can handle multiple,
+  // so we need to construct a list.
+  scoped_ptr<ExtensionList> extensions(new ExtensionList);
+  extensions->push_back(extension);
+  LOG(INFO) << "Done.";
+  // Hand off ownership of the loaded extensions to the frontend.
+  ReportExtensionsLoaded(extensions.release());
 }
 
 // Some extensions will autoupdate themselves externally from Chrome.  These
diff --git a/chrome/browser/extensions/extensions_service.h b/chrome/browser/extensions/extensions_service.h
index d876f34..4bf2323 100644
--- a/chrome/browser/extensions/extensions_service.h
+++ b/chrome/browser/extensions/extensions_service.h
@@ -53,7 +53,7 @@ class ExtensionsServiceFrontendInterface
   // Called with results from InstallExtension().
   // |is_update| is true if the installation was an update to an existing
   // installed extension rather than a new installation.
-  virtual void OnExtensionInstalled(FilePath path, bool is_update) = 0;
+  virtual void OnExtensionInstalled(Extension* extension, bool is_update) = 0;
 };
 
 
@@ -76,7 +76,7 @@ class ExtensionsService : public ExtensionsServiceFrontendInterface {
   virtual void InstallExtension(const FilePath& extension_path);
   virtual void LoadExtension(const FilePath& extension_path);
   virtual void OnExtensionsLoaded(ExtensionList* extensions);
-  virtual void OnExtensionInstalled(FilePath path, bool is_update);
+  virtual void OnExtensionInstalled(Extension* extension, bool is_update);
 
   // Creates a new ExtensionView, grouping it in the appropriate SiteInstance
   // (and therefore process) based on the URL and profile.
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index b4a81c5..d66ea0b 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -68,8 +68,8 @@ class ExtensionsServiceTestFrontend
     return &extensions_;
   }
 
-  std::vector<FilePath>* installed() {
-    return &installed_;
+  Extension* installed() {
+    return installed_;
   }
 
   // ExtensionsServiceFrontendInterface
@@ -92,8 +92,8 @@ class ExtensionsServiceTestFrontend
     std::stable_sort(extensions_.begin(), extensions_.end(), ExtensionsOrder());
   }
 
-  virtual void OnExtensionInstalled(FilePath path, bool is_update) {
-    installed_.push_back(path);
+  virtual void OnExtensionInstalled(Extension* extension, bool is_update) {
+    installed_ = extension;
   }
 
   void TestInstallExtension(const FilePath& path,
@@ -105,18 +105,18 @@ class ExtensionsServiceTestFrontend
     message_loop_.RunAllPending();
     std::vector<std::string> errors = GetErrors();
     if (should_succeed) {
-      EXPECT_EQ(1u, installed_.size()) << path.value();
+      EXPECT_TRUE(installed_) << path.value();
       EXPECT_EQ(0u, errors.size()) << path.value();
       for (std::vector<std::string>::iterator err = errors.begin();
         err != errors.end(); ++err) {
         LOG(ERROR) << *err;
       }
     } else {
-      EXPECT_EQ(0u, installed_.size()) << path.value();
+      EXPECT_FALSE(installed_) << path.value();
       EXPECT_EQ(1u, errors.size()) << path.value();
     }
 
-    installed_.clear();
+    installed_ = NULL;
     ExtensionErrorReporter::GetInstance()->ClearErrors();
   }
 
@@ -124,7 +124,7 @@ class ExtensionsServiceTestFrontend
  private:
   MessageLoop message_loop_;
   ExtensionList extensions_;
-  std::vector<FilePath> installed_;
+  Extension* installed_;
 };
 
 // make the test a PlatformTest to setup autorelease pools properly on mac
diff --git a/chrome/browser/views/bookmark_bar_view.cc b/chrome/browser/views/bookmark_bar_view.cc
index d9ebbd6..88f0fb5 100644
--- a/chrome/browser/views/bookmark_bar_view.cc
+++ b/chrome/browser/views/bookmark_bar_view.cc
@@ -535,8 +535,6 @@ void BookmarkBarView::Layout() {
   if (!GetParent())
     return;
 
-  // First layout out the buttons. Any buttons that are placed beyond the
-  // visible region and made invisible.
   int x = kLeftMargin;
   int y = kTopMargin;
   int width = View::width() - kRightMargin - kLeftMargin;
@@ -566,6 +564,22 @@ void BookmarkBarView::Layout() {
               overflow_pref.width() - kButtonPadding -
               bookmarks_separator_pref.width();
 
+  // First, layout extension toolstrips.
+  // We put these always on the left as a temporary hack until we have the
+  // extension bar. Otherwise, people install extensions and they don't see
+  // anything happen and are confused.
+  for (int i = GetBookmarkButtonCount();
+       i < GetBookmarkButtonCount() + num_extension_toolstrips_; ++i) {
+    views::View* child = GetChildViewAt(i);
+    gfx::Size pref = child->GetPreferredSize();
+    int next_x = x + pref.width() + kButtonPadding;
+    child->SetVisible(next_x < max_x);
+    child->SetBounds(x, y, pref.width(), height);
+    x = next_x;
+  }
+
+  // Next, layout out the buttons. Any buttons that are placed beyond the
+  // visible region and made invisible.
   if (GetBookmarkButtonCount() == 0 && model_ && model_->IsLoaded()) {
     gfx::Size pref = instructions_->GetPreferredSize();
     instructions_->SetBounds(
@@ -587,17 +601,6 @@ void BookmarkBarView::Layout() {
     }
   }
 
-  // Extension toolstrips.
-  for (int i = GetBookmarkButtonCount();
-       i < GetBookmarkButtonCount() + num_extension_toolstrips_; ++i) {
-    views::View* child = GetChildViewAt(i);
-    gfx::Size pref = child->GetPreferredSize();
-    int next_x = x + pref.width() + kButtonPadding;
-    child->SetVisible(next_x < max_x);
-    child->SetBounds(x, y, pref.width(), height);
-    x = next_x;
-  }
-
   // Layout the right side of the bar.
   const bool all_visible =
       (GetBookmarkButtonCount() == 0 ||
diff --git a/chrome/common/temp_scaffolding_stubs.h b/chrome/common/temp_scaffolding_stubs.h
index 2ea2603..4050f4e 100644
--- a/chrome/common/temp_scaffolding_stubs.h
+++ b/chrome/common/temp_scaffolding_stubs.h
@@ -494,7 +494,7 @@ class HWNDView {
   void Layout() { NOTIMPLEMENTED(); }
   void SchedulePaint() { NOTIMPLEMENTED(); }
   HWNDView* GetParent() const { NOTIMPLEMENTED(); return NULL; }
-
+  virtual gfx::Size GetPreferredSize() { NOTIMPLEMENTED(); return gfx::Size(); }
   gfx::NativeWindow GetHWND() { NOTIMPLEMENTED(); return 0; }
   void Detach() { NOTIMPLEMENTED(); }
   gfx::Widget* GetWidget() { NOTIMPLEMENTED(); return NULL; }