New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 625990 link

Starred by 3 users
Status: Fixed
Owner:
█ sfiera@chromium.org
Last visit > 30 days ago
Closed: Jul 2016
Cc:
treib@chromium.org
█ ramyan@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue 611341



Sign in to add a comment

NTPUserDataLogger::EmitNtpStatistics is called inconsistently

Project Member Reported by treib@chromium.org, Jul 6 2016 
Right now, NTPUserDataLogger::EmitNtpStatistics is called at least 5 times for each NTP, and it might actually emit data before everything is accumulated.
It's also called when switching between TopSites and MostLikely, with the intention of flushing all accumulated data, but it doesn't always actually do that.

We should make sure that we emit statistics exactly once per NTP, when the tab is closed or the user navigates away.
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 11 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c48795c69de4878814ca44038544c1afd78c1dce

commit c48795c69de4878814ca44038544c1afd78c1dce
Author: sfiera <sfiera@chromium.org>
Date: Mon Jul 11 12:17:04 2016

Make EmitNtpStatistics() private.

It's still called from the same places, but through public functions
that describe what is happening and why, rather than exposing an
implementation detail.

Also, add an enum so that it's immediately obvious at the call site why
EmitNtpStatistics() is being called.

BUG= 625990 

Review-Url: https://codereview.chromium.org/2127783002
Cr-Commit-Position: refs/heads/master@{#404636}

[modify] https://crrev.com/c48795c69de4878814ca44038544c1afd78c1dce/chrome/browser/ui/search/instant_controller.cc
[modify] https://crrev.com/c48795c69de4878814ca44038544c1afd78c1dce/chrome/browser/ui/search/instant_tab.cc
[modify] https://crrev.com/c48795c69de4878814ca44038544c1afd78c1dce/chrome/browser/ui/search/instant_tab.h
[modify] https://crrev.com/c48795c69de4878814ca44038544c1afd78c1dce/chrome/browser/ui/search/search_tab_helper.cc
[modify] https://crrev.com/c48795c69de4878814ca44038544c1afd78c1dce/chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc
[modify] https://crrev.com/c48795c69de4878814ca44038544c1afd78c1dce/chrome/browser/ui/webui/ntp/ntp_user_data_logger.h
[modify] https://crrev.com/c48795c69de4878814ca44038544c1afd78c1dce/chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 13 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9448453348b63120430d305312fcd894a9ff7f65

commit 9448453348b63120430d305312fcd894a9ff7f65
Author: sfiera <sfiera@chromium.org>
Date: Wed Jul 13 08:44:00 2016

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

Review-Url: https://codereview.chromium.org/2141933002
Cr-Commit-Position: refs/heads/master@{#405087}

[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/resources/local_ntp/most_visited_single.js
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/browser_instant_controller.cc
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/search/instant_controller.cc
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/search/instant_controller.h
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/search/instant_tab.cc
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/search/instant_tab.h
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/search/search_tab_helper.cc
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/search/search_tab_helper.h
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/search/search_tab_helper_unittest.cc
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/webui/ntp/ntp_user_data_logger.h
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/common/search/ntp_logging_events.h

Comment 3 by sfiera@chromium.org, Jul 13 2016

Status: Fixed (was: Assigned)
EmitNtpStatistics() is now called from exactly one place and (we think) it's the right one.
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 13 2016

Labels: merge-merged-2795
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9448453348b63120430d305312fcd894a9ff7f65

commit 9448453348b63120430d305312fcd894a9ff7f65
Author: sfiera <sfiera@chromium.org>
Date: Wed Jul 13 08:44:00 2016

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

Review-Url: https://codereview.chromium.org/2141933002
Cr-Commit-Position: refs/heads/master@{#405087}

[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/resources/local_ntp/most_visited_single.js
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/browser_instant_controller.cc
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/search/instant_controller.cc
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/search/instant_controller.h
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/search/instant_tab.cc
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/search/instant_tab.h
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/search/search_tab_helper.cc
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/search/search_tab_helper.h
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/search/search_tab_helper_unittest.cc
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/webui/ntp/ntp_user_data_logger.h
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc
[modify] https://crrev.com/9448453348b63120430d305312fcd894a9ff7f65/chrome/common/search/ntp_logging_events.h
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 14 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c156d90aec850be4fe24b3f218ede84ea11a2c32

commit c156d90aec850be4fe24b3f218ede84ea11a2c32
Author: sfiera <sfiera@chromium.org>
Date: Thu Jul 14 12:12:46 2016

Force early creation of NTPUserDataLogger.

Without this, it won't be created early enough during startup to detect
that startup is happening.

BUG= 625990 

Review-Url: https://codereview.chromium.org/2152483002
Cr-Commit-Position: refs/heads/master@{#405476}

[modify] https://crrev.com/c156d90aec850be4fe24b3f218ede84ea11a2c32/chrome/browser/ui/search/search_tab_helper.cc
►

Sign in to add a comment