Adds a new UMA histogram to measure the NTP load time since navigation

Adds a new UMA histogram to measure the NTP load time since navigation
start.

It also fixes a bug where the NTP will Emit multiple times for
the same page due to OnMostVisitedChange.

BUG=467633
Committed: https://crrev.com/bafbf65d0bb82dd70bfa82bd246e70d7bd5b684f
Cr-Commit-Position: refs/heads/master@{#322851}

[fserb, 2015-03-16 19:34:16]

fserb@chromium.org changed reviewers:
+ dbeam@chromium.org, jwd@chromium.org, kmadhusu@chromium.org,
mathp@chromium.org

[fserb, 2015-03-16 19:34:16]

fserb@chromium.org changed required reviewers:
+ dbeam@chromium.org, jwd@chromium.org, kmadhusu@chromium.org,
mathp@chromium.org

[beaudoin, 2015-03-16 19:39:35]

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

[beaudoin, 2015-03-16 19:39:36]

LGTM with two nits.

https://codereview.chromium.org/1007383003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1007383003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:23065: +
Could you look for "Recorded before changing focus away from the NTP" in that
file and rephrase them to indicate the fact that you're sending them too if the
page sends new suggestions before navigation occurs.

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

https://codereview.chromium.org/1007383003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:179: load_time_ =
std::max(load_time_, time);
Add a comment here to indicate that the time at which the last tile has loaded
is a good proxy for the total load time of the NTP, therefore we keep the max.

[fserb, 2015-03-16 19:47:38]

https://codereview.chromium.org/1007383003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1007383003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:23065: +
On 2015/03/16 19:39:36, beaudoin wrote:
> Could you look for "Recorded before changing focus away from the NTP" in that
> file and rephrase them to indicate the fact that you're sending them too if
the
> page sends new suggestions before navigation occurs. 

Done.

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

https://codereview.chromium.org/1007383003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:179: load_time_ =
std::max(load_time_, time);
On 2015/03/16 19:39:36, beaudoin wrote:
> Add a comment here to indicate that the time at which the last tile has loaded
> is a good proxy for the total load time of the NTP, therefore we keep the max.

Done.

[Mathieu, 2015-03-16 19:49:46]

lgtm

You will have the same problem when resizing the NTP or blacklisting (the tiles
will most probably reload). You should write it down in the bug or create a new
bug.

https://codereview.chromium.org/1007383003/diff/20001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/1007383003/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_tab_helper.cc:405:
InstantTab::EmitNtpStatistics(web_contents_);
Comment why we are emitting statistics at this point.

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

https://codereview.chromium.org/1007383003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:103: // changes tab away
from the NTP. However, if the user comes back to that NTP
amend comment

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

https://codereview.chromium.org/1007383003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:103: // Whether we have
already emitted NTP stats for this web content.
nit: web contents

[fserb, 2015-03-16 20:08:36]

done. thkx.

https://codereview.chromium.org/1007383003/diff/20001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/1007383003/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_tab_helper.cc:405:
InstantTab::EmitNtpStatistics(web_contents_);
On 2015/03/16 19:49:45, Mathieu Perreault wrote:
> Comment why we are emitting statistics at this point.

Done.

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

https://codereview.chromium.org/1007383003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:103: // changes tab away
from the NTP. However, if the user comes back to that NTP
On 2015/03/16 19:49:45, Mathieu Perreault wrote:
> amend comment

Done.

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

https://codereview.chromium.org/1007383003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:103: // Whether we have
already emitted NTP stats for this web content.
On 2015/03/16 19:49:46, Mathieu Perreault wrote:
> nit: web contents

Done.

[kmadhusu, 2015-03-17 00:06:30]

https://codereview.chromium.org/1007383003/diff/120001/chrome/renderer/search...
File chrome/renderer/searchbox/searchbox.cc (left):

https://codereview.chromium.org/1007383003/diff/120001/chrome/renderer/search...
chrome/renderer/searchbox/searchbox.cc:168: ->performance()
Did you rebase this CL against crrev.com/1006753003? Is this change intentional?

[Dan Beam, 2015-03-17 01:18:46]

lgtm but please address kmadhusu@'s questions

https://codereview.chromium.org/1007383003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/local_ntp/most_visited_title.js (right):

https://codereview.chromium.org/1007383003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/local_ntp/most_visited_title.js:23:
NTP_LOGGING_EVENT_TYPE.NTP_TILE_LOADED);
+\s\s

https://codereview.chromium.org/1007383003/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/1007383003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:97: if (has_emitted_) {
no curlies

https://codereview.chromium.org/1007383003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:102: if (!number_of_tiles_)
{
nit: can you combine with if above?

[fserb, 2015-03-17 16:02:17]

Done. Thanks for the comments.

https://codereview.chromium.org/1007383003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/local_ntp/most_visited_title.js (right):

https://codereview.chromium.org/1007383003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/local_ntp/most_visited_title.js:23:
NTP_LOGGING_EVENT_TYPE.NTP_TILE_LOADED);
On 2015/03/17 01:18:46, Dan Beam wrote:
> +\s\s

Done.

https://codereview.chromium.org/1007383003/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/1007383003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:97: if (has_emitted_) {
On 2015/03/17 01:18:46, Dan Beam wrote:
> no curlies

Done.

https://codereview.chromium.org/1007383003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:102: if (!number_of_tiles_)
{
On 2015/03/17 01:18:46, Dan Beam wrote:
> nit: can you combine with if above?

Done.

https://codereview.chromium.org/1007383003/diff/120001/chrome/renderer/search...
File chrome/renderer/searchbox/searchbox.cc (left):

https://codereview.chromium.org/1007383003/diff/120001/chrome/renderer/search...
chrome/renderer/searchbox/searchbox.cc:168: ->performance()
On 2015/03/17 00:06:29, kmadhusu wrote:
> Did you rebase this CL against crrev.com/1006753003? Is this change
intentional?

yeah. It is on top of the other.
I must have forgotten to "cl upload <base>". Just fixed that.
I can only submit this after the 1006753003. I decided to split them to make
them a bit simpler. But not sure if there's a better way to tell codereview
about that.

[kmadhusu, 2015-03-17 16:57:27]

search_tab_helper.cc changes lgtm.

Thanks.

[jwd, 2015-03-17 17:08:12]

https://codereview.chromium.org/1007383003/diff/200001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1007383003/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:23074: +<histogram
name="NewTabPage.LoadTime">
Can you add units="milliseconds" please.

https://codereview.chromium.org/1007383003/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:23077: +    Histogram of the time, in
milliseconds, it took for the NTP to load all its
nit: all of its

[fserb, 2015-03-17 17:10:11]

done. thanks.

https://codereview.chromium.org/1007383003/diff/200001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1007383003/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:23074: +<histogram
name="NewTabPage.LoadTime">
On 2015/03/17 17:08:11, Jesse Doherty wrote:
> Can you add units="milliseconds" please.

Done.

https://codereview.chromium.org/1007383003/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:23077: +    Histogram of the time, in
milliseconds, it took for the NTP to load all its
On 2015/03/17 17:08:11, Jesse Doherty wrote:
> nit: all of its

Done.

[jwd, 2015-03-17 17:11:05]

lgtm

[fserb, 2015-03-19 21:50:55]

The CQ bit was checked by fserb@chromium.org

[fserb, 2015-03-19 21:50:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
beaudoin@chromium.org, mathp@chromium.org, dbeam@chromium.org,
kmadhusu@chromium.org, jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/1007383003/#ps240001
(title: " ")

[commit-bot: I haz the power, 2015-03-19 21:51:31]

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

[fserb, 2015-03-19 21:57:36]

fserb@chromium.org changed reviewers:
+ jhawkins@google.com

[fserb, 2015-03-19 21:57:37]

fserb@chromium.org changed required reviewers:
+ jhawkins@google.com

[fserb, 2015-03-19 21:57:38]

jhawkins: chrome/common/ntp_logging_events.h
thanks :)

[fserb, 2015-03-19 21:58:07]

The CQ bit was unchecked by fserb@chromium.org

[James Hawkins, 2015-03-26 20:47:16]

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

[James Hawkins, 2015-03-26 20:47:17]

lgtm

[fserb, 2015-03-26 21:04:52]

The CQ bit was checked by fserb@chromium.org

[fserb, 2015-03-26 21:04:53]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
mathp@chromium.org, kmadhusu@chromium.org, beaudoin@chromium.org,
jwd@chromium.org, jhawkins@chromium.org
Link to the patchset: https://codereview.chromium.org/1007383003/#ps260001
(title: " ")

[commit-bot: I haz the power, 2015-03-26 21:05:11]

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

[commit-bot: I haz the power, 2015-03-26 21:05:16]

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

[commit-bot: I haz the power, 2015-03-26 21:05:18]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[fserb, 2015-03-26 21:06:31]

fserb@chromium.org changed reviewers:
- jhawkins@google.com

[fserb, 2015-03-26 21:06:32]

fserb@chromium.org changed required reviewers:
- jhawkins@google.com

[fserb, 2015-03-26 21:06:37]

The CQ bit was checked by fserb@chromium.org

[commit-bot: I haz the power, 2015-03-26 21:07:05]

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

[commit-bot: I haz the power, 2015-03-26 21:27:50]

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

[commit-bot: I haz the power, 2015-03-26 21:27:53]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[fserb, 2015-03-30 18:52:47]

The CQ bit was checked by fserb@chromium.org

[fserb, 2015-03-30 18:52:48]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
mathp@chromium.org, kmadhusu@chromium.org, beaudoin@chromium.org,
jwd@chromium.org, jhawkins@chromium.org
Link to the patchset: https://codereview.chromium.org/1007383003/#ps280001
(title: " ")

[commit-bot: I haz the power, 2015-03-30 18:53:40]

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

[commit-bot: I haz the power, 2015-03-30 20:26:32]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-03-30 20:27:05]

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