Record the number of tabs in a session restore in SampledProfile

Record the number of tabs in a session restore in SampledProfile

The OnSessionRestore callback has been changed to accept the number
of tabs as an argument.

BUG=chromium:455482
TEST=Add logging to make sure the correct number of tabs is being
passed to perf provider during session restore

Signed-off-by: Simon Que <sque@chromium.org>;

Committed: https://crrev.com/ebb7934046dcb4161d32ee182e02e1d9b570dfc5
Cr-Commit-Position: refs/heads/master@{#318168}

[Simon Que, 2015-02-04 18:40:02]

sque@chromium.org changed reviewers:
+ isherman@chromium.org, rapati@chromium.org, sky@chromium.org

[sky, 2015-02-04 21:22:12]

https://codereview.chromium.org/902613003/diff/1/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main_posix.cc (right):

https://codereview.chromium.org/902613003/diff/1/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main_posix.cc:99: void OnSessionRestoreDone(int);
declaration and definition should both have names in them. It's ok to comment
out unused params, but you should have the name here.

https://codereview.chromium.org/902613003/diff/1/chrome/browser/extensions/st...
File chrome/browser/extensions/state_store_notification_observer.h (right):

https://codereview.chromium.org/902613003/diff/1/chrome/browser/extensions/st...
chrome/browser/extensions/state_store_notification_observer.h:25: void
OnSessionRestoreDone(int num_tabs);
Be consistent. num_tabs or num_tabs_restored are fine, but use the same name
everywhere.

https://codereview.chromium.org/902613003/diff/1/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_restore.h (right):

https://codereview.chromium.org/902613003/diff/1/chrome/browser/sessions/sess...
chrome/browser/sessions/session_restore.h:95: // in
on_session_restored_callbacks_.
You need to document what the int means.

[Ilya Sherman, 2015-02-04 22:50:53]

Please file a new bug for this work.  Could you please clarify why this metric
is useful for profiling?

https://codereview.chromium.org/902613003/diff/1/chrome/browser/metrics/perf_...
File chrome/browser/metrics/perf_provider_chromeos.h (right):

https://codereview.chromium.org/902613003/diff/1/chrome/browser/metrics/perf_...
chrome/browser/metrics/perf_provider_chromeos.h:64: void
OnSessionRestoreDone(int num_tabs_restored);
nit: size_t?

[Simon Que, 2015-02-05 00:16:01]

https://codereview.chromium.org/902613003/diff/1/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main_posix.cc (right):

https://codereview.chromium.org/902613003/diff/1/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main_posix.cc:99: void OnSessionRestoreDone(int);
On 2015/02/04 21:22:11, sky wrote:
> declaration and definition should both have names in them. It's ok to comment
> out unused params, but you should have the name here.

Done.

https://codereview.chromium.org/902613003/diff/1/chrome/browser/extensions/st...
File chrome/browser/extensions/state_store_notification_observer.h (right):

https://codereview.chromium.org/902613003/diff/1/chrome/browser/extensions/st...
chrome/browser/extensions/state_store_notification_observer.h:25: void
OnSessionRestoreDone(int num_tabs);
On 2015/02/04 21:22:11, sky wrote:
> Be consistent. num_tabs or num_tabs_restored are fine, but use the same name
> everywhere.

Done.

https://codereview.chromium.org/902613003/diff/1/chrome/browser/metrics/perf_...
File chrome/browser/metrics/perf_provider_chromeos.h (right):

https://codereview.chromium.org/902613003/diff/1/chrome/browser/metrics/perf_...
chrome/browser/metrics/perf_provider_chromeos.h:64: void
OnSessionRestoreDone(int num_tabs_restored);
On 2015/02/04 22:50:53, Ilya Sherman wrote:
> nit: size_t?

It's an int32 in the protobuf, so we should just use an int:
https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...

https://codereview.chromium.org/902613003/diff/1/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_restore.h (right):

https://codereview.chromium.org/902613003/diff/1/chrome/browser/sessions/sess...
chrome/browser/sessions/session_restore.h:95: // in
on_session_restored_callbacks_.
On 2015/02/04 21:22:11, sky wrote:
> You need to document what the int means.

Done.

[Simon Que, 2015-02-05 00:19:46]

On 2015/02/04 22:50:53, Ilya Sherman wrote:
> Please file a new bug for this work.  Could you please clarify why this metric
> is useful for profiling?

This was intended to be part of bug 358778, but it was blocked by the need to
remove the use of notification service for session restore done.

[Ilya Sherman, 2015-02-05 00:29:56]

On 2015/02/05 00:19:46, Simon Que wrote:
> On 2015/02/04 22:50:53, Ilya Sherman wrote:
> > Please file a new bug for this work.  Could you please clarify why this
metric
> > is useful for profiling?
> 
> This was intended to be part of bug 358778, but it was blocked by the need to
> remove the use of notification service for session restore done.

Bug 358778 has been marked as fixed for over six months now.  Please file a new
bug for the follow-up work.  Also, you did not answer my question: What is the
utility of this extra field?

[Simon Que, 2015-02-05 00:51:53]

On 2015/02/05 00:29:56, Ilya Sherman wrote:
> On 2015/02/05 00:19:46, Simon Que wrote:
> > On 2015/02/04 22:50:53, Ilya Sherman wrote:
> > > Please file a new bug for this work.  Could you please clarify why this
> metric
> > > is useful for profiling?
> > 
> > This was intended to be part of bug 358778, but it was blocked by the need
to
> > remove the use of notification service for session restore done.
> 
> Bug 358778 has been marked as fixed for over six months now.  Please file a
new
> bug for the follow-up work.  Also, you did not answer my question: What is the
> utility of this extra field?

As we are collecting perf profiles during session restores, we'd like to know
how many tabs are being restored. Restoring 10 tabs will take up much more
overhead than restoring 1 tab. This would allow us to filter profiles in the CWP
dashboard by the number of tabs.

[Ilya Sherman, 2015-02-05 01:49:04]

On 2015/02/05 00:51:53, Simon Que wrote:
> On 2015/02/05 00:29:56, Ilya Sherman wrote:
> > On 2015/02/05 00:19:46, Simon Que wrote:
> > > On 2015/02/04 22:50:53, Ilya Sherman wrote:
> > > > Please file a new bug for this work.  Could you please clarify why this
> > metric
> > > > is useful for profiling?
> > > 
> > > This was intended to be part of bug 358778, but it was blocked by the need
> to
> > > remove the use of notification service for session restore done.
> > 
> > Bug 358778 has been marked as fixed for over six months now.  Please file a
> new
> > bug for the follow-up work.  Also, you did not answer my question: What is
the
> > utility of this extra field?
> 
> As we are collecting perf profiles during session restores, we'd like to know
> how many tabs are being restored. Restoring 10 tabs will take up much more
> overhead than restoring 1 tab. This would allow us to filter profiles in the
CWP
> dashboard by the number of tabs.

Hmm.  Seems like *which* sites are being loaded would affect almost as much as
how many sites are being loaded, but okay.  Please update the bug with the
motivation.  With that, LGTM.

[Simon Que, 2015-02-05 01:56:21]

On 2015/02/05 01:49:04, Ilya Sherman wrote:
> On 2015/02/05 00:51:53, Simon Que wrote:
> > On 2015/02/05 00:29:56, Ilya Sherman wrote:
> > > On 2015/02/05 00:19:46, Simon Que wrote:
> > > > On 2015/02/04 22:50:53, Ilya Sherman wrote:
> > > > > Please file a new bug for this work.  Could you please clarify why
this
> > > metric
> > > > > is useful for profiling?
> > > > 
> > > > This was intended to be part of bug 358778, but it was blocked by the
need
> > to
> > > > remove the use of notification service for session restore done.
> > > 
> > > Bug 358778 has been marked as fixed for over six months now.  Please file
a
> > new
> > > bug for the follow-up work.  Also, you did not answer my question: What is
> the
> > > utility of this extra field?
> > 
> > As we are collecting perf profiles during session restores, we'd like to
know
> > how many tabs are being restored. Restoring 10 tabs will take up much more
> > overhead than restoring 1 tab. This would allow us to filter profiles in the
> CWP
> > dashboard by the number of tabs.
> 
> Hmm.  Seems like *which* sites are being loaded would affect almost as much as
> how many sites are being loaded, but okay.  Please update the bug with the
> motivation.  With that, LGTM.

That'd be cool, but it would have privacy concerns. Perhaps we could have a
domain whitelist or something... we can discuss more over email.

[Simon Que, 2015-02-05 01:56:38]

The CQ bit was checked by sque@chromium.org

[commit-bot: I haz the power, 2015-02-05 01:57:42]

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

[Ilya Sherman, 2015-02-05 01:58:30]

The CQ bit was unchecked by isherman@chromium.org

[Ilya Sherman, 2015-02-05 01:58:40]

On 2015/02/05 01:57:42, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/902613003/20001

Canceled the CQ, because you still need sky's approval.

[sky, 2015-02-05 17:05:19]

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

https://codereview.chromium.org/902613003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/session_restore.h:96: // being restored.
This isn't quite right. You should clearly document the two possible values so
that two years from now when someone comes to this code and tries to add
something they understand what it really is.

[Simon Que, 2015-02-05 19:00:24]

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

https://codereview.chromium.org/902613003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/session_restore.h:96: // being restored.
On 2015/02/05 17:05:19, sky wrote:
> This isn't quite right. You should clearly document the two possible values so
> that two years from now when someone comes to this code and tries to add
> something they understand what it really is.

I thought I'm passing in the number of tabs in both places where the callback
notification is invoked.

Are you referring to the two different types of notifications here?

[sky, 2015-02-06 00:55:00]

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

https://codereview.chromium.org/902613003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/session_restore.h:96: // being restored.
On 2015/02/05 19:00:24, Simon Que wrote:
> On 2015/02/05 17:05:19, sky wrote:
> > This isn't quite right. You should clearly document the two possible values
so
> > that two years from now when someone comes to this code and tries to add
> > something they understand what it really is.
> 
> I thought I'm passing in the number of tabs in both places where the callback
> notification is invoked.
> 
> Are you referring to the two different types of notifications here?

That is some what true, but there is a big difference between
synchronous/non-synchronous that should be documented.
Here's a shot at what I think we want:
The callback is supplied a tab count. The exact meaning, and timing, depends
upon the restore type:
. SessionRestore::SYNCHRONOUS: the parameter is the number of tabs that were
created. Additionally the callback is invoked immediately after the tabs have
been created. That is, the tabs are not necessarily loading.
. For all other restore types the parameter is the number of tabs that were
restored and is sent after all tabs have started loading. Additionally if a
request to restore tabs comes in while a previous request to restore tabs has
not yet completed (loading tabs is throttled), then the callback is only
notified once both sets of tabs have started loading and with the total number
of tabs for both restores.

[Simon Que, 2015-02-11 00:05:28]

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

https://codereview.chromium.org/902613003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/session_restore.h:96: // being restored.
Can we talk about this over video hangout tomorrow? I'll set up a half hour slot
after lunch.

[Simon Que, 2015-02-13 23:23:47]

New patchsets have been uploaded after l-g-t-m from isherman@chromium.org

[sky, 2015-02-25 22:57:45]

LGTM

[Simon Que, 2015-02-25 22:58:52]

The CQ bit was checked by sque@chromium.org

[commit-bot: I haz the power, 2015-02-25 22:59:20]

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

[commit-bot: I haz the power, 2015-02-26 01:22:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-26 01:23:13]

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