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

Issue 766346 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

NewTabPage.ContentSuggestions.CountOnNtpOpenedIfVisible.Articles.Prefetched.Offline records zero incorrectly

Project Member Reported by dewittj@chromium.org, Sep 18 2017

Issue description

See demo video attached.

Looking at the code, the |isPrefetched| bit is set by an OfflinePageBridge#selectPageForOnlineUrl call, which may or may not have fired its callback by the time the metrics are recorded.
 
demo.mp4
13.3 MB View Download
Cc: dim...@chromium.org
Components: UI>Browser>Offline UI>Browser>NewTabPage
Interesting part of video starts at 0:40.

Comment 3 by mastiz@chromium.org, Sep 19 2017

Labels: zine-triaged
Thank you a lot for looking into this and sorry for missing this when adding the metric! I will look into how to fix this, but I feel like we won't be able to merge it into M62. I believe this does not affect clicks and possibly suggestion impressions (unless they appear in NTP without scrolling).
Status: Started (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 11 2017

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

commit 2a0284b349041d58df86eb42e6c103c7312f818b
Author: vitaliii <vitaliii@chromium.org>
Date: Wed Oct 11 18:03:51 2017

[NTP::Prefetch] Report prefetched count on NTP opened correctly.

1) Deprecate NewTabPage.ContentSuggestions.CountOnNtpOpenedIfVisible.\
Articles.Prefetched.Offline.

2) Replace it with NewTabPage.ContentSuggestions.\
CountOnNtpOpenedIfVisible.Articles.Prefetched.Offline2, which is
reported after all async |selectPageForOnlineUrl| requests to
OfflinePages model finish.

Reason: previously the metric was reported when NTP was opened. As a
result there was a race condition and it could be reported before all
|selectPageForOnlineUrl| requests finish (i.e. before the number of
prefetched articles is actually known).

Bug:  766346 
Change-Id: I249111acd2acfecdc26dd63cf9aa28281a7ad6c7
Reviewed-on: https://chromium-review.googlesource.com/708799
Commit-Queue: vitaliii <vitaliii@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Peter Conn <peconn@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508033}
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsCarouselAdapter.java
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsEventReporter.java
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsEventReporterBridge.java
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsOfflineModelObserver.java
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/chrome/browser/android/ntp/suggestions_event_reporter_bridge.cc
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/DummySuggestionsEventReporter.java
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/components/ntp_snippets/content_suggestions_metrics.cc
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/components/ntp_snippets/content_suggestions_metrics.h
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/components/ntp_snippets/content_suggestions_metrics_unittest.cc
[modify] https://crrev.com/2a0284b349041d58df86eb42e6c103c7312f818b/tools/metrics/histograms/histograms.xml

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 12 2017

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

commit 0f67f764e2ff53662660708ebd0a1960739c21d3
Author: vitaliii <vitaliii@chromium.org>
Date: Thu Oct 12 13:24:54 2017

[NTP::Prefetch] Report prefetched impression on NTP opened correctly.

1) Deprecate NewTabPage.ContentSuggestions.Shown.Articles./
Prefetched.Offline.

2) Replace it with NewTabPage.ContentSuggestions.Shown.Articles./
Prefetched.Offline2, which is reported after issuing an additional async
|selectPageForOnlineUrl| request to OfflinePages model to confirm that
the URL is not prefetched.

Reason: previously the metric was reported for some suggestions
immediately when NTP was opened. As a result there was a race condition
and it could be reported before all |selectPageForOnlineUrl| requests
finish (i.e. before the prefetched status of some articles is actually
known).

Bug:  766346 
Change-Id: Iafccbfcfecdb34a7d7856c79fa3154c4420f7b9e
Reviewed-on: https://chromium-review.googlesource.com/712254
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Peter Conn <peconn@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: vitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508334}
[modify] https://crrev.com/0f67f764e2ff53662660708ebd0a1960739c21d3/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java
[modify] https://crrev.com/0f67f764e2ff53662660708ebd0a1960739c21d3/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
[modify] https://crrev.com/0f67f764e2ff53662660708ebd0a1960739c21d3/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java
[modify] https://crrev.com/0f67f764e2ff53662660708ebd0a1960739c21d3/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
[modify] https://crrev.com/0f67f764e2ff53662660708ebd0a1960739c21d3/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsEventReporterBridge.java
[modify] https://crrev.com/0f67f764e2ff53662660708ebd0a1960739c21d3/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsOfflineModelObserver.java
[modify] https://crrev.com/0f67f764e2ff53662660708ebd0a1960739c21d3/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java
[modify] https://crrev.com/0f67f764e2ff53662660708ebd0a1960739c21d3/chrome/browser/android/ntp/suggestions_event_reporter_bridge.cc
[modify] https://crrev.com/0f67f764e2ff53662660708ebd0a1960739c21d3/components/ntp_snippets/content_suggestions_metrics.cc
[modify] https://crrev.com/0f67f764e2ff53662660708ebd0a1960739c21d3/components/ntp_snippets/content_suggestions_metrics.h
[modify] https://crrev.com/0f67f764e2ff53662660708ebd0a1960739c21d3/components/ntp_snippets/content_suggestions_metrics_unittest.cc
[modify] https://crrev.com/0f67f764e2ff53662660708ebd0a1960739c21d3/ios/chrome/browser/content_suggestions/content_suggestions_metrics_recorder.mm
[modify] https://crrev.com/0f67f764e2ff53662660708ebd0a1960739c21d3/tools/metrics/histograms/histograms.xml

Labels: M-63
NextAction: 2017-10-17
Status: Fixed (was: Started)
I've fixed metrics for

- number of prefetched article suggestions on NTP when it is opened offline.
NewTabPage.ContentSuggestions.CountOnNtpOpenedIfVisible.Articles.Prefetched.Offline2

- position of prefetched article suggestion impression when offline
NewTabPage.ContentSuggestions.CountOnNtpOpenedIfVisible.Articles.Prefetched.Offline2

Possible remaining issues:
- I did not do anything for clicks, because they happen after NTP is opened. As my testing on device has shown, even on a slow device, input is blocked before NTP finished loading.

- In all 3 metrics we use OfflinePageBridge.selectPageForOfflineUrl, which returns only the most recently created offline page. Thus, if the page is prefetched and then user downloads its URL, it won't be reported in our metrics.

Setting next action reminder to verify that the fix has got into Beta and the fix itself.
Status: Verified (was: Fixed)
The fixes have got into Beta.
I've also verified them in Canary 64.0.3241.0.
The NextAction date has arrived: 2017-10-17
NextAction: ----

Sign in to add a comment