Reland fast tab closure behind a flag

This CL is basically the same as https://codereview.chromium.org/11016023/ + https://codereview.chromium.org/17151010 except that the fast tab closure functionality is implemented behind a flag.

The way the 2 UnloadControllers co-exist is oriented towards being able to quickly and easily rip out the old UnloadController when we have all bugs fixed with the new one.

Original change description:
Changes to closing contents with beforeunload/unload handlers: - Closing a single tab, run beforeunload (if needed), then detached the tab from tab strip and close it asynchronously (no ui). - Closing a window, run all beforeunload handlers (same as before), then detach all tabs with unload handlers. Close any remaining tabs and hide the browser window while waiting for the unload handlers to complete.

The feature is currently implemented behind a flag - --enable-fast-unload

BUG=142458, 156896
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209674

[jeremy, 2013-06-25 10:50:50]

Changes VS previous reviewed CLs:
* chrome/test/data/* - unchanged
* unload_browser_test - unchanged, save for running tests with the flag.
* unload_controller.* - moved to fast_unload_controller.* and class rename.
* core_tab_helper.cc - unchanged - the old and new histogram recording code is
separated with an if.

Apart from that changes are just addition and plumbing around the new flag and
if()s based on the new flag aimed at providing clear separation between the
old/new behavior so we can remove the old code quickly and cleanly.

[Avi (use Gerrit), 2013-06-25 14:34:44]

On 2013/06/25 10:50:50, jeremy wrote:
> Changes VS previous reviewed CLs:
> * chrome/test/data/* - unchanged
> * unload_browser_test - unchanged, save for running tests with the flag.
> * unload_controller.* - moved to fast_unload_controller.* and class rename.
> * core_tab_helper.cc - unchanged - the old and new histogram recording code is
> separated with an if.
> 
> Apart from that changes are just addition and plumbing around the new flag and
> if()s based on the new flag aimed at providing clear separation between the
> old/new behavior so we can remove the old code quickly and cleanly.

In that case I'm OK with it. Good luck in working it out.

LGTM

[sky, 2013-06-25 16:00:22]

Could you make it so the first patch is the baseline and the second you're
changes since then?

[jeremy, 2013-06-27 10:47:46]

Ready for another look, diff now based on reviewed patchset.

[sky, 2013-06-27 16:13:16]

The diffs still weren't right. Anyway, LGTM with the following changes.

https://codereview.chromium.org/17571018/diff/22001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/17571018/diff/22001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:238: bool FastTabUnloadEnabled() {
IsFastTabUnloadEnabled?

https://codereview.chromium.org/17571018/diff/22001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:602: else
nit: no else after a return.

https://codereview.chromium.org/17571018/diff/22001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:618: else
same nit here.

https://codereview.chromium.org/17571018/diff/22001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:1203: else
and here.

https://codereview.chromium.org/17571018/diff/22001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:1409: if (FastTabUnloadEnabled())
nit: use {} for multi-line ones.

[commit-bot: I haz the power, 2013-06-30 12:16:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/17571018/44001

[commit-bot: I haz the power, 2013-07-01 07:17:49]

Failed to apply patch for chrome/browser/ui/browser.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/browser.cc
  Hunk #1 FAILED at 108.
  Hunk #2 succeeded at 232 (offset -1 lines).
  Hunk #3 succeeded at 337 (offset -1 lines).
  Hunk #4 succeeded at 352 (offset -1 lines).
  Hunk #5 succeeded at 595 (offset -1 lines).
  Hunk #6 succeeded at 651 (offset -1 lines).
  Hunk #7 succeeded at 1191 (offset -1 lines).
  Hunk #8 succeeded at 1329 (offset -1 lines).
  Hunk #9 succeeded at 1398 (offset -1 lines).
  1 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/ui/browser.cc.rej

Patch:       chrome/browser/ui/browser.cc
Index: chrome/browser/ui/browser.cc
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index
f60bd76cf9842f5557bbd90e09138a8b4b858030..4889c4f691580888605917489310fad6355481e5
100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -108,6 +108,7 @@
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/chrome_select_file_policy.h"
 #include "chrome/browser/ui/extensions/shell_window.h"
+#include "chrome/browser/ui/fast_unload_controller.h"
 #include "chrome/browser/ui/find_bar/find_bar.h"
 #include "chrome/browser/ui/find_bar/find_bar_controller.h"
 #include "chrome/browser/ui/find_bar/find_tab_helper.h"
@@ -233,6 +234,12 @@ BrowserWindow* CreateBrowserWindow(Browser* browser) {
   return BrowserWindow::CreateBrowserWindow(browser);
 }
 
+// Is the fast tab unload experiment enabled?
+bool IsFastTabUnloadEnabled() {
+  return CommandLine::ForCurrentProcess()->HasSwitch(
+        switches::kEnableFastUnload);
+}
+
 }  // namespace
 

////////////////////////////////////////////////////////////////////////////////
@@ -332,7 +339,6 @@ Browser::Browser(const CreateParams& params)
       initial_show_state_(params.initial_show_state),
       is_session_restore_(params.is_session_restore),
       host_desktop_type_(params.host_desktop_type),
-      unload_controller_(new chrome::UnloadController(this)),
       weak_factory_(this),
       content_setting_bubble_model_delegate_(
           new BrowserContentSettingBubbleModelDelegate(this)),
@@ -348,6 +354,12 @@ Browser::Browser(const CreateParams& params)
   // from opening at all, but the path that triggered it should be fixed.
   CHECK(IncognitoModePrefs::CanOpenBrowser(profile_));
 
+  // TODO(jeremy): Move to initializer list once flag is removed.
+  if (IsFastTabUnloadEnabled())
+    fast_unload_controller_.reset(new chrome::FastUnloadController(this));
+  else
+    unload_controller_.reset(new chrome::UnloadController(this));
+
   if (!app_name_.empty())
     chrome::RegisterAppPrefs(app_name_, profile_);
   tab_strip_model_->AddObserver(this);
@@ -585,10 +597,19 @@ bool Browser::ShouldCloseWindow() {
   if (!CanCloseWithInProgressDownloads())
     return false;
 
+  if (IsFastTabUnloadEnabled())
+    return fast_unload_controller_->ShouldCloseWindow();
   return unload_controller_->ShouldCloseWindow();
 }
 
+bool Browser::HasCompletedUnloadProcessing() const {
+  DCHECK(IsFastTabUnloadEnabled());
+  return fast_unload_controller_->HasCompletedUnloadProcessing();
+}
+
 bool Browser::IsAttemptingToCloseBrowser() const {
+  if (IsFastTabUnloadEnabled())
+    return fast_unload_controller_->is_attempting_to_close_browser();
   return unload_controller_->is_attempting_to_close_browser();
 }
 
@@ -632,7 +653,8 @@ void Browser::OnWindowClosing() {
       content::Source<Browser>(this),
       content::NotificationService::NoDetails());
 
-  tab_strip_model_->CloseAllTabs();
+  if (!IsFastTabUnloadEnabled())
+    tab_strip_model_->CloseAllTabs();
 }
 

////////////////////////////////////////////////////////////////////////////////
@@ -1171,6 +1193,8 @@ void Browser::HandleKeyboardEvent(content::WebContents*
source,
 }
 
 bool Browser::TabsNeedBeforeUnloadFired() {
+  if (IsFastTabUnloadEnabled())
+    return fast_unload_controller_->TabsNeedBeforeUnloadFired();
   return unload_controller_->TabsNeedBeforeUnloadFired();
 }
 
@@ -1307,7 +1331,13 @@ void Browser::LoadingStateChanged(WebContents* source) {
 }
 
 void Browser::CloseContents(WebContents* source) {
-  if (unload_controller_->CanCloseContents(source))
+  bool can_close_contents;
+  if (IsFastTabUnloadEnabled())
+    can_close_contents = fast_unload_controller_->CanCloseContents(source);
+  else
+    can_close_contents = unload_controller_->CanCloseContents(source);
+
+  if (can_close_contents)
     chrome::CloseWebContents(this, source, true);
 }
 
@@ -1370,8 +1400,13 @@ gfx::Rect Browser::GetRootWindowResizerRect() const {
 void Browser::BeforeUnloadFired(WebContents* web_contents,
                                 bool proceed,
                                 bool* proceed_to_fire_unload) {
-  *proceed_to_fire_unload =
-      unload_controller_->BeforeUnloadFired(web_contents, proceed);
+  if (IsFastTabUnloadEnabled()) {
+    *proceed_to_fire_unload =
+        fast_unload_controller_->BeforeUnloadFired(web_contents, proceed);
+  } else {
+    *proceed_to_fire_unload =
+        unload_controller_->BeforeUnloadFired(web_contents, proceed);
+  }
 }
 
 bool Browser::ShouldFocusLocationBarByDefault(WebContents* source) {

[commit-bot: I haz the power, 2013-07-02 09:26:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/17571018/63001

[commit-bot: I haz the power, 2013-07-02 09:34:46]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-07-02 11:10:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/17571018/63001

[commit-bot: I haz the power, 2013-07-02 11:56:37]

Change committed as 209674

[Avi (use Gerrit), 2013-10-11 17:58:07]

https://chromiumcodereview.appspot.com/17571018/diff/63001/chrome/browser/unl...
File chrome/browser/unload_browsertest.cc (right):

https://chromiumcodereview.appspot.com/17571018/diff/63001/chrome/browser/unl...
chrome/browser/unload_browsertest.cc:501:
content::NotificationService::AllSources());
You cannot use WindowedNotificationObserver like this. What is triggering the
unload? "Oh, it happens sometime before now"? This is a guaranteed flaky test.

[jeremy, 2013-10-13 14:00:19]

Thank you very much for looking at this Avi!  The plan is to get back to
working on this in a few weeks...


On Fri, Oct 11, 2013 at 8:58 PM, <avi@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/17571018/diff/**
>
63001/chrome/browser/unload_**browsertest.cc<https://chromiumcodereview.appspot.com/17571018/diff/63001/chrome/browser/unload_browsertest.cc>
> File chrome/browser/unload_**browsertest.cc (right):
>
> https://chromiumcodereview.**appspot.com/17571018/diff/**
>
63001/chrome/browser/unload_**browsertest.cc#newcode501<https://chromiumcodereview.appspot.com/17571018/diff/63001/chrome/browser/unload_browsertest.cc#newcode501>
> chrome/browser/unload_**browsertest.cc:501:
> content::NotificationService::**AllSources());
> You cannot use WindowedNotificationObserver like this. What is
> triggering the unload? "Oh, it happens sometime before now"? This is a
> guaranteed flaky test.
>
>
https://chromiumcodereview.**appspot.com/17571018/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Avi (use Gerrit), 2013-10-13 16:29:47]

I found it because it broke my conversion away from notifications. My fix is in
https://codereview.chromium.org/26277010 which I'm working on now. Ignore it; I
will adapt my code to whatever you write.