Video Tutorials : More UI fixes

This CL contains :
1- Hide video duration text if the duration is 0:00 (e.g. summary card)
2- Change close button style to match spec in night mode
3- Disable Try Now for download tutorial
4- Added some text for metrics
5- Some more tests in VideoPlayerViewBinder

Change-Id: Ic435739b7f7d4c5052549aa68352fa66c03bcbf2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2508999Reviewed-by: Min Qin <qinmin@chromium.org>
Commit-Queue: Shakti Sahu <shaktisahu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822874}

chrome/browser/video_tutorials/android/java/res/layout/video_tutorial_iph_card.xml

@@ -60,7 +60,9 @@
         android:layout_width="48dp"
         android:layout_alignParentEnd="true"
         android:background="?attr/selectableItemBackground"
-        android:contentDescription="@string/close" 
-        android:src="@drawable/btn_close" />
+        android:contentDescription="@string/close"
+        android:scaleType="center"
+        android:src="@drawable/btn_close"
+        app:tint="@color/default_icon_color_tint_list" />
 
 </RelativeLayout>

chrome/browser/video_tutorials/android/java/res/layout/video_tutorial_list.xml

@@ -5,6 +5,7 @@
 
 <LinearLayout
     xmlns:android="http://schemas.android.com/apk/res/android"
+    xmlns:app="http://schemas.android.com/apk/res-auto"
     android:id="@+id/video_tutorial_list"
     android:layout_width="match_parent"
     android:layout_height="wrap_content"
@@ -36,7 +37,8 @@
             android:background="?attr/selectableItemBackground"
             android:contentDescription="@string/close"
             android:scaleType="center"
-            android:src="@drawable/btn_close" />
+            android:src="@drawable/btn_close"
+            app:tint="@color/default_icon_color_tint_list" />
     </LinearLayout>
 
     <androidx.recyclerview.widget.RecyclerView

chrome/browser/video_tutorials/internal/BUILD.gn

@@ -148,6 +148,7 @@ if (is_android) {
     sources = [
       "android/java/src/org/chromium/chrome/browser/video_tutorials/PlaybackStateObserverUnitTest.java",
       "android/java/src/org/chromium/chrome/browser/video_tutorials/languages/LanguagePickerMediatorUnitTest.java",
+      "android/java/src/org/chromium/chrome/browser/video_tutorials/metrics/VideoTutorialMetricsUnitTest.java",
       "android/java/src/org/chromium/chrome/browser/video_tutorials/player/VideoPlayerMediatorUnitTest.java",
     ]
 

chrome/browser/video_tutorials/internal/android/java/res/layout/language_picker.xml

@@ -5,6 +5,7 @@
 
 <RelativeLayout
     xmlns:android="http://schemas.android.com/apk/res/android"
+    xmlns:app="http://schemas.android.com/apk/res-auto"
     android:id="@+id/language_picker"
     android:layout_width="match_parent"
     android:layout_height="match_parent"
@@ -19,7 +20,8 @@
         android:background="?attr/selectableItemBackground"
         android:contentDescription="@string/close"
         android:scaleType="center"
-        android:src="@drawable/btn_close" />
+        android:src="@drawable/btn_close"
+        app:tint="@color/default_icon_color_tint_list" />
 
     <TextView
         android:id="@+id/title_view"

chrome/browser/video_tutorials/internal/android/java/src/org/chromium/chrome/browser/video_tutorials/PlaybackStateObserverUnitTest.java

@@ -71,6 +71,8 @@ public class PlaybackStateObserverUnitTest {
 
     @Test
     public void testPlaybackComplete() {
+        mPlaybackStateObserver.reset();
+
         long initialSystemTime = 30000L;
         PlaybackStateObserver.setCurrentSystemTimeForTesting(initialSystemTime);
         MediaPosition position = new MediaPosition(DURATION_MS, 0, 1.f, initialSystemTime);

chrome/browser/video_tutorials/internal/android/java/src/org/chromium/chrome/browser/video_tutorials/VideoTutorialUtils.java

@@ -44,11 +44,11 @@ public class VideoTutorialUtils {
     /** @return Whether or not to show the Try Now button on the video player. */
     public static boolean shouldShowTryNow(@FeatureType int featureType) {
         switch (featureType) {
-            case FeatureType.DOWNLOAD:
             case FeatureType.SEARCH:
             case FeatureType.VOICE_SEARCH:
                 return true;
             case FeatureType.CHROME_INTRO:
+            case FeatureType.DOWNLOAD:
             default:
                 return false;
         }

chrome/browser/video_tutorials/internal/android/java/src/org/chromium/chrome/browser/video_tutorials/iph/VideoIPHCoordinatorImpl.java

@@ -53,6 +53,7 @@ public class VideoIPHCoordinatorImpl implements VideoIPHCoordinator {
         mModel.set(VideoIPHProperties.DISPLAY_TITLE, tutorial.title);
         mModel.set(VideoIPHProperties.VIDEO_LENGTH,
                 VideoTutorialUtils.getVideoLengthString(tutorial.videoLength));
+        mModel.set(VideoIPHProperties.SHOW_VIDEO_LENGTH, tutorial.videoLength != 0);
         mModel.set(VideoIPHProperties.CLICK_LISTENER, () -> mOnClickListener.onResult(tutorial));
         mModel.set(
                 VideoIPHProperties.DISMISS_LISTENER, () -> mOnDismissListener.onResult(tutorial));

chrome/browser/video_tutorials/internal/android/java/src/org/chromium/chrome/browser/video_tutorials/iph/VideoIPHProperties.java

@@ -22,6 +22,13 @@ class VideoIPHProperties {
     public static final WritableObjectPropertyKey<String> VIDEO_LENGTH =
             new WritableObjectPropertyKey<>();
 
+    /**
+     * Whether or not to show the video length text. Typically if the video length is zero, the
+     * view will be hidden, e.g. summary card.
+     */
+    public static final WritableBooleanPropertyKey SHOW_VIDEO_LENGTH =
+            new WritableBooleanPropertyKey();
+
     /** The thumbnail provider to supply thumbnail images. */
     public static final WritableObjectPropertyKey<AsyncImageView.Factory> THUMBNAIL_PROVIDER =
             new WritableObjectPropertyKey<>();
@@ -36,5 +43,5 @@ class VideoIPHProperties {
 
     /** All keys associated with the model. */
     public static final PropertyKey[] ALL_KEYS = new PropertyKey[] {VISIBILITY, DISPLAY_TITLE,
-            VIDEO_LENGTH, THUMBNAIL_PROVIDER, CLICK_LISTENER, DISMISS_LISTENER};
+            VIDEO_LENGTH, SHOW_VIDEO_LENGTH, THUMBNAIL_PROVIDER, CLICK_LISTENER, DISMISS_LISTENER};
 }

chrome/browser/video_tutorials/internal/android/java/src/org/chromium/chrome/browser/video_tutorials/iph/VideoIPHView.java

@@ -42,6 +42,11 @@ class VideoIPHView {
         view.setText(videoLength);
     }
 
+    private void showVideoLength(boolean show) {
+        TextView view = mCardView.findViewById(R.id.video_length);
+        view.setVisibility(show ? View.VISIBLE : View.GONE);
+    }
+
     private View getThumbnailView() {
         return mCardView.findViewById(R.id.thumbnail);
     }
@@ -66,6 +71,8 @@ class VideoIPHView {
             view.setTitle(model.get(VideoIPHProperties.DISPLAY_TITLE));
         } else if (propertyKey == VideoIPHProperties.VIDEO_LENGTH) {
             view.setVideoLength(model.get(VideoIPHProperties.VIDEO_LENGTH));
+        } else if (propertyKey == VideoIPHProperties.SHOW_VIDEO_LENGTH) {
+            view.showVideoLength(model.get(VideoIPHProperties.SHOW_VIDEO_LENGTH));
         } else if (propertyKey == VideoIPHProperties.CLICK_LISTENER) {
             view.setClickListener(model.get(VideoIPHProperties.CLICK_LISTENER));
         } else if (propertyKey == VideoIPHProperties.DISMISS_LISTENER) {

chrome/browser/video_tutorials/internal/android/java/src/org/chromium/chrome/browser/video_tutorials/metrics/VideoTutorialMetricsUnitTest.java

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.video_tutorials.metrics;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.robolectric.annotation.Config;
+
+import org.chromium.base.metrics.test.ShadowRecordHistogram;
+import org.chromium.base.test.BaseRobolectricTestRunner;
+import org.chromium.chrome.browser.video_tutorials.FeatureType;
+import org.chromium.chrome.browser.video_tutorials.metrics.VideoTutorialMetrics.UserAction;
+import org.chromium.chrome.browser.video_tutorials.metrics.VideoTutorialMetrics.WatchState;
+
+/**
+ * Tests for {@link VideoTutorialMetrics}.
+ */
+@RunWith(BaseRobolectricTestRunner.class)
+@Config(shadows = {ShadowRecordHistogram.class})
+public class VideoTutorialMetricsUnitTest {
+    @Before
+    public void setUp() {
+        ShadowRecordHistogram.reset();
+    }
+
+    @After
+    public void tearDown() {
+        ShadowRecordHistogram.reset();
+    }
+
+    @Test
+    public void testUserActionHistogramNames() {
+        VideoTutorialMetrics.recordUserAction(FeatureType.CHROME_INTRO, UserAction.SHARE);
+        VideoTutorialMetrics.recordUserAction(FeatureType.DOWNLOAD, UserAction.CLOSE);
+        VideoTutorialMetrics.recordUserAction(FeatureType.SEARCH, UserAction.CHANGE_LANGUAGE);
+        VideoTutorialMetrics.recordUserAction(
+                FeatureType.VOICE_SEARCH, UserAction.WATCH_NEXT_VIDEO);
+        VideoTutorialMetrics.recordUserAction(FeatureType.SEARCH, UserAction.TRY_NOW);
+        VideoTutorialMetrics.recordUserAction(
+                FeatureType.VOICE_SEARCH, UserAction.BACK_PRESS_WHEN_SHOWING_VIDEO_PLAYER);
+        Assert.assertEquals(1,
+                ShadowRecordHistogram.getHistogramValueCountForTesting(
+                        "VideoTutorials.ChromeIntro.Player.UserAction", UserAction.SHARE));
+        Assert.assertEquals(1,
+                ShadowRecordHistogram.getHistogramValueCountForTesting(
+                        "VideoTutorials.Download.Player.UserAction", UserAction.CLOSE));
+        Assert.assertEquals(1,
+                ShadowRecordHistogram.getHistogramValueCountForTesting(
+                        "VideoTutorials.Search.Player.UserAction", UserAction.CHANGE_LANGUAGE));
+        Assert.assertEquals(1,
+                ShadowRecordHistogram.getHistogramValueCountForTesting(
+                        "VideoTutorials.VoiceSearch.Player.UserAction",
+                        UserAction.WATCH_NEXT_VIDEO));
+        Assert.assertEquals(1,
+                ShadowRecordHistogram.getHistogramValueCountForTesting(
+                        "VideoTutorials.Search.Player.UserAction", UserAction.TRY_NOW));
+        Assert.assertEquals(1,
+                ShadowRecordHistogram.getHistogramValueCountForTesting(
+                        "VideoTutorials.VoiceSearch.Player.UserAction",
+                        UserAction.BACK_PRESS_WHEN_SHOWING_VIDEO_PLAYER));
+    }
+
+    @Test
+    public void testWatchStateHistogramNames() {
+        VideoTutorialMetrics.recordWatchStateUpdate(FeatureType.CHROME_INTRO, WatchState.STARTED);
+        VideoTutorialMetrics.recordWatchStateUpdate(FeatureType.CHROME_INTRO, WatchState.COMPLETED);
+        VideoTutorialMetrics.recordWatchStateUpdate(FeatureType.CHROME_INTRO, WatchState.PAUSED);
+        VideoTutorialMetrics.recordWatchStateUpdate(FeatureType.CHROME_INTRO, WatchState.RESUMED);
+        VideoTutorialMetrics.recordWatchStateUpdate(FeatureType.CHROME_INTRO, WatchState.PAUSED);
+        VideoTutorialMetrics.recordWatchStateUpdate(FeatureType.CHROME_INTRO, WatchState.RESUMED);
+        VideoTutorialMetrics.recordWatchStateUpdate(FeatureType.CHROME_INTRO, WatchState.WATCHED);
+        Assert.assertEquals(1,
+                ShadowRecordHistogram.getHistogramValueCountForTesting(
+                        "VideoTutorials.ChromeIntro.Player.Progress", WatchState.STARTED));
+        Assert.assertEquals(1,
+                ShadowRecordHistogram.getHistogramValueCountForTesting(
+                        "VideoTutorials.ChromeIntro.Player.Progress", WatchState.COMPLETED));
+        Assert.assertEquals(2,
+                ShadowRecordHistogram.getHistogramValueCountForTesting(
+                        "VideoTutorials.ChromeIntro.Player.Progress", WatchState.PAUSED));
+        Assert.assertEquals(2,
+                ShadowRecordHistogram.getHistogramValueCountForTesting(
+                        "VideoTutorials.ChromeIntro.Player.Progress", WatchState.RESUMED));
+        Assert.assertEquals(1,
+                ShadowRecordHistogram.getHistogramValueCountForTesting(
+                        "VideoTutorials.ChromeIntro.Player.Progress", WatchState.WATCHED));
+    }
+}

chrome/browser/video_tutorials/internal/android/java/src/org/chromium/chrome/browser/video_tutorials/player/VideoPlayerMediatorUnitTest.java

@@ -210,7 +210,7 @@ public class VideoPlayerMediatorUnitTest {
     @Test
     public void testTryNowEnabledForFeatures() {
         Assert.assertFalse(VideoTutorialUtils.shouldShowTryNow(FeatureType.CHROME_INTRO));
-        Assert.assertTrue(VideoTutorialUtils.shouldShowTryNow(FeatureType.DOWNLOAD));
+        Assert.assertFalse(VideoTutorialUtils.shouldShowTryNow(FeatureType.DOWNLOAD));
         Assert.assertTrue(VideoTutorialUtils.shouldShowTryNow(FeatureType.SEARCH));
         Assert.assertTrue(VideoTutorialUtils.shouldShowTryNow(FeatureType.VOICE_SEARCH));
         Assert.assertFalse(VideoTutorialUtils.shouldShowTryNow(99));

chrome/browser/video_tutorials/internal/android/java/src/org/chromium/chrome/browser/video_tutorials/player/VideoPlayerViewBinderTest.java

@@ -26,6 +26,7 @@ import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
 
 import org.chromium.base.test.UiThreadTest;
+import org.chromium.chrome.browser.video_tutorials.PlaybackStateObserver.WatchStateInfo.State;
 import org.chromium.chrome.browser.video_tutorials.R;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.components.thinwebview.ThinWebView;
@@ -46,7 +47,7 @@ public class VideoPlayerViewBinderTest {
             new ActivityTestRule<>(DummyUiActivity.class);
 
     private Activity mActivity;
-    private View mMainView;
+    private VideoPlayerView mVideoPlayerView;
     private View mLoadingView;
     private View mLanguagePickerView;
     private View mControls;
@@ -67,24 +68,25 @@ public class VideoPlayerViewBinderTest {
             FrameLayout thinWebViewLayout = new FrameLayout(mActivity);
             Mockito.when(mThinWebView.getView()).thenReturn(thinWebViewLayout);
 
-            VideoPlayerView videoPlayerView = new VideoPlayerView(mActivity, mModel, mThinWebView);
-            mMainView = videoPlayerView.getView();
-            mActivity.setContentView(mMainView);
+            mVideoPlayerView = new VideoPlayerView(mActivity, mModel, mThinWebView);
+            View mainView = mVideoPlayerView.getView();
+            mActivity.setContentView(mainView);
 
-            mLanguagePickerView = mMainView.findViewById(R.id.language_picker);
-            mLoadingView = mMainView.findViewById(R.id.loading_root);
-            mControls = mMainView.findViewById(R.id.player_root);
+            mLanguagePickerView = mainView.findViewById(R.id.language_picker);
+            mLoadingView = mainView.findViewById(R.id.loading_root);
+            mControls = mainView.findViewById(R.id.player_root);
             mLanguagePickerView.setVisibility(View.GONE);
             mLoadingView.setVisibility(View.GONE);
             mControls.setVisibility(View.GONE);
             mMCP = PropertyModelChangeProcessor.create(
-                    mModel, videoPlayerView, new VideoPlayerViewBinder());
+                    mModel, mVideoPlayerView, new VideoPlayerViewBinder());
         });
     }
 
     @After
     public void tearDown() throws Exception {
         mMCP.destroy();
+        mVideoPlayerView.destroy();
     }
 
     @Test
@@ -118,9 +120,13 @@ public class VideoPlayerViewBinderTest {
     @SmallTest
     public void testTryNowButton() {
         View tryNowButton = mControls.findViewById(R.id.try_now);
+        mModel.set(VideoPlayerProperties.SHOW_TRY_NOW, false);
+        assertEquals(View.GONE, tryNowButton.getVisibility());
         mModel.set(VideoPlayerProperties.SHOW_TRY_NOW, true);
         assertEquals(View.VISIBLE, tryNowButton.getVisibility());
 
+        mModel.set(VideoPlayerProperties.WATCH_STATE_FOR_TRY_NOW, State.PAUSED);
+        mModel.set(VideoPlayerProperties.WATCH_STATE_FOR_TRY_NOW, State.ENDED);
         AtomicBoolean buttonClicked = new AtomicBoolean();
         mModel.set(VideoPlayerProperties.CALLBACK_TRY_NOW, () -> buttonClicked.set(true));
         tryNowButton.performClick();
@@ -132,6 +138,8 @@ public class VideoPlayerViewBinderTest {
     @SmallTest
     public void testWatchNextButton() {
         View watchNextButton = mControls.findViewById(R.id.watch_next);
+        mModel.set(VideoPlayerProperties.SHOW_WATCH_NEXT, false);
+        assertEquals(View.GONE, watchNextButton.getVisibility());
         mModel.set(VideoPlayerProperties.SHOW_WATCH_NEXT, true);
         assertEquals(View.VISIBLE, watchNextButton.getVisibility());
 
@@ -147,6 +155,8 @@ public class VideoPlayerViewBinderTest {
     public void testChangeLanguageButton() {
         TextView changeLanguage = mControls.findViewById(R.id.change_language);
         String languageName = "XYZ";
+        mModel.set(VideoPlayerProperties.SHOW_CHANGE_LANGUAGE, false);
+        assertEquals(View.GONE, changeLanguage.getVisibility());
         mModel.set(VideoPlayerProperties.SHOW_CHANGE_LANGUAGE, true);
         mModel.set(VideoPlayerProperties.CHANGE_LANGUAGE_BUTTON_TEXT, languageName);
         assertEquals(View.VISIBLE, changeLanguage.getVisibility());

chrome/browser/video_tutorials/internal/config_unittest.cc

@@ -16,13 +16,18 @@ namespace video_tutorials {
 TEST(VideoTutorialsConfigTest, FinchConfigEnabled) {
   base::test::ScopedFeatureList feature_list;
   std::map<std::string, std::string> params = {
-      {kBaseURLKey, "https://test.com"}, {kPreferredLocaleConfigKey, "en"}};
+      {kBaseURLKey, "https://test.com"},
+      {kPreferredLocaleConfigKey, "en"},
+      {"fetch_frequency", "10"},
+      {"experiment_tag", "{some_param:some_value}"}};
   feature_list.InitAndEnableFeatureWithParameters(features::kVideoTutorials,
                                                   params);
 
   EXPECT_EQ(Config::GetTutorialsServerURL().spec(),
             "https://test.com/v1/videotutorials");
   EXPECT_EQ(Config::GetDefaultPreferredLocale(), "en");
+  EXPECT_EQ(Config::GetFetchFrequency(), base::TimeDelta::FromDays(10));
+  EXPECT_EQ(Config::GetExperimentTag(), "{some_param:some_value}");
 }
 
 TEST(VideoTutorialsConfigTest, ConfigDefaultParams) {
@@ -32,6 +37,8 @@ TEST(VideoTutorialsConfigTest, ConfigDefaultParams) {
             "https://staging-gsaprototype-pa.sandbox.googleapis.com/v1/"
             "videotutorials");
   EXPECT_EQ(Config::GetDefaultPreferredLocale(), "en");
+  EXPECT_EQ(Config::GetFetchFrequency(), base::TimeDelta::FromDays(15));
+  EXPECT_EQ(Config::GetExperimentTag(), "");
 }
 
 }  // namespace video_tutorials