try to address some of the issues of overview mode in window manager.

1. stop using repeated timer, ask for snapshot normally take >200ms range. schedule a update task every 10ms do not make sense. It will cause the task backup in the message loop and I suspect following issue are related. http://code.google.com/p/chromium-os/issues/detail?id=7333 and possibly http://code.google.com/p/chromium-os/issues/detail?id=6964 In this patch only one "ask for snapshots" will be posted at the same time. 2. since the feedback loop; always starting to update from begining will starve some tab contents. causing http://code.google.com/p/chromium-os/issues/detail?id=3142 feedback loop is not resolved in this patch. therefore cpu usage in overview mode still high. BUG=7333 and 3142 TEST=for 7333, open maps.google.com, enter overview mode. memory does not get increased. no crash. for 3142, open multiple tabs, and no blank page is seen. Review URL: http://codereview.chromium.org/4700002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@67283 0039d316-1c4b-4281-b951-d872f2087c98
author: jiesun@chromium.org <jiesun@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> 2010-11-24 18:38:25 +0000
committer: jiesun@chromium.org <jiesun@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> 2010-11-24 18:38:25 +0000
commit: 27b6bd796318e1dcfd179f1e37634b5ae04bf161 (patch)
tree: bc25eb56bf607a5819d2274d5b491804968e67cb
parent: b77af94635fc173db8624b43f984896e8332f78b (diff)
download: chromium_src-27b6bd796318e1dcfd179f1e37634b5ae04bf161.zip
chromium_src-27b6bd796318e1dcfd179f1e37634b5ae04bf161.tar.gz
chromium_src-27b6bd796318e1dcfd179f1e37634b5ae04bf161.tar.bz2
2 files changed, 130 insertions, 66 deletions
diff --git a/chrome/browser/chromeos/wm_overview_controller.cc b/chrome/browser/chromeos/wm_overview_controller.cc
index 8d373d8..d4625de 100644
--- a/chrome/browser/chromeos/wm_overview_controller.cc
+++ b/chrome/browser/chromeos/wm_overview_controller.cc
@@ -36,20 +36,12 @@ using std::vector;
 
 namespace chromeos {
 
-// The time that the delay timer waits before starting the
-// configuration pass.
-//
-// NOTE(gspencer): Yes, this is a pretty short delay, and I could
-// remove it and just use the configure timer.  The reason I'm not
-// doing that is that I have a hunch that we'll be needing to tune
-// things further, and this will be one of the knobs.
+// Use this timer to delay consecutive snapshots during the updating process.
+// We will start the timer upon successfully retrieving a new snapshot, or if
+// for some reason the current snapshot is still pending (while Chrome is
+// still loading the page.)
 static const int kDelayTimeMs = 10;
 
-// The time between setting snapshots during the configuration pass,
-// so that the CPU has a chance to do something else (to keep overview
-// mode responsive).
-static const int kConfigureTimeMs = 10;
-
 // This is the size of the web page when we lay it out for a snapshot.
 static const int kSnapshotWebPageWidth = 1024;
 static const int kSnapshotWebPageHeight = 1280;
@@ -102,15 +94,18 @@ class BrowserListener : public TabStripModelObserver {
   void RecreateSnapshots();
 
   // Mark the given snapshot as dirty, and start the delay timer.
-  void ReloadSnapshot(int index);
+  void MarkSnapshotAsDirty(int index);
 
   // Updates the selected index and tab count on the toplevel window.
   void UpdateSelectedIndex(int index);
 
-  // Finds the first cell with no snapshot and invokes ConfigureCell
-  // for it.  Returns false if there are no more cells to configure on
-  // this listener.
-  bool ConfigureNextUnconfiguredSnapshot();
+  // Update the first "dirty" snapshot, which is ordered after (excluding)
+  // the snapshot whose index is given by |start_from|; When |start_from| is
+  // -1, search start at the begining of the list.
+  // Return the index of the snapshot which is actually updated; -1 if there
+  // are no more tab contents (after |start_from|) to configure on this
+  // listener.
+  int ConfigureNextUnconfiguredSnapshot(int start_from);
 
   // Saves the currently selected tab.
   void SaveCurrentTab() { original_selected_tab_ = browser_->selected_index(); }
@@ -126,10 +121,14 @@ class BrowserListener : public TabStripModelObserver {
   // Shows any snapshots that are not visible.
   void ShowSnapshots();
 
+  // Callback for |AskForSnapshot|, start delay timer for next round.
+  void OnSnapshotReady(const SkBitmap& sk_bitmap);
+
   // Returns the tab contents from the tab model for this child at index.
   TabContents* GetTabContentsAt(int index) const  {
     return browser_->tabstrip_model()->GetTabContentsAt(index)->tab_contents();
   }
+
  private:
   // Calculate the size of a cell based on the browser window's size.
   gfx::Size CalculateCellSize();
@@ -156,6 +155,9 @@ class BrowserListener : public TabStripModelObserver {
   Browser* browser_;  // Not owned
   WmOverviewController* controller_;  // Not owned
 
+  // Which renderer host we are working on.
+  RenderWidgetHost* current_renderer_host_; // Not owned
+
   // Widgets containing snapshot images for this browser.  Note that
   // these are all subclasses of WidgetGtk, and they are all added to
   // parents, so they will be deleted by the parents when they are
@@ -168,9 +170,6 @@ class BrowserListener : public TabStripModelObserver {
   typedef std::vector<SnapshotNode> SnapshotVector;
   SnapshotVector snapshots_;
 
-  // True if the snapshots are showing.
-  bool snapshots_showing_;
-
   // Non-zero if we are currently setting the tab from within SelectTab.
   // This is used to make sure we use the right timestamp when sending
   // property changes that originated from the window manager.
@@ -186,7 +185,7 @@ BrowserListener::BrowserListener(Browser* browser,
                                  WmOverviewController* controller)
     : browser_(browser),
       controller_(controller),
-      snapshots_showing_(false),
+      current_renderer_host_(NULL),
       select_tab_timestamp_(0),
       original_selected_tab_(-1) {
   CHECK(browser_);
@@ -242,7 +241,7 @@ void BrowserListener::TabChangedAt(
     snapshots_[index].fav_icon->SetFavIcon(
         contents->tab_contents()->GetFavIcon());
     if (change_type != TabStripModelObserver::TITLE_NOT_LOADING)
-      ReloadSnapshot(index);
+      MarkSnapshotAsDirty(index);
   }
 }
 
@@ -257,9 +256,9 @@ void BrowserListener::TabSelectedAt(TabContentsWrapper* old_contents,
   UpdateSelectedIndex(index);
 }
 
-void BrowserListener::ReloadSnapshot(int index) {
+void BrowserListener::MarkSnapshotAsDirty(int index) {
   snapshots_[index].snapshot->reload_snapshot();
-  controller_->StartDelayTimer();
+  controller_->UpdateSnapshots();
 }
 
 void BrowserListener::RecreateSnapshots() {
@@ -288,15 +287,16 @@ void BrowserListener::UpdateSelectedIndex(int index) {
   }
 }
 
-bool BrowserListener::ConfigureNextUnconfiguredSnapshot() {
-  for (SnapshotVector::size_type i = 0; i < snapshots_.size(); ++i) {
+int BrowserListener::ConfigureNextUnconfiguredSnapshot(int start_from) {
+  for (SnapshotVector::size_type i = start_from + 1;
+      i < snapshots_.size(); ++i) {
     WmOverviewSnapshot* cell = snapshots_[i].snapshot;
     if (!cell->configured_snapshot()) {
       ConfigureCell(cell, i);
-      return true;
+      return i;
     }
   }
-  return false;
+  return -1;
 }
 
 void BrowserListener::RestoreOriginalSelectedTab() {
@@ -361,6 +361,22 @@ gfx::Size BrowserListener::CalculateCellSize() {
   return cell_size;
 }
 
+void BrowserListener::OnSnapshotReady(const SkBitmap& sk_bitmap) {
+  for (int i = 0; i < count(); i++) {
+    RenderWidgetHostView* view =
+        GetTabContentsAt(i)->GetRenderWidgetHostView();
+    if (view && view->GetRenderWidgetHost() == current_renderer_host_) {
+      snapshots_[i].snapshot->SetImage(sk_bitmap);
+      current_renderer_host_ = NULL;
+
+      // Start timer for next round of snapshot updating.
+      controller_->StartDelayTimer();
+      return;
+    }
+  }
+  DCHECK(current_renderer_host_ == NULL);
+}
+
 void BrowserListener::ConfigureCell(WmOverviewSnapshot* cell,
                                     TabContents* contents) {
   if (contents) {
@@ -375,8 +391,9 @@ void BrowserListener::ConfigureCell(WmOverviewSnapshot* cell,
     // which could happen if a tab is closed while it is being
     // rendered.
     ThumbnailGenerator::ThumbnailReadyCallback* callback =
-        NewCallback(cell, &WmOverviewSnapshot::SetImage);
+        NewCallback(this, &BrowserListener::OnSnapshotReady);
 
+    current_renderer_host_ = contents->render_view_host();
     generator->AskForSnapshot(contents->render_view_host(),
                               false,
                               callback,
@@ -389,7 +406,10 @@ void BrowserListener::ConfigureCell(WmOverviewSnapshot* cell,
     // Make sure we set the snapshot image to something, otherwise
     // configured_snapshot remains false and
     // ConfigureNextUnconfiguredSnapshot would get stuck.
+    current_renderer_host_ = NULL;
     cell->SetImage(SkBitmap());
+    cell->reload_snapshot();
+    controller_->StartDelayTimer();
   }
 }
 
@@ -411,7 +431,7 @@ void BrowserListener::InsertSnapshot(int index) {
 
   snapshots_.insert(snapshots_.begin() + index, node);
   node.snapshot->reload_snapshot();
-  controller_->StartDelayTimer();
+  controller_->UpdateSnapshots();
 }
 
 // Removes the snapshot at index.
@@ -442,7 +462,10 @@ WmOverviewController* WmOverviewController::instance() {
 }
 
 WmOverviewController::WmOverviewController()
-    : layout_mode_(ACTIVE_MODE) {
+    : layout_mode_(ACTIVE_MODE),
+      updating_snapshots_(false),
+      browser_listener_index_(0),
+      tab_contents_index_(-1) {
   AddAllBrowsers();
 
   if (registrar_.IsEmpty()) {
@@ -510,12 +533,13 @@ void WmOverviewController::SnapshotImageChanged(RenderWidgetHost* renderer) {
       RenderWidgetHostView* view =
           (*iter)->GetTabContentsAt(i)->GetRenderWidgetHostView();
       if (view && view->GetRenderWidgetHost() == renderer) {
-        (*iter)->ReloadSnapshot(i);
+        (*iter)->MarkSnapshotAsDirty(i);
         return;
       }
     }
     ++iter;
   }
+  DLOG(ERROR) << "SnapshotImageChanged, but we do not know which it is?";
 }
 
 void WmOverviewController::OnBrowserRemoved(const Browser* browser) {
@@ -572,16 +596,15 @@ void WmOverviewController::ProcessWmMessage(const WmIpc::Message& message,
 }
 
 void WmOverviewController::StartDelayTimer() {
-  // We leave the delay timer running if it already is -- this means
-  // we're rate limiting the number of times we can reconfigure the
+  // We're rate limiting the number of times we can reconfigure the
   // snapshots.  If we were to restart the delay timer, it could
   // result in a very long delay until they get configured if tabs
   // keep changing.
-  if (layout_mode_ == OVERVIEW_MODE &&
-      !delay_timer_.IsRunning() && !configure_timer_.IsRunning()) {
+  updating_snapshots_ = false;
+  if (layout_mode_ == OVERVIEW_MODE) {
     delay_timer_.Start(
         base::TimeDelta::FromMilliseconds(kDelayTimeMs), this,
-        &WmOverviewController::StartConfiguring);
+        &WmOverviewController::UpdateSnapshots);
   }
 }
 
@@ -602,41 +625,79 @@ void WmOverviewController::SaveTabSelections() {
 void WmOverviewController::Show() {
   SaveTabSelections();
 
-  // Reset the timers.
-  configure_timer_.Stop();
-  delay_timer_.Stop();
-
   for (BrowserListenerVector::iterator i = listeners_.begin();
        i != listeners_.end(); ++i) {
     (*i)->ShowSnapshots();
   }
-  StartDelayTimer();
+
+  // TODO(jiesun): Make the focused tab as the start point.
+  browser_listener_index_ = 0;
+  tab_contents_index_ = -1;
+  UpdateSnapshots();
 }
 
 void WmOverviewController::Hide(bool cancelled) {
-  configure_timer_.Stop();
   delay_timer_.Stop();
+  updating_snapshots_ = false;
   if (cancelled) {
     RestoreTabSelections();
   }
 }
 
-void WmOverviewController::StartConfiguring() {
-  configure_timer_.Stop();
-  configure_timer_.Start(
-      base::TimeDelta::FromMilliseconds(kConfigureTimeMs), this,
-      &WmOverviewController::ConfigureNextUnconfiguredSnapshot);
-}
+void WmOverviewController::UpdateSnapshots() {
 
-// Just configure one unconfigured cell.  If there aren't any left,
-// then stop the timer.
-void WmOverviewController::ConfigureNextUnconfiguredSnapshot() {
-  for (BrowserListenerVector::size_type i = 0; i < listeners_.size(); ++i) {
-    BrowserListener* listener = listeners_[i].get();
-    if (listener->ConfigureNextUnconfiguredSnapshot())
+  // Only updating snapshots during overview mode.
+  if (layout_mode_ != OVERVIEW_MODE)
+    return;
+
+  // Only reloading snapshots when not already started.
+  if (updating_snapshots_ || delay_timer_.IsRunning())
+    return;
+
+  // Only update one snapshot in round-robin mode.
+  // Start delay timer after each update unless all snapshots had been updated.
+  if (!listeners_.size())
+    return;
+
+  if (int(listeners_.size()) <= browser_listener_index_) {
+    DLOG(INFO) << "Browser listener(s) have disappeared since last update";
+    browser_listener_index_ = 0;
+    tab_contents_index_ = -1;
+  } else {
+    BrowserListener* listener = listeners_[browser_listener_index_].get();
+    if (listener->count() <= tab_contents_index_) {
+      DLOG(INFO) << "Tab content(s) have disappeared since last update";
+      tab_contents_index_ = -1;
+    }
+  }
+
+  int browser_listener_index = browser_listener_index_;
+  int tab_contents_index = tab_contents_index_;
+
+  bool loop_back = false;
+  while (1) {
+    BrowserListener* listener = listeners_[browser_listener_index].get();
+    tab_contents_index =
+        listener->ConfigureNextUnconfiguredSnapshot(tab_contents_index);
+    if (tab_contents_index >= 0) {
+      updating_snapshots_ = true;  // Prevent future parallel updates.
+      browser_listener_index_ = browser_listener_index;
+      tab_contents_index_ = tab_contents_index;
       return;
+    }
+
+    // Found next one;
+    browser_listener_index ++;
+    browser_listener_index = browser_listener_index % listeners_.size();
+    tab_contents_index = -1;
+
+    if (loop_back)
+      break;
+    loop_back = browser_listener_index == browser_listener_index_;
   }
-  configure_timer_.Stop();
+
+  // All snapshots have been fully updated.
+  updating_snapshots_ = false;
 }
 
 void WmOverviewController::AddAllBrowsers() {
diff --git a/chrome/browser/chromeos/wm_overview_controller.h b/chrome/browser/chromeos/wm_overview_controller.h
index d308129..8618e3e 100644
--- a/chrome/browser/chromeos/wm_overview_controller.h
+++ b/chrome/browser/chromeos/wm_overview_controller.h
@@ -86,6 +86,9 @@ class WmOverviewController : public BrowserList::Observer,
   // Used by the BrowserListeners to configure their snapshots.
   const gfx::Rect& monitor_bounds() const { return monitor_bounds_; }
 
+  // Start reloading snapshots if in overview mode.
+  void UpdateSnapshots();
+
   // Starts the delay timer, and once the delay is over, configures
   // any unconfigured snapshots one at a time until none are left to
   // be configured.
@@ -116,13 +119,6 @@ class WmOverviewController : public BrowserList::Observer,
   // shown are restored.
   void Hide(bool cancelled);
 
-  // Invoked by delay_timer_. Starts configure_timer_.
-  void StartConfiguring();
-
-  // Configure the next unconfigured snapshot window owned by any of
-  // the listeners.
-  void ConfigureNextUnconfiguredSnapshot();
-
   // Add browser listeners for all existing browsers, reusing any that
   // were already there.
   void AddAllBrowsers();
@@ -146,12 +142,19 @@ class WmOverviewController : public BrowserList::Observer,
   // See description above class for details.
   base::OneShotTimer<WmOverviewController> delay_timer_;
 
-  // See description above class for details.
-  base::RepeatingTimer<WmOverviewController> configure_timer_;
-
   // The current layout mode.
   LayoutMode layout_mode_;
 
+  // This flag is set whenever there is a pending |AskForSnapshot| to Chrome;
+  // This is used to prevent |UpdateSnapshots| from being called multiple times.
+  bool updating_snapshots_;
+
+  // These indices are used to track the last updated browser listener and tab
+  // content, They are used to implement update tab contents in a round-robin
+  // fashion.
+  int browser_listener_index_;  // index of the last updated browser listener.
+  int tab_contents_index_;      // index of the last updated tab contents.
+
   DISALLOW_COPY_AND_ASSIGN(WmOverviewController);
 };
author	jiesun@chromium.org <jiesun@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>	2010-11-24 18:38:25 +0000
committer	jiesun@chromium.org <jiesun@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>	2010-11-24 18:38:25 +0000
commit	27b6bd796318e1dcfd179f1e37634b5ae04bf161 (patch)
tree	bc25eb56bf607a5819d2274d5b491804968e67cb
parent	b77af94635fc173db8624b43f984896e8332f78b (diff)
download	chromium_src-27b6bd796318e1dcfd179f1e37634b5ae04bf161.zip chromium_src-27b6bd796318e1dcfd179f1e37634b5ae04bf161.tar.gz chromium_src-27b6bd796318e1dcfd179f1e37634b5ae04bf161.tar.bz2