[Session restore] Expose lazy mode and fix browser tests.

[Session restore] Expose lazy mode and fix browser tests.

- SessionRestore now exposes IsLoadingActiveTabsOnly
- Fixed 2 browser tests that were failing with new behavior
- One last telemetry test still failing, will be addressed in a separate CL

Notes:
- First patchset inverts the default behavior to show tests succeeding
- Second patchset restores default behavior

BUG=

Committed: https://crrev.com/2f469295118ecfdfc41209b4c495ca6cb5644c6f
Cr-Commit-Position: refs/heads/master@{#325528}

[Georges Khalil, 2015-04-15 15:59:37]

Patchset #1 (id:1) has been deleted

[Georges Khalil, 2015-04-15 15:59:42]

Patchset #1 (id:20001) has been deleted

[Georges Khalil, 2015-04-15 15:59:46]

Patchset #1 (id:40001) has been deleted

[Georges Khalil, 2015-04-15 16:01:28]

Patchset #1 (id:60001) has been deleted

[Georges Khalil, 2015-04-15 16:55:13]

Patchset #1 (id:70005) has been deleted

[Georges Khalil, 2015-04-15 16:55:19]

Patchset #1 (id:90001) has been deleted

[Georges Khalil, 2015-04-15 16:55:24]

Patchset #1 (id:110001) has been deleted

[Georges Khalil, 2015-04-15 17:58:41]

georgesak@chromium.org changed reviewers:
+ sky@chromium.org

[Georges Khalil, 2015-04-15 17:58:41]

PTAL.

- SessionRestore now exposes IsLoadingActiveTabsOnly
- Fixed 2 browser tests that were failing with new behavior
- One last telemetry test still failing, will be addressed in a separate CL

Notes:
- First patchset inverts the default behavior to show tests succeeding
- Second patchset restores default behavior

[sky, 2015-04-15 19:51:38]

I got the feeling we were going to nuke the experiment as it didn't
pan out. Am I wrong there?

On Wed, Apr 15, 2015 at 10:58 AM,  <georgesak@chromium.org> wrote:
> Reviewers: sky,
>
> Message:
> PTAL.
>
> - SessionRestore now exposes IsLoadingActiveTabsOnly
> - Fixed 2 browser tests that were failing with new behavior
> - One last telemetry test still failing, will be addressed in a separate CL
>
> Notes:
> - First patchset inverts the default behavior to show tests succeeding
> - Second patchset restores default behavior
>
> Description:
> [Session restore] Expose lazy mode and fix browser tests.
>
> - SessionRestore now exposes IsLoadingActiveTabsOnly
> - Fixed 2 browser tests that were failing with new behavior
> - One last telemetry test still failing, will be addressed in a separate CL
>
> Notes:
> - First patchset inverts the default behavior to show tests succeeding
> - Second patchset restores default behavior
>
> BUG=
>
> Please review this at https://codereview.chromium.org/1087063003/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+31, -13 lines):
>   M chrome/browser/sessions/better_session_restore_browsertest.cc
>   M chrome/browser/sessions/session_restore.h
>   M chrome/browser/sessions/session_restore.cc
>   M chrome/browser/sessions/session_restore_delegate.h
>   M chrome/browser/sessions/session_restore_delegate.cc
>
>
> Index: chrome/browser/sessions/better_session_restore_browsertest.cc
> diff --git a/chrome/browser/sessions/better_session_restore_browsertest.cc
> b/chrome/browser/sessions/better_session_restore_browsertest.cc
> index
>
b6c902602705912c394a4dfe3bd2a2f7ec7deb59..c611a04e2fe3aa2b8bd7cd299116904d2ec6012b
> 100644
> --- a/chrome/browser/sessions/better_session_restore_browsertest.cc
> +++ b/chrome/browser/sessions/better_session_restore_browsertest.cc
> @@ -214,6 +214,12 @@ class BetterSessionRestoreTest : public
> InProcessBrowserTest {
>    }
>
>    void CheckReloadedPageNotRestored() {
> +    // If only active tabs are loaded, force loading of the tab.
> +    if (SessionRestore::IsLoadingActiveTabsOnly()) {
> +      content::WebContents* web_contents =
> +          browser()->tab_strip_model()->GetWebContentsAt(0);
> +      web_contents->GetController().LoadIfNecessary();
> +    }
>      CheckReloadedPageNotRestored(browser());
>    }
>
> Index: chrome/browser/sessions/session_restore.cc
> diff --git a/chrome/browser/sessions/session_restore.cc
> b/chrome/browser/sessions/session_restore.cc
> index
>
82d6432b38bc273c043928e2093f387a7f7fa0f1..400f479d43b9bc83610a7f1f1130747e735464eb
> 100644
> --- a/chrome/browser/sessions/session_restore.cc
> +++ b/chrome/browser/sessions/session_restore.cc
> @@ -16,6 +16,7 @@
>  #include "base/debug/alias.h"
>  #include "base/memory/scoped_ptr.h"
>  #include "base/memory/scoped_vector.h"
> +#include "base/metrics/field_trial.h"
>  #include "base/metrics/histogram.h"
>  #include "base/run_loop.h"
>  #include "base/stl_util.h"
> @@ -298,7 +299,9 @@ class SessionRestoreImpl : public
> content::NotificationObserver {
>
>      if (succeeded) {
>        // Start Loading tabs.
> -      SessionRestoreDelegate::RestoreTabs(contents_created,
> restore_started_);
> +      bool active_only = SessionRestore::IsLoadingActiveTabsOnly();
> +      SessionRestoreDelegate::RestoreTabs(contents_created,
> restore_started_,
> +                                          active_only);
>      }
>
>      if (!synchronous_) {
> @@ -818,5 +821,14 @@ SessionRestore::CallbackSubscription
>  }
>
>  // static
> +bool SessionRestore::IsLoadingActiveTabsOnly() {
> +  base::FieldTrial* trial =
> +      base::FieldTrialList::Find("SessionRestoreBackgroundLoading");
> +  if (!trial || trial->group_name() == "Restore")
> +    return false;
> +  return true;
> +}
> +
> +// static
>  base::CallbackList<void(int)>*
>      SessionRestore::on_session_restored_callbacks_ = nullptr;
> Index: chrome/browser/sessions/session_restore.h
> diff --git a/chrome/browser/sessions/session_restore.h
> b/chrome/browser/sessions/session_restore.h
> index
>
e6cb7819e514e7319d77cb150a8902ad28df3234..6eaccbdbfbdedb74c7d120a75db6cf312457e2d5
> 100644
> --- a/chrome/browser/sessions/session_restore.h
> +++ b/chrome/browser/sessions/session_restore.h
> @@ -98,6 +98,11 @@ class SessionRestore {
>    static CallbackSubscription RegisterOnSessionRestoredCallback(
>        const base::Callback<void(int)>& callback);
>
> +  // Returns true if only active tabs are to be loaded during session
> restore
> +  // (lazy load). Otherwise returns false if all tabs will be loaded when
> +  // restored.
> +  static bool IsLoadingActiveTabsOnly();
> +
>   private:
>    SessionRestore();
>
> Index: chrome/browser/sessions/session_restore_delegate.cc
> diff --git a/chrome/browser/sessions/session_restore_delegate.cc
> b/chrome/browser/sessions/session_restore_delegate.cc
> index
>
d409d38a66c6c9265c21e61a175586057a609446..39be456ad2bbf5c52c630fb63b4f2e557f3e7376
> 100644
> --- a/chrome/browser/sessions/session_restore_delegate.cc
> +++ b/chrome/browser/sessions/session_restore_delegate.cc
> @@ -10,17 +10,12 @@
>  #include "components/favicon/content/content_favicon_driver.h"
>
>  // static
> -void SessionRestoreDelegate::RestoreTabs(
> -    const std::vector<RestoredTab>& tabs,
> -    const base::TimeTicks& restore_started) {
> -  // TODO(georgesak): make tests aware of that behavior so that they
> succeed if
> -  // tab loading is disabled.
> -  base::FieldTrial* trial =
> -      base::FieldTrialList::Find("SessionRestoreBackgroundLoading");
> -  bool active_only = true;
> -  if (!trial || trial->group_name() == "Restore") {
> +void SessionRestoreDelegate::RestoreTabs(const std::vector<RestoredTab>&
> tabs,
> +                                         const base::TimeTicks&
> restore_started,
> +                                         bool active_only) {
> +  SessionRestoreStatsCollector::TrackTabs(tabs, restore_started,
> active_only);
> +  if (!active_only) {
>      TabLoader::RestoreTabs(tabs, restore_started);
> -    active_only = false;
>    } else {
>      // If we are not loading inactive tabs, restore their favicons (title
> has
>      // already been set by now).
> @@ -32,5 +27,4 @@ void SessionRestoreDelegate::RestoreTabs(
>        }
>      }
>    }
> -  SessionRestoreStatsCollector::TrackTabs(tabs, restore_started,
> active_only);
>  }
> Index: chrome/browser/sessions/session_restore_delegate.h
> diff --git a/chrome/browser/sessions/session_restore_delegate.h
> b/chrome/browser/sessions/session_restore_delegate.h
> index
>
019ddacd24e67c42aeee9fc526eaed5ddb54c42b..646126ca59ca526c4aecba15f7b813ce28315daf
> 100644
> --- a/chrome/browser/sessions/session_restore_delegate.h
> +++ b/chrome/browser/sessions/session_restore_delegate.h
> @@ -23,7 +23,8 @@ class SessionRestoreDelegate {
>    };
>
>    static void RestoreTabs(const std::vector<RestoredTab>& tabs,
> -                          const base::TimeTicks& restore_started);
> +                          const base::TimeTicks& restore_started,
> +                          bool active_only);
>
>   private:
>    DISALLOW_IMPLICIT_CONSTRUCTORS(SessionRestoreDelegate);
>
>

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

[Georges Khalil, 2015-04-15 19:56:12]

Experiment is still alive. It's 90/10 on canary/dev and going to be rolled out
on beta soon.

On 2015/04/15 19:51:38, sky wrote:
> I got the feeling we were going to nuke the experiment as it didn't
> pan out. Am I wrong there?
> 
> On Wed, Apr 15, 2015 at 10:58 AM,  <mailto:georgesak@chromium.org> wrote:
> > Reviewers: sky,
> >
> > Message:
> > PTAL.
> >
> > - SessionRestore now exposes IsLoadingActiveTabsOnly
> > - Fixed 2 browser tests that were failing with new behavior
> > - One last telemetry test still failing, will be addressed in a separate CL
> >
> > Notes:
> > - First patchset inverts the default behavior to show tests succeeding
> > - Second patchset restores default behavior
> >
> > Description:
> > [Session restore] Expose lazy mode and fix browser tests.
> >
> > - SessionRestore now exposes IsLoadingActiveTabsOnly
> > - Fixed 2 browser tests that were failing with new behavior
> > - One last telemetry test still failing, will be addressed in a separate CL
> >
> > Notes:
> > - First patchset inverts the default behavior to show tests succeeding
> > - Second patchset restores default behavior
> >
> > BUG=
> >
> > Please review this at https://codereview.chromium.org/1087063003/
> >
> > Base URL: https://chromium.googlesource.com/chromium/src.git@master
> >
> > Affected files (+31, -13 lines):
> >   M chrome/browser/sessions/better_session_restore_browsertest.cc
> >   M chrome/browser/sessions/session_restore.h
> >   M chrome/browser/sessions/session_restore.cc
> >   M chrome/browser/sessions/session_restore_delegate.h
> >   M chrome/browser/sessions/session_restore_delegate.cc
> >
> >
> > Index: chrome/browser/sessions/better_session_restore_browsertest.cc
> > diff --git a/chrome/browser/sessions/better_session_restore_browsertest.cc
> > b/chrome/browser/sessions/better_session_restore_browsertest.cc
> > index
> >
>
b6c902602705912c394a4dfe3bd2a2f7ec7deb59..c611a04e2fe3aa2b8bd7cd299116904d2ec6012b
> > 100644
> > --- a/chrome/browser/sessions/better_session_restore_browsertest.cc
> > +++ b/chrome/browser/sessions/better_session_restore_browsertest.cc
> > @@ -214,6 +214,12 @@ class BetterSessionRestoreTest : public
> > InProcessBrowserTest {
> >    }
> >
> >    void CheckReloadedPageNotRestored() {
> > +    // If only active tabs are loaded, force loading of the tab.
> > +    if (SessionRestore::IsLoadingActiveTabsOnly()) {
> > +      content::WebContents* web_contents =
> > +          browser()->tab_strip_model()->GetWebContentsAt(0);
> > +      web_contents->GetController().LoadIfNecessary();
> > +    }
> >      CheckReloadedPageNotRestored(browser());
> >    }
> >
> > Index: chrome/browser/sessions/session_restore.cc
> > diff --git a/chrome/browser/sessions/session_restore.cc
> > b/chrome/browser/sessions/session_restore.cc
> > index
> >
>
82d6432b38bc273c043928e2093f387a7f7fa0f1..400f479d43b9bc83610a7f1f1130747e735464eb
> > 100644
> > --- a/chrome/browser/sessions/session_restore.cc
> > +++ b/chrome/browser/sessions/session_restore.cc
> > @@ -16,6 +16,7 @@
> >  #include "base/debug/alias.h"
> >  #include "base/memory/scoped_ptr.h"
> >  #include "base/memory/scoped_vector.h"
> > +#include "base/metrics/field_trial.h"
> >  #include "base/metrics/histogram.h"
> >  #include "base/run_loop.h"
> >  #include "base/stl_util.h"
> > @@ -298,7 +299,9 @@ class SessionRestoreImpl : public
> > content::NotificationObserver {
> >
> >      if (succeeded) {
> >        // Start Loading tabs.
> > -      SessionRestoreDelegate::RestoreTabs(contents_created,
> > restore_started_);
> > +      bool active_only = SessionRestore::IsLoadingActiveTabsOnly();
> > +      SessionRestoreDelegate::RestoreTabs(contents_created,
> > restore_started_,
> > +                                          active_only);
> >      }
> >
> >      if (!synchronous_) {
> > @@ -818,5 +821,14 @@ SessionRestore::CallbackSubscription
> >  }
> >
> >  // static
> > +bool SessionRestore::IsLoadingActiveTabsOnly() {
> > +  base::FieldTrial* trial =
> > +      base::FieldTrialList::Find("SessionRestoreBackgroundLoading");
> > +  if (!trial || trial->group_name() == "Restore")
> > +    return false;
> > +  return true;
> > +}
> > +
> > +// static
> >  base::CallbackList<void(int)>*
> >      SessionRestore::on_session_restored_callbacks_ = nullptr;
> > Index: chrome/browser/sessions/session_restore.h
> > diff --git a/chrome/browser/sessions/session_restore.h
> > b/chrome/browser/sessions/session_restore.h
> > index
> >
>
e6cb7819e514e7319d77cb150a8902ad28df3234..6eaccbdbfbdedb74c7d120a75db6cf312457e2d5
> > 100644
> > --- a/chrome/browser/sessions/session_restore.h
> > +++ b/chrome/browser/sessions/session_restore.h
> > @@ -98,6 +98,11 @@ class SessionRestore {
> >    static CallbackSubscription RegisterOnSessionRestoredCallback(
> >        const base::Callback<void(int)>& callback);
> >
> > +  // Returns true if only active tabs are to be loaded during session
> > restore
> > +  // (lazy load). Otherwise returns false if all tabs will be loaded when
> > +  // restored.
> > +  static bool IsLoadingActiveTabsOnly();
> > +
> >   private:
> >    SessionRestore();
> >
> > Index: chrome/browser/sessions/session_restore_delegate.cc
> > diff --git a/chrome/browser/sessions/session_restore_delegate.cc
> > b/chrome/browser/sessions/session_restore_delegate.cc
> > index
> >
>
d409d38a66c6c9265c21e61a175586057a609446..39be456ad2bbf5c52c630fb63b4f2e557f3e7376
> > 100644
> > --- a/chrome/browser/sessions/session_restore_delegate.cc
> > +++ b/chrome/browser/sessions/session_restore_delegate.cc
> > @@ -10,17 +10,12 @@
> >  #include "components/favicon/content/content_favicon_driver.h"
> >
> >  // static
> > -void SessionRestoreDelegate::RestoreTabs(
> > -    const std::vector<RestoredTab>& tabs,
> > -    const base::TimeTicks& restore_started) {
> > -  // TODO(georgesak): make tests aware of that behavior so that they
> > succeed if
> > -  // tab loading is disabled.
> > -  base::FieldTrial* trial =
> > -      base::FieldTrialList::Find("SessionRestoreBackgroundLoading");
> > -  bool active_only = true;
> > -  if (!trial || trial->group_name() == "Restore") {
> > +void SessionRestoreDelegate::RestoreTabs(const std::vector<RestoredTab>&
> > tabs,
> > +                                         const base::TimeTicks&
> > restore_started,
> > +                                         bool active_only) {
> > +  SessionRestoreStatsCollector::TrackTabs(tabs, restore_started,
> > active_only);
> > +  if (!active_only) {
> >      TabLoader::RestoreTabs(tabs, restore_started);
> > -    active_only = false;
> >    } else {
> >      // If we are not loading inactive tabs, restore their favicons (title
> > has
> >      // already been set by now).
> > @@ -32,5 +27,4 @@ void SessionRestoreDelegate::RestoreTabs(
> >        }
> >      }
> >    }
> > -  SessionRestoreStatsCollector::TrackTabs(tabs, restore_started,
> > active_only);
> >  }
> > Index: chrome/browser/sessions/session_restore_delegate.h
> > diff --git a/chrome/browser/sessions/session_restore_delegate.h
> > b/chrome/browser/sessions/session_restore_delegate.h
> > index
> >
>
019ddacd24e67c42aeee9fc526eaed5ddb54c42b..646126ca59ca526c4aecba15f7b813ce28315daf
> > 100644
> > --- a/chrome/browser/sessions/session_restore_delegate.h
> > +++ b/chrome/browser/sessions/session_restore_delegate.h
> > @@ -23,7 +23,8 @@ class SessionRestoreDelegate {
> >    };
> >
> >    static void RestoreTabs(const std::vector<RestoredTab>& tabs,
> > -                          const base::TimeTicks& restore_started);
> > +                          const base::TimeTicks& restore_started,
> > +                          bool active_only);
> >
> >   private:
> >    DISALLOW_IMPLICIT_CONSTRUCTORS(SessionRestoreDelegate);
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-04-16 15:40:10]

https://codereview.chromium.org/1087063003/diff/140001/chrome/browser/session...
File chrome/browser/sessions/better_session_restore_browsertest.cc (right):

https://codereview.chromium.org/1087063003/diff/140001/chrome/browser/session...
chrome/browser/sessions/better_session_restore_browsertest.cc:220:
browser()->tab_strip_model()->GetWebContentsAt(0);
Is there only ever one webcontents that isn't loading? Seems better to invoke
LoadIfNecessary on all webcontents.

https://codereview.chromium.org/1087063003/diff/140001/chrome/browser/session...
File chrome/browser/sessions/session_restore.h (right):

https://codereview.chromium.org/1087063003/diff/140001/chrome/browser/session...
chrome/browser/sessions/session_restore.h:104: static bool
IsLoadingActiveTabsOnly();
This seems more like a will, eg WillLoadActiveTabsOnly.

[Georges Khalil, 2015-04-16 18:10:18]

PTAnL.

Tested locally with the default behavior inverted, tests pass.

https://codereview.chromium.org/1087063003/diff/140001/chrome/browser/session...
File chrome/browser/sessions/better_session_restore_browsertest.cc (right):

https://codereview.chromium.org/1087063003/diff/140001/chrome/browser/session...
chrome/browser/sessions/better_session_restore_browsertest.cc:220:
browser()->tab_strip_model()->GetWebContentsAt(0);
On 2015/04/16 15:40:10, sky wrote:
> Is there only ever one webcontents that isn't loading? Seems better to invoke
> LoadIfNecessary on all webcontents.

Yes, this only has one page. If calls CheckTitle which waits
for the title of that single page to change (line 230).

That being said, the change was simple to loop the webcontents
and makes the code more futureproof.

Done.

https://codereview.chromium.org/1087063003/diff/140001/chrome/browser/session...
File chrome/browser/sessions/session_restore.h (right):

https://codereview.chromium.org/1087063003/diff/140001/chrome/browser/session...
chrome/browser/sessions/session_restore.h:104: static bool
IsLoadingActiveTabsOnly();
On 2015/04/16 15:40:10, sky wrote:
> This seems more like a will, eg WillLoadActiveTabsOnly.

Done.

[sky, 2015-04-16 19:15:44]

LGTM

[Georges Khalil, 2015-04-16 19:23:27]

The CQ bit was checked by georgesak@chromium.org

[commit-bot: I haz the power, 2015-04-16 19:25:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1087063003/160001

[commit-bot: I haz the power, 2015-04-16 22:46:23]

Committed patchset #3 (id:160001)

[commit-bot: I haz the power, 2015-04-16 22:48:04]

Patchset 3 (id:??) landed as
https://crrev.com/2f469295118ecfdfc41209b4c495ca6cb5644c6f
Cr-Commit-Position: refs/heads/master@{#325528}