[Launcher impressions] Fix tablet mode impressions

We currently don't handle tablet mode correctly, because the launcher
is considered 'shown' in fullscreen mode at all times behind the active
app. We need to observe some extra signal of when the launcher is shown
or hidden. Talking to mmourgos@, the AppListControllerObserver is the
right way to do this.

Observer: I've had to lift the AddObserver and RemoveObserver methods
out of the AppListControllerImpl and into the AppListController itself,
so they are visible from chrome.

Impressions logic: When the view state is 'shown', only the chips are
shown. When the state is 'closed', we should set all views to none.

Bug: 1097599
Change-Id: Idf971ccc5a4bb91c6167e234449a8ed6bee564f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2309770Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Thanh Nguyen <thanhdng@chromium.org>
Commit-Queue: Tony Yeoman <tby@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791679}

ash/app_list/app_list_controller_impl.h

@@ -82,6 +82,8 @@ class ASH_EXPORT AppListControllerImpl : public AppListController,
   // AppListController:
   void SetClient(AppListClient* client) override;
   AppListClient* GetClient() override;
+  void AddObserver(AppListControllerObserver* observer) override;
+  void RemoveObserver(AppListControllerObserver* obsever) override;
   void AddItem(std::unique_ptr<AppListItemMetadata> app_item) override;
   void AddItemToFolder(std::unique_ptr<AppListItemMetadata> app_item,
                        const std::string& folder_id) override;
@@ -219,9 +221,6 @@ class ASH_EXPORT AppListControllerImpl : public AppListController,
   int GetShelfSize() override;
   bool IsInTabletMode() override;
 
-  void AddObserver(AppListControllerObserver* observer);
-  void RemoveObserver(AppListControllerObserver* obsever);
-
   // Notifies observers of AppList visibility changes.
   void OnVisibilityChanged(bool visible, int64_t display_id);
   void OnVisibilityWillChange(bool visible, int64_t display_id);

ash/public/cpp/app_list/app_list_controller.h

@@ -18,6 +18,7 @@
 namespace ash {
 
 class AppListClient;
+class AppListControllerObserver;
 
 // An interface implemented in Ash to handle calls from Chrome.
 // These include:
@@ -38,6 +39,9 @@ class ASH_PUBLIC_EXPORT AppListController {
   // Gets the client that handles calls from Ash.
   virtual AppListClient* GetClient() = 0;
 
+  virtual void AddObserver(AppListControllerObserver* observer) = 0;
+  virtual void RemoveObserver(AppListControllerObserver* obsever) = 0;
+
   // Adds an item to AppListModel.
   virtual void AddItem(std::unique_ptr<AppListItemMetadata> app_item) = 0;
 

chrome/browser/ui/app_list/app_list_client_impl.cc

@@ -55,8 +55,9 @@ bool IsTabletMode() {
 }  // namespace
 
 AppListClientImpl::AppListClientImpl()
-    : app_list_notifier_(std::make_unique<AppListNotifierImpl>()),
-      app_list_controller_(ash::AppListController::Get()) {
+    : app_list_controller_(ash::AppListController::Get()),
+      app_list_notifier_(
+          std::make_unique<AppListNotifierImpl>(app_list_controller_)) {
   app_list_controller_->SetClient(this);
   user_manager::UserManager::Get()->AddSessionStateObserver(this);
 

chrome/browser/ui/app_list/app_list_client_impl.h

@@ -179,7 +179,6 @@ class AppListClientImpl
   // callbacks.
   std::map<int, AppListModelUpdater*> profile_model_mappings_;
 
-  std::unique_ptr<AppListNotifierImpl> app_list_notifier_;
   std::unique_ptr<app_list::SearchController> search_controller_;
   std::unique_ptr<AppSyncUIStateWatcher> app_sync_ui_state_watcher_;
 
@@ -188,6 +187,8 @@ class AppListClientImpl
 
   ash::AppListController* app_list_controller_ = nullptr;
 
+  std::unique_ptr<AppListNotifierImpl> app_list_notifier_;
+
   bool app_list_target_visibility_ = false;
   bool app_list_visible_ = false;
 

chrome/browser/ui/app_list/app_list_notifier_impl.cc

@@ -4,8 +4,11 @@
 
 #include "chrome/browser/ui/app_list/app_list_notifier_impl.h"
 
+#include "ash/public/cpp/app_list/app_list_controller.h"
+#include "base/check.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
+#include "ui/display/types/display_constants.h"
 
 namespace {
 
@@ -15,8 +18,18 @@ constexpr base::TimeDelta kImpressionTimer = base::TimeDelta::FromSeconds(1);
 
 }  // namespace
 
-AppListNotifierImpl::AppListNotifierImpl() = default;
-AppListNotifierImpl::~AppListNotifierImpl() = default;
+AppListNotifierImpl::AppListNotifierImpl(
+    ash::AppListController* app_list_controller)
+    : app_list_controller_(app_list_controller) {
+  DCHECK(app_list_controller_);
+  app_list_controller_->AddObserver(this);
+  OnAppListVisibilityWillChange(app_list_controller_->IsVisible(base::nullopt),
+                                display::kInvalidDisplayId);
+}
+
+AppListNotifierImpl::~AppListNotifierImpl() {
+  app_list_controller_->RemoveObserver(this);
+}
 
 void AppListNotifierImpl::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
@@ -75,7 +88,11 @@ void AppListNotifierImpl::NotifyUIStateChanged(ash::AppListViewState view) {
   //
   //  3. kPeeking to kFullscreenAllApps. This doesn't change the displayed
   //     chip results.
-  if (view_ == view ||
+  //
+  //  We should also ignore this if the call comes while the launcher is not
+  //  shown at all. This happens, for example, in the transition between
+  //  clamshell and tablet modes.
+  if (!shown_ || view_ == view ||
       (view_ == ash::AppListViewState::kHalf &&
        view == ash::AppListViewState::kFullscreenSearch) ||
       (view_ == ash::AppListViewState::kPeeking &&
@@ -101,6 +118,21 @@ void AppListNotifierImpl::NotifyUIStateChanged(ash::AppListViewState view) {
   }
 }
 
+void AppListNotifierImpl::OnAppListVisibilityWillChange(bool shown,
+                                                        int64_t display_id) {
+  if (shown_ == shown)
+    return;
+  shown_ = shown;
+
+  if (shown) {
+    DoStateTransition(Location::kChip, State::kShown);
+  } else {
+    DoStateTransition(Location::kChip, State::kNone);
+    DoStateTransition(Location::kList, State::kNone);
+    DoStateTransition(Location::kTile, State::kNone);
+  }
+}
+
 void AppListNotifierImpl::RestartTimer(Location location) {
   if (timers_.find(location) == timers_.end()) {
     timers_[location] = std::make_unique<base::OneShotTimer>();

chrome/browser/ui/app_list/app_list_notifier_impl.h

@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "ash/public/cpp/app_list/app_list_controller_observer.h"
 #include "ash/public/cpp/app_list/app_list_notifier.h"
 #include "ash/public/cpp/app_list/app_list_types.h"
 #include "base/containers/flat_map.h"
@@ -19,6 +20,10 @@ namespace base {
 class OneShotTimer;
 }
 
+namespace ash {
+class AppListController;
+}
+
 // Chrome implementation of the AppListNotifier. This is mainly responsible for
 // translating notifications about launcher UI events - eg. launcher opened,
 // results changed, etc. - into impression, launch, and abandon notifications
@@ -123,9 +128,10 @@ class OneShotTimer;
 // Warning: NotifyResultsUpdated cannot be used as a signal of user actions or
 // UI state. Results can be updated at any time for any UI view, regardless
 // of the state of the launcher or what the user is doing.
-class AppListNotifierImpl : public ash::AppListNotifier {
+class AppListNotifierImpl : public ash::AppListNotifier,
+                            public ash::AppListControllerObserver {
  public:
-  AppListNotifierImpl();
+  explicit AppListNotifierImpl(ash::AppListController* app_list_controller);
   ~AppListNotifierImpl() override;
 
   AppListNotifierImpl(const AppListNotifierImpl&) = delete;
@@ -140,6 +146,9 @@ class AppListNotifierImpl : public ash::AppListNotifier {
   void NotifySearchQueryChanged(const base::string16& query) override;
   void NotifyUIStateChanged(ash::AppListViewState view) override;
 
+  // AppListControllerObserver:
+  void OnAppListVisibilityWillChange(bool shown, int64_t display_id) override;
+
  private:
   // Possible states of the state machine.
   enum class State {
@@ -162,6 +171,8 @@ class AppListNotifierImpl : public ash::AppListNotifier {
   // Handles a finished impression timer for |location|.
   void OnTimerFinished(Location location);
 
+  ash::AppListController* const app_list_controller_;
+
   base::ObserverList<Observer> observers_;
 
   // The current state of each state machine.
@@ -169,7 +180,10 @@ class AppListNotifierImpl : public ash::AppListNotifier {
   // An impression timer for each state machine.
   base::flat_map<Location, std::unique_ptr<base::OneShotTimer>> timers_;
 
-  // The current UI view.
+  // Whether or not the app list is shown.
+  bool shown_ = false;
+  // The current UI view. Can have a non-kClosed value when the app list is not
+  // |shown_| due to tablet mode.
   ash::AppListViewState view_ = ash::AppListViewState::kClosed;
   // The currently shown results for each UI view.
   base::flat_map<Location, std::vector<std::string>> results_;