Split NTP_TILE_LOADED event.

Split NTP_TILE_LOADED event.

Multi-iframe NTP continues to use it, but no longer logs statistics for
load time and number of tiles. This happens only for external NTPs,
where the load time and number of tiles are not fully under our control
anyway.

Single-iframe NTP now uses NTP_ALL_TILES_LOADED. When this is logged, we
emit statistics, ensuring that we log a load time and number of tiles
exactly once per NTP (well, unless the user closes the page or navigates
away without the page having finished loading).

BUG=625990
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65
Cr-Commit-Position: refs/heads/master@{#405087}

[sfiera, 2016-07-12 11:31:59]

Description was changed from

==========
Split NTP_TILE_LOADED event.

Multi-iframe NTP continues to use it, but no longer logs statistics for
load time and number of tiles. This happens only for external NTPs,
where the load time and number of tiles are not fully under our control
anyway.

Single-iframe NTP now uses NTP_ALL_TILES_LOADED. When this is logged, we
emit statistics, ensuring that we log a load time and number of tiles
exactly once per NTP (well, unless the user closes the page or navigates
away without the page having finished loading).

BUG=625990
==========

to

==========
Split NTP_TILE_LOADED event.

Multi-iframe NTP continues to use it, but no longer logs statistics for
load time and number of tiles. This happens only for external NTPs,
where the load time and number of tiles are not fully under our control
anyway.

Single-iframe NTP now uses NTP_ALL_TILES_LOADED. When this is logged, we
emit statistics, ensuring that we log a load time and number of tiles
exactly once per NTP (well, unless the user closes the page or navigates
away without the page having finished loading).

BUG=625990
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[sfiera, 2016-07-12 11:32:18]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 11:32:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 11:44:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-12 11:44:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sfiera, 2016-07-12 11:51:17]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 11:51:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sfiera, 2016-07-12 12:20:56]

sfiera@chromium.org changed reviewers:
+ treib@chromium.org

[commit-bot: I haz the power, 2016-07-12 12:23:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-12 12:23:24]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sfiera, 2016-07-12 12:38:13]

Removed the two failing tests--the thing that they test (SearchTabHelper
notifies NTPUserDataLogger directly, without an intervening NTP) is no longer
supposed to happen.

[Marc Treib, 2016-07-12 12:40:09]

Yay!

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/local_ntp/most_visited_single.js (right):

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/local_ntp/most_visited_single.js:26: // A NTP Tile has
finished loading (successfully or failing).
Please update the comment.

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:109: case NTP_TILE:
Unrelated to this CL, but: Can we get rid of NTP_TILE and instead increment
number_of_tiles_ on NTP_SERVER_SIDE_SUGGESTION and NTP_CLIENT_SIDE_SUGGESTION?

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:116: load_time_ = time;
Might as well get rid of the load_time_ member now and instead just pass it into
EmitNtpStatistics - WDYT?

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:192: if (has_emitted_ ||
!number_of_tiles_)
Can we get rid of the !number_of_tiles_ check now? I don't see what that serves.
(I'm guessing it was needed as a hack before, to avoid emitting before anything
was actually loaded)

Also, do we want to keep has_emitted_? Should be safe to get rid of it now, no?

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.h (right):

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:14: #include
"content/public/browser/web_contents_observer.h"
not needed anymore

[sfiera, 2016-07-12 13:21:21]

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/local_ntp/most_visited_single.js (right):

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/local_ntp/most_visited_single.js:26: // A NTP Tile has
finished loading (successfully or failing).
On 2016/07/12 12:40:09, Marc Treib wrote:
> Please update the comment.

Done.

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:109: case NTP_TILE:
On 2016/07/12 12:40:09, Marc Treib wrote:
> Unrelated to this CL, but: Can we get rid of NTP_TILE and instead increment
> number_of_tiles_ on NTP_SERVER_SIDE_SUGGESTION and NTP_CLIENT_SIDE_SUGGESTION?

I think so.

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:116: load_time_ = time;
On 2016/07/12 12:40:09, Marc Treib wrote:
> Might as well get rid of the load_time_ member now and instead just pass it
into
> EmitNtpStatistics - WDYT?

Done.

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:192: if (has_emitted_ ||
!number_of_tiles_)
On 2016/07/12 12:40:09, Marc Treib wrote:
> Can we get rid of the !number_of_tiles_ check now? I don't see what that
serves.
> (I'm guessing it was needed as a hack before, to avoid emitting before
anything
> was actually loaded)

I think it was a hack and is now unneeded. Removed.

> Also, do we want to keep has_emitted_? Should be safe to get rid of it now,
no?

I don't think it's safe. The single-iframe NTP could get tiles multiple times,
and logs NTP_ALL_TILES_LOADED each time, with increasing load times. And in the
case where there's an in-page reload, we don't really care about load times or
number of tiles anymore, do we?

On the other hand, in the other CL, we can safely reset it with everything else
when the user goes back to the NTP.

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.h (right):

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:14: #include
"content/public/browser/web_contents_observer.h"
On 2016/07/12 12:40:09, Marc Treib wrote:
> not needed anymore

Done.

[sfiera, 2016-07-12 13:21:28]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 13:21:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marc Treib, 2016-07-12 13:39:01]

lgtm

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:109: case NTP_TILE:
On 2016/07/12 13:21:21, sfiera wrote:
> On 2016/07/12 12:40:09, Marc Treib wrote:
> > Unrelated to this CL, but: Can we get rid of NTP_TILE and instead increment
> > number_of_tiles_ on NTP_SERVER_SIDE_SUGGESTION and
NTP_CLIENT_SIDE_SUGGESTION?
> 
> I think so.

Maybe add a TODO for this?

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:192: if (has_emitted_ ||
!number_of_tiles_)
On 2016/07/12 13:21:21, sfiera wrote:
> On 2016/07/12 12:40:09, Marc Treib wrote:
> > Can we get rid of the !number_of_tiles_ check now? I don't see what that
> serves.
> > (I'm guessing it was needed as a hack before, to avoid emitting before
> anything
> > was actually loaded)
> 
> I think it was a hack and is now unneeded. Removed.
> 
> > Also, do we want to keep has_emitted_? Should be safe to get rid of it now,
> no?
> 
> I don't think it's safe. The single-iframe NTP could get tiles multiple times,
> and logs NTP_ALL_TILES_LOADED each time, with increasing load times. And in
the
> case where there's an in-page reload, we don't really care about load times or
> number of tiles anymore, do we?
> 
> On the other hand, in the other CL, we can safely reset it with everything
else
> when the user goes back to the NTP.

Yep, the "going back to NTP" case is what I was thinking about. Let's do that in
the other CL then.

https://codereview.chromium.org/2141933002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/2141933002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:194: // LoadTime only gets
update once per page, so we don't have it on reloads.
nit: Comment is out of date now

[sfiera, 2016-07-12 14:10:25]

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/2141933002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:109: case NTP_TILE:
On 2016/07/12 13:39:01, Marc Treib wrote:
> On 2016/07/12 13:21:21, sfiera wrote:
> > On 2016/07/12 12:40:09, Marc Treib wrote:
> > > Unrelated to this CL, but: Can we get rid of NTP_TILE and instead
increment
> > > number_of_tiles_ on NTP_SERVER_SIDE_SUGGESTION and
> NTP_CLIENT_SIDE_SUGGESTION?
> > 
> > I think so.
> 
> Maybe add a TODO for this?

Done.

https://codereview.chromium.org/2141933002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/2141933002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:194: // LoadTime only gets
update once per page, so we don't have it on reloads.
On 2016/07/12 13:39:01, Marc Treib wrote:
> nit: Comment is out of date now

Removed comment and the conditional too--we should always get a load time > 0,
as far as I understand it.

Will kick off a CQ dry run, holler if you think the conditional should stay.

[sfiera, 2016-07-12 14:11:38]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 14:12:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 18:19:17]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 18:19:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sfiera, 2016-07-13 08:10:00]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2016-07-13 08:10:00]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2141933002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-07-13 08:10:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 08:44:14]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-13 08:44:35]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 08:47:19]

Description was changed from

==========
Split NTP_TILE_LOADED event.

Multi-iframe NTP continues to use it, but no longer logs statistics for
load time and number of tiles. This happens only for external NTPs,
where the load time and number of tiles are not fully under our control
anyway.

Single-iframe NTP now uses NTP_ALL_TILES_LOADED. When this is logged, we
emit statistics, ensuring that we log a load time and number of tiles
exactly once per NTP (well, unless the user closes the page or navigates
away without the page having finished loading).

BUG=625990
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Split NTP_TILE_LOADED event.

Multi-iframe NTP continues to use it, but no longer logs statistics for
load time and number of tiles. This happens only for external NTPs,
where the load time and number of tiles are not fully under our control
anyway.

Single-iframe NTP now uses NTP_ALL_TILES_LOADED. When this is logged, we
emit statistics, ensuring that we log a load time and number of tiles
exactly once per NTP (well, unless the user closes the page or navigates
away without the page having finished loading).

BUG=625990
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65
Cr-Commit-Position: refs/heads/master@{#405087}
==========

[commit-bot: I haz the power, 2016-07-13 08:47:20]

Patchset 6 (id:??) landed as
https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65
Cr-Commit-Position: refs/heads/master@{#405087}

[sfiera, 2016-07-13 13:33:14]

This broke NewTabPage.LoadTime.Startup. I guess the old code caused the
NTPUserDataLogger to exist earlier, but now it's not created until the the NTP
JS is already loaded and running, which is considered after startup.

I don't know how to preserve the metric with this situation.

[Marc Treib, 2016-07-13 13:43:38]

On 2016/07/13 13:33:14, sfiera wrote:
> This broke NewTabPage.LoadTime.Startup. I guess the old code caused the
> NTPUserDataLogger to exist earlier, but now it's not created until the the NTP
> JS is already loaded and running, which is considered after startup.
> 
> I don't know how to preserve the metric with this situation.

Hm, so we really need the NTPUserDataLogger instance to be created as soon as
the tab is opened. Looks like that would only have worked by accident before...
out of curiosity, do you know which of the GetOrCreateFromWebContents calls it
was that created the thing?

Anyway, can't we just create the thing from, say,
SearchTabHelper::OnTabActivated?

[sfiera, 2016-07-13 13:49:20]

On 2016/07/13 13:43:38, Marc Treib wrote:
> On 2016/07/13 13:33:14, sfiera wrote:
> > This broke NewTabPage.LoadTime.Startup. I guess the old code caused the
> > NTPUserDataLogger to exist earlier, but now it's not created until the the
NTP
> > JS is already loaded and running, which is considered after startup.
> > 
> > I don't know how to preserve the metric with this situation.
> 
> Hm, so we really need the NTPUserDataLogger instance to be created as soon as
> the tab is opened. Looks like that would only have worked by accident
before...
> out of curiosity, do you know which of the GetOrCreateFromWebContents calls it
> was that created the thing?

I think it would have to have been the most-visited-items-changed hook (the only
other external one being tab-deactivated).

> Anyway, can't we just create the thing from, say,
> SearchTabHelper::OnTabActivated?

I'll give it a try. That would have the side-effect of creating it on tabs that
will never have an NTP (URL opened in new tab), which eats just a little more
memory...

[Marc Treib, 2016-07-13 14:00:30]

On 2016/07/13 13:49:20, sfiera wrote:
> On 2016/07/13 13:43:38, Marc Treib wrote:
> > On 2016/07/13 13:33:14, sfiera wrote:
> > > This broke NewTabPage.LoadTime.Startup. I guess the old code caused the
> > > NTPUserDataLogger to exist earlier, but now it's not created until the the
> NTP
> > > JS is already loaded and running, which is considered after startup.
> > > 
> > > I don't know how to preserve the metric with this situation.
> > 
> > Hm, so we really need the NTPUserDataLogger instance to be created as soon
as
> > the tab is opened. Looks like that would only have worked by accident
> before...
> > out of curiosity, do you know which of the GetOrCreateFromWebContents calls
it
> > was that created the thing?
> 
> I think it would have to have been the most-visited-items-changed hook (the
only
> other external one being tab-deactivated).
> 
> > Anyway, can't we just create the thing from, say,
> > SearchTabHelper::OnTabActivated?
> 
> I'll give it a try. That would have the side-effect of creating it on tabs
that
> will never have an NTP (URL opened in new tab), which eats just a little more
> memory...

Also, NTPUserDataLogger currently still relies on only being called for actual
NTPs, right?

[sfiera, 2016-07-13 14:23:16]

On 2016/07/13 14:00:30, Marc Treib wrote:
> On 2016/07/13 13:49:20, sfiera wrote:
> > On 2016/07/13 13:43:38, Marc Treib wrote:
> > > On 2016/07/13 13:33:14, sfiera wrote:
> > > > This broke NewTabPage.LoadTime.Startup. I guess the old code caused the
> > > > NTPUserDataLogger to exist earlier, but now it's not created until the
the
> > NTP
> > > > JS is already loaded and running, which is considered after startup.
> > > > 
> > > > I don't know how to preserve the metric with this situation.
> > > 
> > > Hm, so we really need the NTPUserDataLogger instance to be created as soon
> as
> > > the tab is opened. Looks like that would only have worked by accident
> > before...
> > > out of curiosity, do you know which of the GetOrCreateFromWebContents
calls
> it
> > > was that created the thing?
> > 
> > I think it would have to have been the most-visited-items-changed hook (the
> only
> > other external one being tab-deactivated).
> > 
> > > Anyway, can't we just create the thing from, say,
> > > SearchTabHelper::OnTabActivated?
> > 
> > I'll give it a try. That would have the side-effect of creating it on tabs
> that
> > will never have an NTP (URL opened in new tab), which eats just a little
more
> > memory...
> 
> Also, NTPUserDataLogger currently still relies on only being called for actual
> NTPs, right?

Hmm. There will still be things that "shouldn't happen" if it is called for a
non-NTP. For example, we'll pick up the initial page as "ntp_url_" and reset our
stats whenever the user navigates back to that page. On the other hand, nothing
outside the NTP JS is able to trigger those stats to be logged anymore, so none
of the bad behavior will be observable.

Somehow, we must know whether a tab is being created as an NTP tab or not,
right?

[Marc Treib, 2016-07-13 14:28:09]

On 2016/07/13 14:23:16, sfiera wrote:
> On 2016/07/13 14:00:30, Marc Treib wrote:
> > On 2016/07/13 13:49:20, sfiera wrote:
> > > On 2016/07/13 13:43:38, Marc Treib wrote:
> > > > On 2016/07/13 13:33:14, sfiera wrote:
> > > > > This broke NewTabPage.LoadTime.Startup. I guess the old code caused
the
> > > > > NTPUserDataLogger to exist earlier, but now it's not created until the
> the
> > > NTP
> > > > > JS is already loaded and running, which is considered after startup.
> > > > > 
> > > > > I don't know how to preserve the metric with this situation.
> > > > 
> > > > Hm, so we really need the NTPUserDataLogger instance to be created as
soon
> > as
> > > > the tab is opened. Looks like that would only have worked by accident
> > > before...
> > > > out of curiosity, do you know which of the GetOrCreateFromWebContents
> calls
> > it
> > > > was that created the thing?
> > > 
> > > I think it would have to have been the most-visited-items-changed hook
(the
> > only
> > > other external one being tab-deactivated).
> > > 
> > > > Anyway, can't we just create the thing from, say,
> > > > SearchTabHelper::OnTabActivated?
> > > 
> > > I'll give it a try. That would have the side-effect of creating it on tabs
> > that
> > > will never have an NTP (URL opened in new tab), which eats just a little
> more
> > > memory...
> > 
> > Also, NTPUserDataLogger currently still relies on only being called for
actual
> > NTPs, right?
> 
> Hmm. There will still be things that "shouldn't happen" if it is called for a
> non-NTP. For example, we'll pick up the initial page as "ntp_url_" and reset
our
> stats whenever the user navigates back to that page. On the other hand,
nothing
> outside the NTP JS is able to trigger those stats to be logged anymore, so
none
> of the bad behavior will be observable.
> 
> Somehow, we must know whether a tab is being created as an NTP tab or not,
> right?

Check the URL, I guess (if any of the "IsNTP" functions actually does what we
need)? I vaguely remember seeing code somewhere that essentially did
OpenTab("chrome://newtab"), so I don't think there's anything much nicer.