Video Tutorials : Fixed language list initialization after a fresh fetch

This CL contains :
1- The list of supported languages weren't updated after a fresh fetch.
This CL fixes it. In general, after a fresh network fetch is completed,
the next call to the service should return the most updated data,
without waiting for a chrome restart.

2- A few minor UX improvements such as increasing close button target
area, increasing header text sizes, fixing close button callback for
language picker etc.

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

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

@@ -11,16 +11,15 @@
     android:layout_width="match_parent"
     android:layout_height="wrap_content"
     android:background="@drawable/hairline_border_card_background"
-    android:paddingTop="@dimen/promo_compact_padding"
-    android:paddingStart="@dimen/promo_compact_padding"
-    android:paddingEnd="@dimen/promo_compact_padding"
-    android:paddingBottom="@dimen/promo_compact_padding">
+    android:paddingStart="12dp"
+    android:paddingBottom="12dp" >
 
     <org.chromium.components.browser_ui.widget.async_image.AsyncImageView
         android:id="@+id/thumbnail"
         android:layout_width="80dp"
         android:layout_height="58dp"
         android:layout_gravity="center_vertical"
+        android:layout_marginTop="12dp"
         android:importantForAccessibility="no"
         app:unavailableSrc="@color/image_loading_color"
         app:waitingSrc="@color/image_loading_color"
@@ -51,14 +50,15 @@
         android:layout_gravity="center_vertical"
         android:layout_marginStart="12dp"
         android:layout_marginEnd="12dp"
+        android:layout_marginTop="12dp"
         android:ellipsize="end"
         android:maxLines="3"
         android:textAppearance="@style/TextAppearance.TextLarge.Primary" />
 
     <org.chromium.ui.widget.ChromeImageButton
         android:id="@+id/close_button"
-        android:layout_height="24dp"
-        android:layout_width="24dp"
+        android:layout_height="48dp"
+        android:layout_width="48dp"
         android:layout_alignParentEnd="true"
         android:background="?attr/selectableItemBackground"
         android:contentDescription="@string/close" 

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

@@ -9,21 +9,20 @@
     android:layout_width="match_parent"
     android:layout_height="wrap_content"
     android:orientation="vertical"
-    android:paddingTop="30dp"
-    android:paddingStart="@dimen/promo_compact_padding"
-    android:paddingEnd="@dimen/promo_compact_padding">
-
+    android:paddingTop="20dp" >
 
     <LinearLayout
         android:layout_width="match_parent"
         android:layout_height="wrap_content"
-        android:orientation="horizontal">
+        android:orientation="horizontal" >
 
         <TextView
             android:layout_width="wrap_content"
             android:layout_height="wrap_content"
+            android:layout_marginStart="12dp"
+            android:layout_gravity="center_vertical"
             android:text="@string/video_tutorials_learn_chrome"
-            android:textAppearance="@style/TextAppearance.TextLarge.Primary" />
+            android:textAppearance="@style/TextAppearance.Headline.Primary" />
 
         <Space
             android:layout_width="0dp"
@@ -32,10 +31,11 @@
 
         <org.chromium.ui.widget.ChromeImageButton
             android:id="@+id/close_button"
-            android:layout_height="24dp"
-            android:layout_width="24dp"
+            android:layout_height="48dp"
+            android:layout_width="48dp"
             android:background="?attr/selectableItemBackground"
             android:contentDescription="@string/close"
+            android:scaleType="center"
             android:src="@drawable/btn_close" />
     </LinearLayout>
 
@@ -43,5 +43,7 @@
         android:id="@+id/recycler_view"
         android:layout_width="match_parent"
         android:layout_height="match_parent"
-        android:layout_marginTop="16dp" />
+        android:layout_marginTop="16dp"
+        android:layout_marginStart="12dp"
+        android:layout_marginEnd="12dp" />
 </LinearLayout>

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

@@ -9,17 +9,16 @@
     android:layout_width="match_parent"
     android:layout_height="match_parent"
     android:background="@color/default_bg_color"
-    android:paddingEnd="12dp"
-    android:paddingTop="12dp"
-    android:paddingStart="16dp" >
+    android:paddingTop="12dp" >
 
     <org.chromium.ui.widget.ChromeImageButton
         android:id="@+id/close_button"
-        android:layout_height="24dp"
-        android:layout_width="24dp"
+        android:layout_height="48dp"
+        android:layout_width="48dp"
         android:layout_alignParentEnd="true"
         android:background="?attr/selectableItemBackground"
         android:contentDescription="@string/close"
+        android:scaleType="center"
         android:src="@drawable/btn_close" />
 
     <TextView
@@ -27,10 +26,11 @@
         android:layout_width="wrap_content"
         android:layout_height="wrap_content"
         android:layout_marginBottom="36dp"
+        android:layout_marginStart="24dp"
         android:layout_above="@id/recycler_view"
         android:layout_centerHorizontal="true"
         android:text="@string/video_tutorials_language_picker_title"
-        android:textAppearance="@style/TextAppearance.TextLarge.Primary" />
+        android:textAppearance="@style/TextAppearance.Headline.Primary" />
 
     <androidx.recyclerview.widget.RecyclerView
         android:id="@+id/recycler_view"

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

@@ -12,22 +12,22 @@
 
     <org.chromium.ui.widget.ChromeImageButton
         android:id="@+id/close_button"
-        android:layout_height="24dp"
-        android:layout_width="24dp"
+        android:layout_height="48dp"
+        android:layout_width="48dp"
         android:layout_alignParentEnd="true"
-        android:layout_marginEnd="16dp"
         android:background="?attr/selectableItemBackground"
         android:contentDescription="@string/close"
+        android:scaleType="center"
         android:src="@drawable/btn_close" />
 
     <org.chromium.ui.widget.ChromeImageButton
         android:id="@+id/share_button"
-        android:layout_height="24dp"
-        android:layout_width="24dp"
+        android:layout_height="48dp"
+        android:layout_width="48dp"
         android:layout_toStartOf="@id/close_button"
-        android:layout_marginEnd="16dp"
         android:background="?attr/selectableItemBackground"
         android:contentDescription="@string/share"
+        android:scaleType="center"
         android:src="@drawable/ic_share_white_24dp" />
 
     <View

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

@@ -73,6 +73,6 @@
         android:ellipsize="end"
         app:layout_column="0"
         app:layout_row="1"
-        android:textAppearance="@style/TextAppearance.TextMedium.Secondary" />
+        android:textAppearance="@style/TextAppearance.TextLarge.Primary" />
 
 </androidx.gridlayout.widget.GridLayout>

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

@@ -143,7 +143,7 @@ class VideoPlayerMediator implements PlaybackStateObserver.Observer {
 
     private void changeLanguage() {
         mModel.set(VideoPlayerProperties.SHOW_LANGUAGE_PICKER, true);
-        mLanguagePicker.showLanguagePicker(this::onLanguageSelected, () -> {} /* closeCallback */);
+        mLanguagePicker.showLanguagePicker(this::onLanguageSelected, this::onLanguagePickerClosed);
         VideoTutorialMetrics.recordUserAction(mTutorial.featureType, UserAction.CHANGE_LANGUAGE);
     }
 
@@ -163,6 +163,10 @@ class VideoPlayerMediator implements PlaybackStateObserver.Observer {
         mVideoTutorialService.getTutorial(mTutorial.featureType, this::startVideo);
     }
 
+    private void onLanguagePickerClosed() {
+        mModel.set(VideoPlayerProperties.SHOW_LANGUAGE_PICKER, false);
+    }
+
     private void tryNow() {
         VideoTutorialMetrics.recordUserAction(mTutorial.featureType, UserAction.TRY_NOW);
         mTryNowCallback.onResult(mTutorial);

chrome/browser/video_tutorials/internal/tutorial_manager.h

@@ -34,8 +34,8 @@ class TutorialManager {
 
   // Saves a fresh set of video tutorials into database. Called after a network
   // fetch.
-  virtual void SaveGroups(std::unique_ptr<std::vector<TutorialGroup>> groups,
-                          SuccessCallback callback) = 0;
+  virtual void SaveGroups(
+      std::unique_ptr<std::vector<TutorialGroup>> groups) = 0;
 
   virtual ~TutorialManager() = default;
 

chrome/browser/video_tutorials/internal/tutorial_manager_impl.cc

@@ -82,12 +82,12 @@ void TutorialManagerImpl::OnInitialDataLoaded(
     bool success,
     std::unique_ptr<std::vector<TutorialGroup>> all_groups) {
   if (all_groups) {
+    supported_languages_.clear();
     for (auto& tutorial_group : *all_groups) {
       supported_languages_.emplace_back(tutorial_group.language);
     }
   }
 
-  DCHECK(!init_success_.has_value());
   init_success_ = success;
 
   // Flush all cached calls in FIFO sequence.
@@ -121,8 +121,7 @@ void TutorialManagerImpl::OnTutorialsLoaded(
 }
 
 void TutorialManagerImpl::SaveGroups(
-    std::unique_ptr<std::vector<TutorialGroup>> groups,
-    SuccessCallback callback) {
+    std::unique_ptr<std::vector<TutorialGroup>> groups) {
   std::vector<std::string> new_locales;
   std::vector<std::pair<std::string, TutorialGroup>> key_entry_pairs;
   for (auto& group : *groups.get()) {
@@ -140,7 +139,9 @@ void TutorialManagerImpl::SaveGroups(
     }
   }
 
-  store_->UpdateAll(key_entry_pairs, keys_to_delete, std::move(callback));
+  store_->UpdateAll(key_entry_pairs, keys_to_delete,
+                    base::BindOnce(&TutorialManagerImpl::OnInitCompleted,
+                                   weak_ptr_factory_.GetWeakPtr()));
 }
 
 void TutorialManagerImpl::MaybeCacheApiCall(base::OnceClosure api_call) {

chrome/browser/video_tutorials/internal/tutorial_manager_impl.h

@@ -32,8 +32,7 @@ class TutorialManagerImpl : public TutorialManager {
   const std::vector<std::string>& GetSupportedLanguages() override;
   base::Optional<std::string> GetPreferredLocale() override;
   void SetPreferredLocale(const std::string& locale) override;
-  void SaveGroups(std::unique_ptr<std::vector<TutorialGroup>> groups,
-                  SuccessCallback callback) override;
+  void SaveGroups(std::unique_ptr<std::vector<TutorialGroup>> groups) override;
 
   void Initialize();
   void OnInitCompleted(bool success);

chrome/browser/video_tutorials/internal/tutorial_manager_impl_unittest.cc

@@ -132,12 +132,8 @@ class TutorialManagerTest : public testing::Test {
   }
 
   void SaveGroups(std::unique_ptr<std::vector<TutorialGroup>> groups) {
-    base::RunLoop loop;
-    manager()->SaveGroups(
-        std::move(groups),
-        base::BindOnce(&TutorialManagerTest::OnComplete, base::Unretained(this),
-                       loop.QuitClosure()));
-    loop.Run();
+    manager()->SaveGroups(std::move(groups));
+    base::RunLoop().RunUntilIdle();
   }
 
  protected:

chrome/browser/video_tutorials/internal/tutorial_service_impl.cc

@@ -80,9 +80,7 @@ void TutorialServiceImpl::OnFetchFinished(
   auto tutorial_groups = std::make_unique<std::vector<TutorialGroup>>();
   TutorialGroupsFromServerResponseProto(&response_proto, tutorial_groups.get());
 
-  auto lambda = [](bool success) {};
-  tutorial_manager_->SaveGroups(std::move(tutorial_groups),
-                                base::BindOnce(std::move(lambda)));
+  tutorial_manager_->SaveGroups(std::move(tutorial_groups));
 }
 
 const std::vector<std::string>& TutorialServiceImpl::GetSupportedLanguages() {