[Session restore] Add MRU logic to loading of background pages.

[Session restore] Add MRU logic to loading of background pages.

This patch adds the notion of last activation time to tabs and when a session restores, background tabs are loaded using MRU.

BUG=472772
Committed: https://crrev.com/5582cbe405f6dc48b14e2cae7254aa03abcf353b
Cr-Commit-Position: refs/heads/master@{#331189}

[Georges Khalil, 2015-05-11 21:12:49]

Patchset #1 (id:1) has been deleted

[Georges Khalil, 2015-05-12 17:40:18]

Patchset #1 (id:10014) has been deleted

[Georges Khalil, 2015-05-12 17:51:01]

Patchset #1 (id:30001) has been deleted

[Georges Khalil, 2015-05-12 18:11:56]

Patchset #2 (id:60001) has been deleted

[Georges Khalil, 2015-05-12 18:12:03]

Patchset #1 (id:50001) has been deleted

[Georges Khalil, 2015-05-12 19:37:20]

Patchset #1 (id:80001) has been deleted

[Georges Khalil, 2015-05-12 19:39:11]

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

[Georges Khalil, 2015-05-12 19:39:26]

PTAL.

[sky, 2015-05-12 21:25:30]

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore.cc:600:
web_contents->SetLastActiveTime(tab.last_activation_time);
It's my understanding there is no guarantee that timeticks increases across a
reboot. I suspect if you have your machine running for a long time, reboot, then
tab.last_activation_time could be very far in the future.

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
File chrome/browser/sessions/session_restore_browsertest.cc (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore_browsertest.cc:282:
"IntelligentSessionRestore.TestGroup:PrioritizeTabs/mru");
Ouch. Is the old one dumb?

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore_browsertest.cc:1432: const int NumTabs =
6;
num_tabs

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore_browsertest.cc:1446:
std::random_shuffle(activation_order.begin(), activation_order.end());
It's not a good idea to randomize test data. If there is flake then it can be
hard to repro. Much better if you order it in a specific way. Do multiple runs
of the test if appropriate.

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore_browsertest.cc:1459: for (auto i :
activation_order) {
nit: no {}

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
File chrome/browser/sessions/session_restore_delegate.h (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore_delegate.h:26: base::TimeTicks
last_activation_time);
If you push this to the WebContents, do you need it here too?

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_service.cc:496: ScheduleCommand(
Don't you want to gate this by SessionRestore::GetSmartRestoreMode() ==
SessionRestore::SMART_RESTORE_MODE_MRU) ? Otherwise you can't really compare
metrics relating to file information.

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_service.cc:740: // This works because we are
persisting the last activation when restoring
I don't understand this comment. It likely can be nuked.

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:1084: session_service->SetActivationTime(
I think you should only do this if the tab is active for long enough. For
example, if I rapidly control-tab between a bunch of tabs there is no point in
generating an id for each. If you didn't do this, then you could add the
activation time to SetSelectedTabInWindow.

https://codereview.chromium.org/1131373003/diff/100001/components/sessions/ba...
File components/sessions/base_session_service_commands.h (right):

https://codereview.chromium.org/1131373003/diff/100001/components/sessions/ba...
components/sessions/base_session_service_commands.h:47: SESSIONS_EXPORT
scoped_ptr<SessionCommand> CreateLastActiveTimeCommand(
Is this used?

[Georges Khalil, 2015-05-15 15:31:41]

Patchset #3 (id:140001) has been deleted

[Georges Khalil, 2015-05-15 16:54:08]

Patchset #4 (id:170001) has been deleted

[Georges Khalil, 2015-05-15 16:55:29]

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

[Georges Khalil, 2015-05-15 16:55:31]

sky@chromium.org: PTAnL.

jam@chromium.org: Please review changes in content/*

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore.cc:600:
web_contents->SetLastActiveTime(tab.last_activation_time);
On 2015/05/12 21:25:29, sky wrote:
> It's my understanding there is no guarantee that timeticks increases across a
> reboot. I suspect if you have your machine running for a long time, reboot,
then
> tab.last_activation_time could be very far in the future.

You are right, if the OS reboots, TimeTicks become irrelevant. Landed another CL
that changes WebContents::last_active_time to Time instead of TimeTicks. This
should fix it as we're using Time now. I also changed last_activation_time to
last_active_time for consistency with WebContents.

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
File chrome/browser/sessions/session_restore_browsertest.cc (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore_browsertest.cc:282:
"IntelligentSessionRestore.TestGroup:PrioritizeTabs/mru");
On 2015/05/12 21:25:30, sky wrote:
> Ouch. Is the old one dumb?

No comments :)

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore_browsertest.cc:1432: const int NumTabs =
6;
On 2015/05/12 21:25:30, sky wrote:
> num_tabs

Oops, done. Fixed the other test too.

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore_browsertest.cc:1446:
std::random_shuffle(activation_order.begin(), activation_order.end());
On 2015/05/12 21:25:30, sky wrote:
> It's not a good idea to randomize test data. If there is flake then it can be
> hard to repro. Much better if you order it in a specific way. Do multiple runs
> of the test if appropriate.

Acknowledged. Changed to a fixed order.

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore_browsertest.cc:1459: for (auto i :
activation_order) {
On 2015/05/12 21:25:30, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
File chrome/browser/sessions/session_restore_delegate.h (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore_delegate.h:26: base::TimeTicks
last_activation_time);
On 2015/05/12 21:25:30, sky wrote:
> If you push this to the WebContents, do you need it here too?

Good catch.

Pushing to WebContents was added at the very end when we realized that
successive restores would not work, so this was needed before that.

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_service.cc:496: ScheduleCommand(
On 2015/05/12 21:25:30, sky wrote:
> Don't you want to gate this by SessionRestore::GetSmartRestoreMode() ==
> SessionRestore::SMART_RESTORE_MODE_MRU) ? Otherwise you can't really compare
> metrics relating to file information.

Good idea. I'm actually going to gate the call to this function instead, to
avoid creating the ID (see your other comment in browser.cc).

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_service.cc:740: // This works because we are
persisting the last activation when restoring
On 2015/05/12 21:25:30, sky wrote:
> I don't understand this comment. It likely can be nuked.

Removed.

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:1084: session_service->SetActivationTime(
On 2015/05/12 21:25:30, sky wrote:
> I think you should only do this if the tab is active for long enough. For
> example, if I rapidly control-tab between a bunch of tabs there is no point in
> generating an id for each. If you didn't do this, then you could add the
> activation time to SetSelectedTabInWindow.

The length if the activation is something that's going to be tackled in a
subsequent CL. For now, we're going with the simple method of recording all tab
switching but this will be improved soon.

Putting this in the experiment, as per previous comment.

https://codereview.chromium.org/1131373003/diff/100001/components/sessions/ba...
File components/sessions/base_session_service_commands.h (right):

https://codereview.chromium.org/1131373003/diff/100001/components/sessions/ba...
components/sessions/base_session_service_commands.h:47: SESSIONS_EXPORT
scoped_ptr<SessionCommand> CreateLastActiveTimeCommand(
On 2015/05/12 21:25:30, sky wrote:
> Is this used?

Not anymore, removed.

[jam, 2015-05-18 15:31:38]

content lgtm with change

https://codereview.chromium.org/1131373003/diff/210001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1131373003/diff/210001/content/public/browser...
content/public/browser/web_contents.h:355: // was created or shown with
WasShown()). If the tab has been restored from a
undo this comment change. content doesn't know about session restore, as that's
a chrome feature

[sky, 2015-05-18 15:53:29]

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore.cc:600:
web_contents->SetLastActiveTime(tab.last_activation_time);
On 2015/05/15 16:55:30, Georges Khalil wrote:
> On 2015/05/12 21:25:29, sky wrote:
> > It's my understanding there is no guarantee that timeticks increases across
a
> > reboot. I suspect if you have your machine running for a long time, reboot,
> then
> > tab.last_activation_time could be very far in the future.
> 
> You are right, if the OS reboots, TimeTicks become irrelevant. Landed another
CL
> that changes WebContents::last_active_time to Time instead of TimeTicks. This
> should fix it as we're using Time now. I also changed last_activation_time to
> last_active_time for consistency with WebContents.

I think time ticks is safer as blocks can go back. You just need to sanitize the
time ticks value when restoring.

https://codereview.chromium.org/1131373003/diff/210001/chrome/browser/session...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/1131373003/diff/210001/chrome/browser/session...
chrome/browser/sessions/session_restore.cc:584: WebContents* web_contents =
chrome::AddRestoredTab(
Might this set last active time if the tab is selected?

https://codereview.chromium.org/1131373003/diff/210001/chrome/browser/session...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/1131373003/diff/210001/chrome/browser/session...
chrome/browser/sessions/session_service.cc:740:
base_session_service_->AppendRebuildCommand(
Only if SessionRestore::SMART_RESTORE_MODE_MRU?

[Georges Khalil, 2015-05-19 17:53:22]

Patchset #6 (id:230001) has been deleted

[Georges Khalil, 2015-05-19 18:22:58]

Patchset #7 (id:270001) has been deleted

[Georges Khalil, 2015-05-19 19:32:16]

sky@, PTAnL.

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore.cc:600:
web_contents->SetLastActiveTime(tab.last_activation_time);
On 2015/05/18 15:53:28, sky wrote:
> On 2015/05/15 16:55:30, Georges Khalil wrote:
> > On 2015/05/12 21:25:29, sky wrote:
> > > It's my understanding there is no guarantee that timeticks increases
across
> a
> > > reboot. I suspect if you have your machine running for a long time,
reboot,
> > then
> > > tab.last_activation_time could be very far in the future.
> > 
> > You are right, if the OS reboots, TimeTicks become irrelevant. Landed
another
> CL
> > that changes WebContents::last_active_time to Time instead of TimeTicks.
This
> > should fix it as we're using Time now. I also changed last_activation_time
to
> > last_active_time for consistency with WebContents.
> 
> I think time ticks is safer as blocks can go back. You just need to sanitize
the
> time ticks value when restoring.

I'm not sure how we could sanitize time ticks, as they're relative to the OS
session. The fact that the clock can change (daylight, for example), is not that
big of a deal, as we will still be able to order them correctly. And worst case,
the order won't be perfect, which is no worse than the original way of doing
things.

If you have a good way of sanitizing time ticks, that would be great (I'd have
to revert my TimeTicks==>Time patch, but not a big deal :) )

https://codereview.chromium.org/1131373003/diff/210001/chrome/browser/session...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/1131373003/diff/210001/chrome/browser/session...
chrome/browser/sessions/session_restore.cc:584: WebContents* web_contents =
chrome::AddRestoredTab(
On 2015/05/18 15:53:28, sky wrote:
> Might this set last active time if the tab is selected?

True, if the tab is selected, we do not want to override the last active time.
Added a check at line 598. Also added a check to make sure time is not in the
future.

https://codereview.chromium.org/1131373003/diff/210001/chrome/browser/session...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/1131373003/diff/210001/chrome/browser/session...
chrome/browser/sessions/session_service.cc:740:
base_session_service_->AppendRebuildCommand(
On 2015/05/18 15:53:28, sky wrote:
> Only if SessionRestore::SMART_RESTORE_MODE_MRU?

Done.

https://codereview.chromium.org/1131373003/diff/210001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1131373003/diff/210001/content/public/browser...
content/public/browser/web_contents.h:355: // was created or shown with
WasShown()). If the tab has been restored from a
On 2015/05/18 15:31:37, jam wrote:
> undo this comment change. content doesn't know about session restore, as
that's
> a chrome feature

Done.

[sky, 2015-05-19 19:55:16]

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore.cc:600:
web_contents->SetLastActiveTime(tab.last_activation_time);
On 2015/05/19 19:32:15, Georges Khalil wrote:
> On 2015/05/18 15:53:28, sky wrote:
> > On 2015/05/15 16:55:30, Georges Khalil wrote:
> > > On 2015/05/12 21:25:29, sky wrote:
> > > > It's my understanding there is no guarantee that timeticks increases
> across
> > a
> > > > reboot. I suspect if you have your machine running for a long time,
> reboot,
> > > then
> > > > tab.last_activation_time could be very far in the future.
> > > 
> > > You are right, if the OS reboots, TimeTicks become irrelevant. Landed
> another
> > CL
> > > that changes WebContents::last_active_time to Time instead of TimeTicks.
> This
> > > should fix it as we're using Time now. I also changed last_activation_time
> to
> > > last_active_time for consistency with WebContents.
> > 
> > I think time ticks is safer as blocks can go back. You just need to sanitize
> the
> > time ticks value when restoring.
> 
> I'm not sure how we could sanitize time ticks, as they're relative to the OS
> session. The fact that the clock can change (daylight, for example), is not
that
> big of a deal, as we will still be able to order them correctly. And worst
case,
> the order won't be perfect, which is no worse than the original way of doing
> things.
> 
> If you have a good way of sanitizing time ticks, that would be great (I'd have
> to revert my TimeTicks==>Time patch, but not a big deal :) )

After you've read the data in all we care about is relative order. You could
achieve this by sorting the original time ticks you read in, and assigning new
values based on sort position and current time ticks.

[gab, 2015-05-20 17:08:57]

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/session...
chrome/browser/sessions/session_restore.cc:600:
web_contents->SetLastActiveTime(tab.last_activation_time);
On 2015/05/19 19:55:16, sky wrote:
> On 2015/05/19 19:32:15, Georges Khalil wrote:
> > On 2015/05/18 15:53:28, sky wrote:
> > > On 2015/05/15 16:55:30, Georges Khalil wrote:
> > > > On 2015/05/12 21:25:29, sky wrote:
> > > > > It's my understanding there is no guarantee that timeticks increases
> > across
> > > a
> > > > > reboot. I suspect if you have your machine running for a long time,
> > reboot,
> > > > then
> > > > > tab.last_activation_time could be very far in the future.
> > > > 
> > > > You are right, if the OS reboots, TimeTicks become irrelevant. Landed
> > another
> > > CL
> > > > that changes WebContents::last_active_time to Time instead of TimeTicks.
> > This
> > > > should fix it as we're using Time now. I also changed
last_activation_time
> > to
> > > > last_active_time for consistency with WebContents.
> > > 
> > > I think time ticks is safer as blocks can go back. You just need to
sanitize
> > the
> > > time ticks value when restoring.
> > 
> > I'm not sure how we could sanitize time ticks, as they're relative to the OS
> > session. The fact that the clock can change (daylight, for example), is not
> that
> > big of a deal, as we will still be able to order them correctly. And worst
> case,
> > the order won't be perfect, which is no worse than the original way of doing
> > things.
> > 
> > If you have a good way of sanitizing time ticks, that would be great (I'd
have
> > to revert my TimeTicks==>Time patch, but not a big deal :) )
> 
> After you've read the data in all we care about is relative order. 

I'm not convinced that this is true. For example, we could decide to stop
loading any tab whose latest timestamp is X days/hours behind the latest
shutdown.

I also agree with Georges that small order flips in tabs that have almost the
same timestamp but for which the clock has somehow jumped slightly backward and
introduced an ordering flip are mostly irrelevant.

> You could achieve this by sorting the original time ticks you read in, and
assigning new
> values based on sort position and current time ticks.

[Georges Khalil, 2015-05-21 20:38:35]

Patchset #8 (id:310001) has been deleted

[Georges Khalil, 2015-05-21 20:42:04]

sky@, PTAnL.

- Reverted back to using TimeTicks
- Sanitized Timeticks when restoring, to preserve correct ordering and time
differences
- Increased coverage of browser test by also verifying the sanitization and
activating a tab between the two restores

[sky, 2015-05-22 03:00:19]

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:1084: session_service->SetActivationTime(
On 2015/05/15 16:55:30, Georges Khalil wrote:
> On 2015/05/12 21:25:30, sky wrote:
> > I think you should only do this if the tab is active for long enough. For
> > example, if I rapidly control-tab between a bunch of tabs there is no point
in
> > generating an id for each. If you didn't do this, then you could add the
> > activation time to SetSelectedTabInWindow.
> 
> The length if the activation is something that's going to be tackled in a
> subsequent CL. For now, we're going with the simple method of recording all
tab
> switching but this will be improved soon.
> 
> Putting this in the experiment, as per previous comment.

Can you outline where you're thinking you'll go with that? If we're always going
to update active time you could make SetSelectedTabInWindow take the time stamp
and not create another command.

https://codereview.chromium.org/1131373003/diff/330001/chrome/browser/session...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/1131373003/diff/330001/chrome/browser/session...
chrome/browser/sessions/session_restore.cc:620:
web_contents->SetLastActiveTime(tab.last_active_time);
I don't like pushing potentially bogus times to the webcontents. I would rather
you do the sanitizing first.

https://codereview.chromium.org/1131373003/diff/330001/components/sessions/se...
File components/sessions/session_types.h (right):

https://codereview.chromium.org/1131373003/diff/330001/components/sessions/se...
components/sessions/session_types.h:100: // Timestamp for when this tab was last
activated.
Add a comment about being careful as value is only useful when comparing against
other last_active_times.

[Georges Khalil, 2015-05-22 20:09:47]

Patchset #9 (id:350001) has been deleted

[Georges Khalil, 2015-05-22 20:14:10]

sky@, PTAnL.

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:1084: session_service->SetActivationTime(
On 2015/05/22 03:00:19, sky wrote:
> On 2015/05/15 16:55:30, Georges Khalil wrote:
> > On 2015/05/12 21:25:30, sky wrote:
> > > I think you should only do this if the tab is active for long enough. For
> > > example, if I rapidly control-tab between a bunch of tabs there is no
point
> in
> > > generating an id for each. If you didn't do this, then you could add the
> > > activation time to SetSelectedTabInWindow.
> > 
> > The length if the activation is something that's going to be tackled in a
> > subsequent CL. For now, we're going with the simple method of recording all
> tab
> > switching but this will be improved soon.
> > 
> > Putting this in the experiment, as per previous comment.
> 
> Can you outline where you're thinking you'll go with that? If we're always
going
> to update active time you could make SetSelectedTabInWindow take the time
stamp
> and not create another command.

Forgot to mention that we need to keep them separated because when
BuildCommandsFromBrowsers is called, it will only save one
SetSelectedTabInWindow per window, for the selected tab, whereas we need one per
tab, to save all the last active times.

As for the length of the activation, I haven't looked into it in detail, but my
first instinct would be to start a timer here for new_contents and stop the
timer of old_contents. And whenever the timer fires, enough time has elapsed and
save the time.

WDYT?

https://codereview.chromium.org/1131373003/diff/330001/chrome/browser/session...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/1131373003/diff/330001/chrome/browser/session...
chrome/browser/sessions/session_restore.cc:620:
web_contents->SetLastActiveTime(tab.last_active_time);
On 2015/05/22 03:00:19, sky wrote:
> I don't like pushing potentially bogus times to the webcontents. I would
rather
> you do the sanitizing first.

That was done because otherwise, by the time we sanitized the values, the wrong
ones had already been saved.

I worked around this by sanitizing the values earlier.

https://codereview.chromium.org/1131373003/diff/330001/components/sessions/se...
File components/sessions/session_types.h (right):

https://codereview.chromium.org/1131373003/diff/330001/components/sessions/se...
components/sessions/session_types.h:100: // Timestamp for when this tab was last
activated.
On 2015/05/22 03:00:19, sky wrote:
> Add a comment about being careful as value is only useful when comparing
against
> other last_active_times.

Done.

[sky, 2015-05-22 20:25:30]

LGTM

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1131373003/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:1084: session_service->SetActivationTime(
On 2015/05/22 20:14:09, Georges Khalil wrote:
> On 2015/05/22 03:00:19, sky wrote:
> > On 2015/05/15 16:55:30, Georges Khalil wrote:
> > > On 2015/05/12 21:25:30, sky wrote:
> > > > I think you should only do this if the tab is active for long enough.
For
> > > > example, if I rapidly control-tab between a bunch of tabs there is no
> point
> > in
> > > > generating an id for each. If you didn't do this, then you could add the
> > > > activation time to SetSelectedTabInWindow.
> > > 
> > > The length if the activation is something that's going to be tackled in a
> > > subsequent CL. For now, we're going with the simple method of recording
all
> > tab
> > > switching but this will be improved soon.
> > > 
> > > Putting this in the experiment, as per previous comment.
> > 
> > Can you outline where you're thinking you'll go with that? If we're always
> going
> > to update active time you could make SetSelectedTabInWindow take the time
> stamp
> > and not create another command.
> 
> Forgot to mention that we need to keep them separated because when
> BuildCommandsFromBrowsers is called, it will only save one
> SetSelectedTabInWindow per window, for the selected tab, whereas we need one
per
> tab, to save all the last active times.

We could change session restore to not only keep one around, but that is only
useful it we don't do something more interesting here. Sounds like we are going
to more interesting, which is good.

> As for the length of the activation, I haven't looked into it in detail, but
my
> first instinct would be to start a timer here for new_contents and stop the
> timer of old_contents. And whenever the timer fires, enough time has elapsed
and
> save the time.

I think you'll end up needing to track this in sessionrestore too (or perhaps
set some userdata on the webcontents), but you can worry about that later.

[Georges Khalil, 2015-05-22 20:49:38]

The CQ bit was checked by georgesak@chromium.org

[Georges Khalil, 2015-05-22 20:49:39]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1131373003/#ps370001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2015-05-22 20:51:53]

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

[commit-bot: I haz the power, 2015-05-22 22:08:21]

Committed patchset #9 (id:370001)

[commit-bot: I haz the power, 2015-05-22 22:09:32]

Patchset 9 (id:??) landed as
https://crrev.com/5582cbe405f6dc48b14e2cae7254aa03abcf353b
Cr-Commit-Position: refs/heads/master@{#331189}