Ensure the 1993 NTP logs to UMA Most Visited tiles that use the fallback thumbnail instead of the p…

Ensure the 1993 NTP logs to UMA Most Visited tiles that use the fallback thumbnail instead of the preferred one.

TBR=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221463

[beaudoin, 2013-09-03 21:01:55]

mathp: Overall logic.
jeremycho: chrome/browser/resources/local_ntp/most_visited_util.js
estade: chrome/browser/ui/webui/ntp/*

[Mathieu, 2013-09-03 21:40:22]

Proposed some new names for the enums and variables, since attempts can get
confusing in this scenario. Let me know what you think.

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_ntp/most_visited_thumbnail.js (right):

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:54:
logEvent(NTP_LOGGING_EVENT_TYPE.NTP_THUMBNAIL_FALLBACK);
NTP_FALLBACK_THUMBNAIL_USED?

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:58:
logEvent(NTP_LOGGING_EVENT_TYPE.NTP_THUMBNAIL_WITH_FALLBACK_ATTEMPT);
nit: NTP_FALLBACK_THUMBNAIL_REQUESTED to be closer to what's happening?

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_ntp/most_visited_util.js (right):

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/most_visited_util.js:22: // There was an
error in loading a thumbnail image.
nit: update comment to mention this is the ultimate failure -- a grey tile.

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/most_visited_util.js:24: // The page
attempted to load a thumbnail image and provided a fallback.
// The page was provided a fallback thumbnail URL, loaded at the same time as
the primary thumbnail URL.

https://codereview.chromium.org/23874005/diff/1/chrome/browser/ui/webui/ntp/n...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.h (right):

https://codereview.chromium.org/23874005/diff/1/chrome/browser/ui/webui/ntp/n...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:69: size_t
number_of_thumbnail_with_fallback_attempts_;
nit: number_of_fallback_thumbnails_requested_?

https://codereview.chromium.org/23874005/diff/1/chrome/browser/ui/webui/ntp/n...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:73: size_t
number_of_thumbnail_fallbacks_;
nit: number_of_fallback_thumbnails_used_?

[beaudoin, 2013-09-03 22:34:54]

Mathieu: PTAL.

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_ntp/most_visited_thumbnail.js (right):

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:54:
logEvent(NTP_LOGGING_EVENT_TYPE.NTP_THUMBNAIL_FALLBACK);
On 2013/09/03 21:40:23, Mathieu Perreault wrote:
> NTP_FALLBACK_THUMBNAIL_USED?

Done.

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:58:
logEvent(NTP_LOGGING_EVENT_TYPE.NTP_THUMBNAIL_WITH_FALLBACK_ATTEMPT);
On 2013/09/03 21:40:23, Mathieu Perreault wrote:
> nit: NTP_FALLBACK_THUMBNAIL_REQUESTED to be closer to what's happening?

Done.

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_ntp/most_visited_util.js (right):

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/most_visited_util.js:22: // There was an
error in loading a thumbnail image.
On 2013/09/03 21:40:23, Mathieu Perreault wrote:
> nit: update comment to mention this is the ultimate failure -- a grey tile.

Done.

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/most_visited_util.js:24: // The page
attempted to load a thumbnail image and provided a fallback.
On 2013/09/03 21:40:23, Mathieu Perreault wrote:
> // The page was provided a fallback thumbnail URL, loaded at the same time as
> the primary thumbnail URL.

Rephrased a little.

Done.

https://codereview.chromium.org/23874005/diff/1/chrome/browser/ui/webui/ntp/n...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.h (right):

https://codereview.chromium.org/23874005/diff/1/chrome/browser/ui/webui/ntp/n...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:69: size_t
number_of_thumbnail_with_fallback_attempts_;
On 2013/09/03 21:40:23, Mathieu Perreault wrote:
> nit: number_of_fallback_thumbnails_requested_?

Done.

https://codereview.chromium.org/23874005/diff/1/chrome/browser/ui/webui/ntp/n...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:73: size_t
number_of_thumbnail_fallbacks_;
On 2013/09/03 21:40:23, Mathieu Perreault wrote:
> nit: number_of_fallback_thumbnails_used_?

Done.

[Mathieu, 2013-09-03 22:40:14]

lgtm

[beaudoin, 2013-09-04 20:40:39]

jeremycho, estade: Friendly ping.

[Evan Stade, 2013-09-04 20:53:19]

lgtm

[jeremycho, 2013-09-04 20:53:56]

lgtm

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_ntp/most_visited_util.js (right):

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/most_visited_util.js:22: // There was an
error in loading a thumbnail image.
On 2013/09/03 22:34:55, beaudoin wrote:
> On 2013/09/03 21:40:23, Mathieu Perreault wrote:
> > nit: update comment to mention this is the ultimate failure -- a grey tile.
> 
> Done.

Not done?

[beaudoin, 2013-09-05 12:47:31]

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_ntp/most_visited_util.js (right):

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/most_visited_util.js:22: // There was an
error in loading a thumbnail image.
On 2013/09/04 20:53:57, jeremycho wrote:
> On 2013/09/03 22:34:55, beaudoin wrote:
> > On 2013/09/03 21:40:23, Mathieu Perreault wrote:
> > > nit: update comment to mention this is the ultimate failure -- a grey
tile.
> > 
> > Done.
> 
> Not done?

Done.

https://codereview.chromium.org/23874005/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/most_visited_util.js:24: // The page
attempted to load a thumbnail image and provided a fallback.
On 2013/09/03 22:34:55, beaudoin wrote:
> On 2013/09/03 21:40:23, Mathieu Perreault wrote:
> > // The page was provided a fallback thumbnail URL, loaded at the same time
as
> > the primary thumbnail URL.
> 
> Rephrased a little.
> 
> Done.

Done done.

[commit-bot: I haz the power, 2013-09-05 12:47:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/23874005/31001

[beaudoin, 2013-09-05 12:58:40]

Added TBR=jam for trivial change to chrome/common/ntp_logging_events.h

[beaudoin, 2013-09-05 14:50:13]

+asvitkine for histograms.xml

[Alexei Svitkine (slow), 2013-09-05 14:59:25]

https://codereview.chromium.org/23874005/diff/54001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/23874005/diff/54001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:19:
number_of_fallback_thumbnails_requested_);
Nit: Move this above the if block that logs it.

https://codereview.chromium.org/23874005/diff/54001/tools/metrics/histograms/...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/23874005/diff/54001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:10084: +<histogram
name="NewTabPage.ThumbnailFallbackRate">
Nit: Add units="%"

https://codereview.chromium.org/23874005/diff/54001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:10087: +    requests that actually
specify a fallback thumbnail are considered here.
Please mention when this is logged.

[beaudoin, 2013-09-05 15:02:55]

Answered asvitkine comments.

https://codereview.chromium.org/23874005/diff/54001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/23874005/diff/54001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:19:
number_of_fallback_thumbnails_requested_);
On 2013/09/05 14:59:25, Alexei Svitkine wrote:
> Nit: Move this above the if block that logs it.

Done.

https://codereview.chromium.org/23874005/diff/54001/tools/metrics/histograms/...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/23874005/diff/54001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:10084: +<histogram
name="NewTabPage.ThumbnailFallbackRate">
On 2013/09/05 14:59:25, Alexei Svitkine wrote:
> Nit: Add units="%"

Done.

https://codereview.chromium.org/23874005/diff/54001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:10087: +    requests that actually
specify a fallback thumbnail are considered here.
On 2013/09/05 14:59:25, Alexei Svitkine wrote:
> Please mention when this is logged.

Done.

[Alexei Svitkine (slow), 2013-09-05 15:04:50]

LGTM

[commit-bot: I haz the power, 2013-09-05 15:06:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/23874005/58001

[jam, 2013-09-05 15:53:53]

On 2013/09/05 12:58:40, beaudoin wrote:
> Added TBR=jam for trivial change to chrome/common/ntp_logging_events.h

can you please send generic reviews to an owner in chrome/owners? i'm overloaded
with reviews in content, so I only like to review stuff in chrome if it's code
that i'm the best reviewer for.

[beaudoin, 2013-09-05 16:02:55]

Added TBR=sky for trivial change to chrome/common/ntp_logging_events.h

[commit-bot: I haz the power, 2013-09-05 16:03:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/23874005/58001

[commit-bot: I haz the power, 2013-09-05 18:20:28]

Change committed as 221463