Desktop NTP: Add a UMA metric NewTabPage.TilesReceivedTime

Desktop NTP: Add a UMA metric NewTabPage.TilesReceivedTime

This also adds a unit test for NTP load time histograms, and contains
some cleanup in the single-iframe JS.

BUG=514752
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2696223004
Cr-Commit-Position: refs/heads/master@{#451304}
Committed: https://chromium.googlesource.com/chromium/src/+/848f7e4273c840fb95028b513e4902e6f6157a58

[Marc Treib, 2017-02-16 12:27:44]

Description was changed from

==========
Desktop NTP: Add a UMA metric NewTabPage.TilesReceivedTime

This also adds a unit test for NTP load time histograms, and contains
some cleanup in the single-iframe JS.

BUG=514752
==========

to

==========
Desktop NTP: Add a UMA metric NewTabPage.TilesReceivedTime

This also adds a unit test for NTP load time histograms, and contains
some cleanup in the single-iframe JS.

BUG=514752
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Marc Treib, 2017-02-16 12:59:37]

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

[commit-bot: I haz the power, 2017-02-16 12:59:47]

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

[Marc Treib, 2017-02-16 13:07:04]

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

[Marc Treib, 2017-02-16 13:07:05]

PTAL!

[Marc Treib, 2017-02-16 13:09:29]

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

https://codereview.chromium.org/2696223004/diff/20001/chrome/browser/resource...
chrome/browser/resources/local_ntp/most_visited_single.js:174: var showTiles =
function(info) {
The old "showTiles" was renamed to "swapInNewTiles", since that's a) closer to
what it actually does, and b) less confusing wrt the "show" message.

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

https://codereview.chromium.org/2696223004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:179:
UMA_HISTOGRAM_LOAD_TIME("NewTabPage.LoadTime", load_time);
The change from function to macros is really unrelated. Mostly it's because I
like having the full histogram name inline.

https://codereview.chromium.org/2696223004/diff/20001/chrome/common/search/nt...
File chrome/common/search/ntp_logging_events.h (left):

https://codereview.chromium.org/2696223004/diff/20001/chrome/common/search/nt...
chrome/common/search/ntp_logging_events.h:13: // A suggestion coming from the
server was rendered.
Any concerns with removing these old comments?

[sfiera, 2017-02-16 14:35:33]

LGTM

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

https://codereview.chromium.org/2696223004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc:231: // at this
point everything should still be empty.
Is this something we actually care about testing? I would be totally OK with it
if TilesReceivedTime were recorded already at this point.

[commit-bot: I haz the power, 2017-02-16 15:00:42]

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

[commit-bot: I haz the power, 2017-02-16 15:00:43]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-02-16 15:13:09]

Thanks!

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

https://codereview.chromium.org/2696223004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc:231: // at this
point everything should still be empty.
On 2017/02/16 14:35:33, sfiera wrote:
> Is this something we actually care about testing? I would be totally OK with
it
> if TilesReceivedTime were recorded already at this point.

Good point. Removed.

[Marc Treib, 2017-02-16 15:13:30]

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

[Marc Treib, 2017-02-16 15:13:30]

+holte for histograms.xml, PTAL!

[Steven Holte, 2017-02-16 21:32:33]

lgtm

[Marc Treib, 2017-02-17 09:08:47]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2017-02-17 09:08:47]

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

[commit-bot: I haz the power, 2017-02-17 09:09:04]

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, 2017-02-17 10:34:00]

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

[commit-bot: I haz the power, 2017-02-17 10:34:02]

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

[Marc Treib, 2017-02-17 13:49:57]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2017-02-17 13:50:21]

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, 2017-02-17 14:37:57]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487339397356170,
"parent_rev": "0b863f41df7d4eafb9c900e447dbf596150a6771", "commit_rev":
"848f7e4273c840fb95028b513e4902e6f6157a58"}

[commit-bot: I haz the power, 2017-02-17 14:38:32]

Description was changed from

==========
Desktop NTP: Add a UMA metric NewTabPage.TilesReceivedTime

This also adds a unit test for NTP load time histograms, and contains
some cleanup in the single-iframe JS.

BUG=514752
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Desktop NTP: Add a UMA metric NewTabPage.TilesReceivedTime

This also adds a unit test for NTP load time histograms, and contains
some cleanup in the single-iframe JS.

BUG=514752
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2696223004
Cr-Commit-Position: refs/heads/master@{#451304}
Committed:
https://chromium.googlesource.com/chromium/src/+/848f7e4273c840fb95028b513e49...
==========

[commit-bot: I haz the power, 2017-02-17 14:38:34]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/848f7e4273c840fb95028b513e49...