Make session restore forced tab load delay Finch configurable, and separate first tab loads from su…

Make session restore forced tab load delay Finch configurable, and separate first tab loads from subsequent.

BUG=472772
Committed: https://crrev.com/9586a8fafe0f8ff5ecaab86691b154e7ebeb1899
Cr-Commit-Position: refs/heads/master@{#330171}

[chrisha, 2015-05-15 15:33:16]

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

[chrisha, 2015-05-15 15:33:17]

Kind of a last minute CL, but would be great to get this in before the beta cut.
We want to explore the 1500ms magic value in the field (as it was chosen
entirely in the lab), and will do so in a subsequent experiment.

Based on lots of discussions there's the thought that having a longer timeout
for visible tabs will be a win for perceived startup time as less contention on
them means faster load and paint times.

PTAL?

[chrisha, 2015-05-15 17:42:25]

chrisha@chromium.org changed reviewers:
+ gab@chromium.org, georgesak@chromium.org

[chrisha, 2015-05-15 17:43:26]

sky@ is OOO today, and the other owner marja@ is gone home for the day. Do you
guys care to take a quick look?

[Georges Khalil, 2015-05-15 17:50:46]

lgtm

[gab, 2015-05-15 18:21:03]

lgtm w/ nits

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
File chrome/browser/sessions/tab_loader.cc (right):

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/tab_loader.cc:156: force_load_timer_.Stop();
Looks like Start() already takes care of stopping a previous timer, so calling
Stop() first seems overkill?

(but I guess it's fine since existing code already does this..)

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
File chrome/browser/sessions/tab_loader.h (right):

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/tab_loader.h:74: void StartFirstTimer();
I think it's more readable if we separate these two and their respective
comments.

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
File chrome/browser/sessions/tab_loader_delegate.cc (right):

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/tab_loader_delegate.cc:70: timeout_ms <= 0)
Multi-line conditional, add {}

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/tab_loader_delegate.cc:78: timeout_ms <= 0)
{}

[Nico, 2015-05-15 18:36:31]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-05-15 18:36:32]

lgtm. I found the comments a bit confusing and it took me a while to understand
what "first" refers to; maybe you can try to make that clearer.

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
File chrome/browser/sessions/tab_loader.h (right):

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/tab_loader.h:74: void StartFirstTimer();
On 2015/05/15 18:21:03, gab wrote:
> I think it's more readable if we separate these two and their respective
> comments.

+1

[chrisha, 2015-05-15 18:47:12]

Thanks, committing. Cleaned up the comments and other nits. I'll ping sky@ about
this next week to double check he's happy.

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
File chrome/browser/sessions/tab_loader.cc (right):

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/tab_loader.cc:156: force_load_timer_.Stop();
On 2015/05/15 18:21:02, gab wrote:
> Looks like Start() already takes care of stopping a previous timer, so calling
> Stop() first seems overkill?
> 
> (but I guess it's fine since existing code already does this..)

Acknowledged.

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
File chrome/browser/sessions/tab_loader.h (right):

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/tab_loader.h:74: void StartFirstTimer();
On 2015/05/15 18:21:03, gab wrote:
> I think it's more readable if we separate these two and their respective
> comments.

Done.

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
File chrome/browser/sessions/tab_loader_delegate.cc (right):

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/tab_loader_delegate.cc:70: timeout_ms <= 0)
On 2015/05/15 18:21:03, gab wrote:
> Multi-line conditional, add {}

Done.

https://codereview.chromium.org/1130673003/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/tab_loader_delegate.cc:78: timeout_ms <= 0)
On 2015/05/15 18:21:03, gab wrote:
> {}

Done.

[chrisha, 2015-05-15 18:48:15]

The CQ bit was checked by chrisha@chromium.org

[chrisha, 2015-05-15 18:48:15]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
georgesak@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1130673003/#ps60001
(title: "Cleaned up comments.")

[commit-bot: I haz the power, 2015-05-15 18:50:16]

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

[commit-bot: I haz the power, 2015-05-15 19:43:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-15 19:44:18]

Patchset 4 (id:??) landed as
https://crrev.com/9586a8fafe0f8ff5ecaab86691b154e7ebeb1899
Cr-Commit-Position: refs/heads/master@{#330171}