feed v2: fix reporting for feed opens

See bug for detailed problem.
This change adds a new signal sent to the feed component, informing
that the feed is shown. This happens the first time feed content is
seen, even if a card is hardly shown.

Bug: 1117265
Change-Id: Ic0eb7fbb67a16b7f06554bc1ae48efa34662d106
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2360600Reviewed-by: Ian Wells <iwells@chromium.org>
Commit-Queue: Dan H <harringtond@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799253}

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedSliceViewTracker.java

@@ -30,12 +30,17 @@ class FeedSliceViewTracker implements ViewTreeObserver.OnPreDrawListener {
     private FeedListContentManager mContentManager;
     // The set of content keys already reported as visible.
     private HashSet<String> mContentKeysVisible = new HashSet<String>();
+    private boolean mFeedContentVisible;
     @Nullable
     private Observer mObserver;
 
     /** Notified the first time slices are visible */
     public interface Observer {
+        // Invoked the first time a slice is 66% visible.
         void sliceVisible(String sliceId);
+        // Invoked when feed content is first visible. This can happens as soon as an xsurface view
+        // is partially visible.
+        void feedContentVisible();
     }
 
     FeedSliceViewTracker(@NonNull RecyclerView rootView,
@@ -56,6 +61,14 @@ class FeedSliceViewTracker implements ViewTreeObserver.OnPreDrawListener {
         mContentManager = null;
     }
 
+    /**
+     * Clear tracking so that slices already seen can be reported as viewed again.
+     */
+    public void clear() {
+        mContentKeysVisible.clear();
+        mFeedContentVisible = false;
+    }
+
     // ViewTreeObserver.OnPreDrawListener.
     @Override
     public boolean onPreDraw() {
@@ -69,11 +82,19 @@ class FeedSliceViewTracker implements ViewTreeObserver.OnPreDrawListener {
         for (int i = firstPosition;
                 i <= lastPosition && i < mContentManager.getItemCount() && i >= 0; ++i) {
             String contentKey = mContentManager.getContent(i).getKey();
+            // Feed content slices come with a 'c/' prefix. Ignore everything else.
+            if (!contentKey.startsWith("c/")) continue;
             View childView = layoutManager.findViewByPosition(i);
-            if (mContentKeysVisible.contains(contentKey) || childView == null
-                    || !isViewVisible(childView)) {
+            if (childView == null) continue;
+
+            if (!mFeedContentVisible) {
+                mFeedContentVisible = true;
+                mObserver.feedContentVisible();
+            }
+            if (mContentKeysVisible.contains(contentKey) || !isViewVisible(childView)) {
                 continue;
             }
+
             mContentKeysVisible.add(contentKey);
             mObserver.sliceVisible(contentKey);
         }

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedStreamSurface.java

@@ -348,10 +348,7 @@ public class FeedStreamSurface implements SurfaceActionsHandler, FeedActionsHand
             mRootView = (RecyclerView) mHybridListRenderer.bind(mContentManager);
 
             mSliceViewTracker =
-                    new FeedSliceViewTracker(mRootView, mContentManager, (String sliceId) -> {
-                        FeedStreamSurfaceJni.get().reportSliceViewed(
-                                mNativeFeedStreamSurface, FeedStreamSurface.this, sliceId);
-                    });
+                    new FeedSliceViewTracker(mRootView, mContentManager, new ViewTrackerObserver());
         } else {
             mRootView = null;
         }
@@ -863,6 +860,7 @@ public class FeedStreamSurface implements SurfaceActionsHandler, FeedActionsHand
         }
 
         mScrollReporter.onUnbind();
+        mSliceViewTracker.clear();
 
         FeedStreamSurfaceJni.get().surfaceClosed(mNativeFeedStreamSurface, FeedStreamSurface.this);
         mOpened = false;
@@ -907,10 +905,24 @@ public class FeedStreamSurface implements SurfaceActionsHandler, FeedActionsHand
         }
     }
 
+    private class ViewTrackerObserver implements FeedSliceViewTracker.Observer {
+        @Override
+        public void sliceVisible(String sliceId) {
+            FeedStreamSurfaceJni.get().reportSliceViewed(
+                    mNativeFeedStreamSurface, FeedStreamSurface.this, sliceId);
+        }
+        @Override
+        public void feedContentVisible() {
+            FeedStreamSurfaceJni.get().reportFeedViewed(
+                    mNativeFeedStreamSurface, FeedStreamSurface.this);
+        }
+    }
+
     @NativeMethods
     interface Natives {
         long init(FeedStreamSurface caller);
         int[] getExperimentIds();
+        void reportFeedViewed(long nativeFeedStreamSurface, FeedStreamSurface caller);
         void reportSliceViewed(
                 long nativeFeedStreamSurface, FeedStreamSurface caller, String sliceId);
         void reportNavigationStarted(long nativeFeedStreamSurface, FeedStreamSurface caller);

chrome/android/junit/src/org/chromium/chrome/browser/feed/v2/FeedSliceViewTrackerTest.java

@@ -8,6 +8,7 @@ import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.eq;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
@@ -111,8 +112,8 @@ public class FeedSliceViewTrackerTest {
     public void testOnPreDraw_BothVisibleAreReportedExactlyOnce() {
         mContentManager.addContents(0,
                 Arrays.asList(new FeedListContentManager.FeedContent[] {
-                        new FeedListContentManager.NativeViewContent("key1", mChildA),
-                        new FeedListContentManager.NativeViewContent("key2", mChildB),
+                        new FeedListContentManager.NativeViewContent("c/key1", mChildA),
+                        new FeedListContentManager.NativeViewContent("c/key2", mChildB),
                 }));
         doReturn(0).when(mLayoutManager).findFirstVisibleItemPosition();
         doReturn(1).when(mLayoutManager).findLastVisibleItemPosition();
@@ -124,8 +125,58 @@ public class FeedSliceViewTrackerTest {
 
         mTracker.onPreDraw();
 
-        verify(mObserver).sliceVisible(eq("key1"));
-        verify(mObserver).sliceVisible(eq("key2"));
+        verify(mObserver).feedContentVisible();
+        verify(mObserver).sliceVisible(eq("c/key1"));
+        verify(mObserver).sliceVisible(eq("c/key2"));
+
+        mTracker.onPreDraw(); // Does not repeat call to sliceVisible().
+    }
+
+    @Test
+    @SmallTest
+    public void testOnPreDraw_AfterClearReportsAgain() {
+        mContentManager.addContents(0,
+                Arrays.asList(new FeedListContentManager.FeedContent[] {
+                        new FeedListContentManager.NativeViewContent("c/key1", mChildA),
+                        new FeedListContentManager.NativeViewContent("c/key2", mChildB),
+                }));
+        doReturn(0).when(mLayoutManager).findFirstVisibleItemPosition();
+        doReturn(1).when(mLayoutManager).findLastVisibleItemPosition();
+        doReturn(mChildA).when(mLayoutManager).findViewByPosition(eq(0));
+        doReturn(mChildB).when(mLayoutManager).findViewByPosition(eq(1));
+
+        doReturn(true).when(mTracker).isViewVisible(mChildA);
+        doReturn(true).when(mTracker).isViewVisible(mChildB);
+
+        mTracker.onPreDraw();
+        mTracker.clear();
+        mTracker.onPreDraw(); // repeats observer calls.
+
+        verify(mObserver, times(2)).feedContentVisible();
+        verify(mObserver, times(2)).sliceVisible(eq("c/key1"));
+        verify(mObserver, times(2)).sliceVisible(eq("c/key2"));
+    }
+
+    @Test
+    @SmallTest
+    public void testOnPreDraw_IgnoresNonContentViews() {
+        mContentManager.addContents(0,
+                Arrays.asList(new FeedListContentManager.FeedContent[] {
+                        new FeedListContentManager.NativeViewContent("non-content-key1", mChildA),
+                        new FeedListContentManager.NativeViewContent("non-content-key2", mChildB),
+                }));
+        doReturn(0).when(mLayoutManager).findFirstVisibleItemPosition();
+        doReturn(1).when(mLayoutManager).findLastVisibleItemPosition();
+        doReturn(mChildA).when(mLayoutManager).findViewByPosition(eq(0));
+        doReturn(mChildB).when(mLayoutManager).findViewByPosition(eq(1));
+
+        doReturn(true).when(mTracker).isViewVisible(mChildA);
+        doReturn(true).when(mTracker).isViewVisible(mChildB);
+
+        mTracker.onPreDraw();
+
+        verify(mObserver, times(0)).feedContentVisible();
+        verify(mObserver, times(0)).sliceVisible(any());
 
         mTracker.onPreDraw(); // Does not repeat call to sliceVisible().
     }
@@ -135,8 +186,8 @@ public class FeedSliceViewTrackerTest {
     public void testOnPreDraw_OnlyOneVisible() {
         mContentManager.addContents(0,
                 Arrays.asList(new FeedListContentManager.FeedContent[] {
-                        new FeedListContentManager.NativeViewContent("key1", mChildA),
-                        new FeedListContentManager.NativeViewContent("key2", mChildB),
+                        new FeedListContentManager.NativeViewContent("c/key1", mChildA),
+                        new FeedListContentManager.NativeViewContent("c/key2", mChildB),
                 }));
         doReturn(0).when(mLayoutManager).findFirstVisibleItemPosition();
         doReturn(1).when(mLayoutManager).findLastVisibleItemPosition();
@@ -148,7 +199,7 @@ public class FeedSliceViewTrackerTest {
 
         mTracker.onPreDraw();
 
-        verify(mObserver).sliceVisible(eq("key2"));
+        verify(mObserver).sliceVisible(eq("c/key2"));
     }
 
     @Test
@@ -156,8 +207,8 @@ public class FeedSliceViewTrackerTest {
     public void testOnPreDraw_EmptyRecyclerView() {
         mContentManager.addContents(0,
                 Arrays.asList(new FeedListContentManager.FeedContent[] {
-                        new FeedListContentManager.NativeViewContent("key1", mChildA),
-                        new FeedListContentManager.NativeViewContent("key2", mChildB),
+                        new FeedListContentManager.NativeViewContent("c/key1", mChildA),
+                        new FeedListContentManager.NativeViewContent("c/key2", mChildB),
                 }));
         doReturn(RecyclerView.NO_POSITION).when(mLayoutManager).findFirstVisibleItemPosition();
         doReturn(RecyclerView.NO_POSITION).when(mLayoutManager).findLastVisibleItemPosition();

chrome/browser/android/feed/v2/feed_stream_surface.cc

@@ -186,6 +186,12 @@ void FeedStreamSurface::ReportSliceViewed(
       GetSurfaceId(), base::android::ConvertJavaStringToUTF8(env, slice_id));
 }
 
+void FeedStreamSurface::ReportFeedViewed(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& obj) {
+  feed_stream_api_->ReportFeedViewed(GetSurfaceId());
+}
+
 void FeedStreamSurface::ReportSendFeedbackAction(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj) {

chrome/browser/android/feed/v2/feed_stream_surface.h

@@ -71,6 +71,8 @@ class FeedStreamSurface : public FeedStreamApi::SurfaceInterface {
   void ReportSliceViewed(JNIEnv* env,
                          const base::android::JavaParamRef<jobject>& obj,
                          const base::android::JavaParamRef<jstring>& slice_id);
+  void ReportFeedViewed(JNIEnv* env,
+                        const base::android::JavaParamRef<jobject>& obj);
   void ReportOpenAction(JNIEnv* env,
                         const base::android::JavaParamRef<jobject>& obj,
                         const base::android::JavaParamRef<jstring>& slice_id);

components/feed/core/v2/feed_stream.cc

@@ -673,7 +673,9 @@ void FeedStream::ReportSliceViewed(SurfaceId surface_id,
   if (index >= 0)
     metrics_reporter_->ContentSliceViewed(surface_id, index);
 }
-
+void FeedStream::ReportFeedViewed(SurfaceId surface_id) {
+  metrics_reporter_->FeedViewed(surface_id);
+}
 void FeedStream::ReportSendFeedbackAction() {
   metrics_reporter_->SendFeedbackAction();
 }

components/feed/core/v2/feed_stream.h

@@ -147,6 +147,7 @@ class FeedStream : public FeedStreamApi,
 
   void ReportSliceViewed(SurfaceId surface_id,
                          const std::string& slice_id) override;
+  void ReportFeedViewed(SurfaceId surface_id) override;
   void ReportNavigationStarted() override;
   void ReportPageLoaded() override;
   void ReportOpenAction(const std::string& slice_id) override;

components/feed/core/v2/metrics_reporter.cc

@@ -171,6 +171,9 @@ void MetricsReporter::ContentSliceViewed(SurfaceId surface_id,
                                          int index_in_stream) {
   base::UmaHistogramExactLinear("NewTabPage.ContentSuggestions.Shown",
                                 index_in_stream, kMaxSuggestionsTotal);
+}
+
+void MetricsReporter::FeedViewed(SurfaceId surface_id) {
   if (load_latencies_) {
     load_latencies_->StepComplete(LoadLatencyTimes::kStreamViewed);
 
@@ -291,7 +294,6 @@ void MetricsReporter::EphemeralStreamChangeRejected() {
 
 void MetricsReporter::SurfaceOpened(SurfaceId surface_id) {
   ReportPersistentDataIfDayIsDone();
-
   surfaces_waiting_for_content_.emplace(surface_id, clock_->NowTicks());
   ReportUserActionHistogram(FeedUserActionType::kOpenedFeedSurface);
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
@@ -327,6 +329,7 @@ void MetricsReporter::ReportOpenFeedIfNeeded(SurfaceId surface_id,
     return;
   base::TimeDelta latency = clock_->NowTicks() - iter->second;
   surfaces_waiting_for_content_.erase(iter);
+
   if (success) {
     base::UmaHistogramCustomTimes(
         "ContentSuggestions.Feed.UserJourney.OpenFeed.SuccessDuration", latency,

components/feed/core/v2/metrics_reporter.h

@@ -68,6 +68,7 @@ class MetricsReporter {
   // User interactions. See |FeedStreamApi| for definitions.
 
   virtual void ContentSliceViewed(SurfaceId surface_id, int index_in_stream);
+  void FeedViewed(SurfaceId surface_id);
   void OpenAction(int index_in_stream);
   void OpenVisitComplete(base::TimeDelta visit_time);
   void OpenInNewTabAction(int index_in_stream);

components/feed/core/v2/metrics_reporter_unittest.cc

@@ -196,8 +196,8 @@ TEST_F(MetricsReporterTest, ReportsLoadStepLatenciesOnFirstView) {
                             std::move(latencies));
   }
   task_environment_.FastForwardBy(base::TimeDelta::FromMilliseconds(300));
-  reporter_->ContentSliceViewed(kSurfaceId, 0);
-  reporter_->ContentSliceViewed(kSurfaceId, 1);
+  reporter_->FeedViewed(kSurfaceId);
+  reporter_->FeedViewed(kSurfaceId);
 
   histogram_.ExpectUniqueTimeSample(
       "ContentSuggestions.Feed.LoadStepLatency.LoadFromStore",
@@ -388,7 +388,7 @@ TEST_F(MetricsReporterTest, SurfaceOpened) {
 TEST_F(MetricsReporterTest, OpenFeedSuccessDuration) {
   reporter_->SurfaceOpened(kSurfaceId);
   task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(9));
-  reporter_->ContentSliceViewed(kSurfaceId, 0);
+  reporter_->FeedViewed(kSurfaceId);
 
   histogram_.ExpectUniqueTimeSample(
       "ContentSuggestions.Feed.UserJourney.OpenFeed.SuccessDuration",

components/feed/core/v2/public/feed_stream_api.h

@@ -110,6 +110,9 @@ class FeedStreamApi {
   // once for each viewed slice in the stream.
   virtual void ReportSliceViewed(SurfaceId surface_id,
                                  const std::string& slice_id) = 0;
+  // Some feed content has been loaded and is now available to the user on the
+  // feed surface. Reported only once after a surface is attached.
+  virtual void ReportFeedViewed(SurfaceId surface_id) = 0;
   // Navigation was started in response to a link in the Feed. This event
   // eventually leads to |ReportPageLoaded()| if a page is loaded successfully.
   virtual void ReportNavigationStarted() = 0;

components/feed/core/v2/types.cc

@@ -8,6 +8,7 @@
 
 #include "base/base64.h"
 #include "base/pickle.h"
+#include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/util/values/values_util.h"
 #include "base/values.h"
@@ -108,14 +109,21 @@ NetworkResponseInfo& NetworkResponseInfo::operator=(
     const NetworkResponseInfo&) = default;
 
 std::string ToString(ContentRevision c) {
-  return base::NumberToString(c.value());
+  // The 'c/' prefix is used to identify slices as content. Don't change this
+  // without updating the Java side.
+  return base::StrCat({"c/", base::NumberToString(c.value())});
 }
 
 ContentRevision ToContentRevision(const std::string& str) {
-  uint32_t value;
-  if (!base::StringToUint(str, &value))
+  if (str.size() < 3)
     return {};
-  return ContentRevision(value);
+
+  uint32_t value;
+  if (str[0] == 'c' && str[1] == '/' &&
+      base::StringToUint(base::StringPiece(str).substr(2), &value)) {
+    return ContentRevision(value);
+  }
+  return {};
 }
 
 std::string SerializeDebugStreamData(const DebugStreamData& data) {

components/feed/core/v2/types_unittest.cc

@@ -123,4 +123,14 @@ TEST(PersistentMetricsData, SerializesAndDeserializes) {
   EXPECT_EQ(data.current_day_start, deserialized_value.current_day_start);
 }
 
+TEST(Types, ToContentRevision) {
+  const ContentRevision cr = ContentRevision::Generator().GenerateNextId();
+
+  EXPECT_EQ("c/1", ToString(cr));
+  EXPECT_EQ(cr, ToContentRevision(ToString(cr)));
+  EXPECT_EQ(ContentRevision(), ToContentRevision("2"));
+  EXPECT_EQ(ContentRevision(), ToContentRevision("c"));
+  EXPECT_EQ(ContentRevision(), ToContentRevision("c/"));
+}
+
 }  // namespace feed