Fixing bug of display orientation setting

(1) Modified code so that tablet
mode updating will enable/disable display
orientation in setting page.

(2) Code refactoring: move all of methods
related with display information in
SystemInfoApi::SystemInfoEventRouter to
DisplayInfoProvider and its subclasses.

In the previous code, tablet mode's change
will not affect display orientation when
setting page has already shown. As result,
when opening setting page then changing
laptop to tablet mode, user is still able
to modify display orientation. After
investigation, the bug reason is that in
ChromeOS change in tablet mode will not
trigger OnDisplayChanged event.

Bug: 889625
Change-Id: Ic526ec54e6c01cd14a0d4a7e883774b9fafed268
Reviewed-on: https://chromium-review.googlesource.com/c/1264544
Commit-Queue: Andrew Xu <andrewxu@chromium.org>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599846}

chrome/browser/extensions/api/system_display/system_display_chromeos_apitest.cc

+// Copyright 2018 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.
+
+#include "ash/public/cpp/ash_switches.h"
+#include "chrome/browser/extensions/extension_apitest.h"
+#include "chrome/browser/extensions/system_display/display_info_provider_chromeos.h"
+#include "extensions/browser/api/system_display/display_info_provider.h"
+#include "extensions/test/extension_test_message_listener.h"
+#include "extensions/test/result_catcher.h"
+
+class SystemDisplayChromeOSApiTest : public extensions::ExtensionApiTest {
+ public:
+  SystemDisplayChromeOSApiTest() = default;
+  ~SystemDisplayChromeOSApiTest() override = default;
+
+  void SetUpDefaultCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(ash::switches::kAshEnableTabletMode);
+    extensions::ExtensionApiTest::SetUpDefaultCommandLine(command_line);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SystemDisplayChromeOSApiTest);
+};
+
+IN_PROC_BROWSER_TEST_F(SystemDisplayChromeOSApiTest,
+                       CheckOnDisplayChangedEvent) {
+  ExtensionTestMessageListener listener_for_extension_ready("ready", false);
+  ASSERT_TRUE(
+      LoadExtension(test_data_dir_.AppendASCII("system_display_chromeos")));
+  ASSERT_TRUE(listener_for_extension_ready.WaitUntilSatisfied());
+
+  extensions::DisplayInfoProviderChromeOS* provider =
+      static_cast<extensions::DisplayInfoProviderChromeOS*>(
+          extensions::DisplayInfoProvider::Get());
+
+  // Change Tablet Mode then ensure that OnDisplayChangedEvent is triggered
+  provider->OnTabletModeToggled(true);
+
+  extensions::ResultCatcher catcher;
+  EXPECT_TRUE(catcher.GetNextResult());
+}

chrome/browser/extensions/system_display/display_info_provider_chromeos.cc

@@ -12,6 +12,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "chrome/browser/ui/ash/tablet_mode_client.h"
 #include "content/public/common/service_manager_connection.h"
 #include "extensions/common/api/system_display.h"
 #include "services/service_manager/public/cpp/connector.h"
@@ -274,7 +275,7 @@ DisplayInfoProviderChromeOS::DisplayInfoProviderChromeOS(
   connector->BindInterface(ash::mojom::kServiceName, &cros_display_config_);
 }
 
-DisplayInfoProviderChromeOS::~DisplayInfoProviderChromeOS() {}
+DisplayInfoProviderChromeOS::~DisplayInfoProviderChromeOS() = default;
 
 void DisplayInfoProviderChromeOS::SetDisplayProperties(
     const std::string& display_id_str,
@@ -595,6 +596,26 @@ void DisplayInfoProviderChromeOS::SetMirrorMode(
           std::move(callback)));
 }
 
+void DisplayInfoProviderChromeOS::StartObserving() {
+  DisplayInfoProvider::StartObserving();
+
+  TabletModeClient* client = TabletModeClient::Get();
+  if (client)
+    client->AddObserver(this);
+}
+
+void DisplayInfoProviderChromeOS::StopObserving() {
+  DisplayInfoProvider::StopObserving();
+
+  TabletModeClient* client = TabletModeClient::Get();
+  if (client)
+    client->RemoveObserver(this);
+}
+
+void DisplayInfoProviderChromeOS::OnTabletModeToggled(bool enabled) {
+  DispatchOnDisplayChangedEvent();
+}
+
 // static
 DisplayInfoProvider* DisplayInfoProvider::Create() {
   std::unique_ptr<service_manager::Connector> connector =

chrome/browser/extensions/system_display/display_info_provider_chromeos.h

@@ -11,6 +11,7 @@
 #include "ash/public/interfaces/cros_display_config.mojom.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "chrome/browser/ui/ash/tablet_mode_client_observer.h"
 #include "extensions/browser/api/system_display/display_info_provider.h"
 
 namespace service_manager {
@@ -19,7 +20,8 @@ class Connector;
 
 namespace extensions {
 
-class DisplayInfoProviderChromeOS : public DisplayInfoProvider {
+class DisplayInfoProviderChromeOS : public DisplayInfoProvider,
+                                    public TabletModeClientObserver {
  public:
   explicit DisplayInfoProviderChromeOS(service_manager::Connector* connector);
   ~DisplayInfoProviderChromeOS() override;
@@ -52,6 +54,11 @@ class DisplayInfoProviderChromeOS : public DisplayInfoProvider {
   bool ClearTouchCalibration(const std::string& id) override;
   void SetMirrorMode(const api::system_display::MirrorModeInfo& info,
                      ErrorCallback callback) override;
+  void StartObserving() override;
+  void StopObserving() override;
+
+  // TabletModeClientObserver implementation.
+  void OnTabletModeToggled(bool enabled) override;
 
  private:
   void CallSetDisplayLayoutInfo(ash::mojom::DisplayLayoutInfoPtr layout_info,

chrome/test/BUILD.gn

@@ -1440,6 +1440,10 @@ test("browser_tests") {
         sources += [ "../browser/extensions/background_app_browsertest.cc" ]
       }
 
+      if (is_chromeos) {
+        sources += [ "../browser/extensions/api/system_display/system_display_chromeos_apitest.cc" ]
+      }
+
       deps += [
         "//chrome/common/extensions/api",
         "//chrome/services/media_gallery_util/public/cpp:browser_tests",

chrome/test/data/extensions/api_test/system_display_chromeos/background.js

+// Copyright 2018 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.
+
+chrome.test.runTests([
+  function waitForDisplayChangedEvent() {
+    chrome.test.listenOnce(chrome.system.display.onDisplayChanged,
+                           function() {});
+  }
+]);
+
+chrome.test.sendMessage("ready");

chrome/test/data/extensions/api_test/system_display_chromeos/manifest.json

+{
+  "name": "ondisplaychangedevent",
+  "manifest_version": 2,
+  "version": "1.0",
+  "description": "Test system.display API",
+  "permissions": ["system.display"],
+  "app": {
+    "background": {
+      "scripts": ["background.js"]
+    }
+  }
+}

extensions/browser/api/system_display/display_info_provider.cc

@@ -6,6 +6,7 @@
 
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "extensions/browser/extensions_browser_client.h"
 #include "extensions/common/api/system_display.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
@@ -121,6 +122,18 @@ void DisplayInfoProvider::GetDisplayLayout(
       FROM_HERE, base::BindOnce(std::move(callback), DisplayLayoutList()));
 }
 
+void DisplayInfoProvider::StartObserving() {
+  display::Screen* screen = display::Screen::GetScreen();
+  if (screen)
+    screen->AddObserver(this);
+}
+
+void DisplayInfoProvider::StopObserving() {
+  display::Screen* screen = display::Screen::GetScreen();
+  if (screen)
+    screen->RemoveObserver(this);
+}
+
 bool DisplayInfoProvider::OverscanCalibrationStart(const std::string& id) {
   return false;
 }
@@ -169,10 +182,32 @@ void DisplayInfoProvider::SetMirrorMode(
       FROM_HERE, base::BindOnce(std::move(callback), "Not supported"));
 }
 
+void DisplayInfoProvider::DispatchOnDisplayChangedEvent() {
+  ExtensionsBrowserClient::Get()->BroadcastEventToRenderers(
+      events::SYSTEM_DISPLAY_ON_DISPLAY_CHANGED,
+      extensions::api::system_display::OnDisplayChanged::kEventName,
+      std::make_unique<base::ListValue>());
+}
+
 void DisplayInfoProvider::UpdateDisplayUnitInfoForPlatform(
     const display::Display& display,
     extensions::api::system_display::DisplayUnitInfo* unit) {
   NOTIMPLEMENTED_LOG_ONCE();
 }
 
+void DisplayInfoProvider::OnDisplayAdded(const display::Display& new_display) {
+  DispatchOnDisplayChangedEvent();
+}
+
+void DisplayInfoProvider::OnDisplayRemoved(
+    const display::Display& old_display) {
+  DispatchOnDisplayChangedEvent();
+}
+
+void DisplayInfoProvider::OnDisplayMetricsChanged(
+    const display::Display& display,
+    uint32_t metrics) {
+  DispatchOnDisplayChangedEvent();
+}
+
 }  // namespace extensions

extensions/browser/api/system_display/display_info_provider.h

@@ -14,6 +14,7 @@
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/optional.h"
+#include "ui/display/display_observer.h"
 
 namespace display {
 class Display;
@@ -36,13 +37,13 @@ struct TouchCalibrationPairQuad;
 // Implementation class for chrome.system.display extension API
 // (system_display_api.cc). Callbacks that provide an error string use an
 // empty string for success.
-class DisplayInfoProvider {
+class DisplayInfoProvider : public display::DisplayObserver {
  public:
   using DisplayUnitInfoList = std::vector<api::system_display::DisplayUnitInfo>;
   using DisplayLayoutList = std::vector<api::system_display::DisplayLayout>;
   using ErrorCallback = base::OnceCallback<void(base::Optional<std::string>)>;
 
-  virtual ~DisplayInfoProvider();
+  ~DisplayInfoProvider() override;
 
   // Returns a pointer to DisplayInfoProvider or null if Create() or
   // InitializeForTesting() have not been called yet.
@@ -82,6 +83,10 @@ class DisplayInfoProvider {
   virtual void GetDisplayLayout(
       base::OnceCallback<void(DisplayLayoutList result)> callback);
 
+  // Start/Stop observing display state change
+  virtual void StartObserving();
+  virtual void StopObserving();
+
   // Implements overscan calibration methods. See system_display.idl. These
   // return false if |id| is invalid.
   virtual bool OverscanCalibrationStart(const std::string& id);
@@ -113,6 +118,9 @@ class DisplayInfoProvider {
  protected:
   DisplayInfoProvider();
 
+  // Trigger OnDisplayChangedEvent
+  void DispatchOnDisplayChangedEvent();
+
   // Create a DisplayUnitInfo from a display::Display for implementations of
   // GetAllDisplaysInfo()
   static api::system_display::DisplayUnitInfo CreateDisplayUnitInfo(
@@ -128,6 +136,12 @@ class DisplayInfoProvider {
       const display::Display& display,
       api::system_display::DisplayUnitInfo* unit);
 
+  // DisplayObserver
+  void OnDisplayAdded(const display::Display& new_display) override;
+  void OnDisplayRemoved(const display::Display& old_display) override;
+  void OnDisplayMetricsChanged(const display::Display& display,
+                               uint32_t metrics) override;
+
   DISALLOW_COPY_AND_ASSIGN(DisplayInfoProvider);
 };
 

extensions/browser/api/system_info/system_info_api.cc

@@ -8,6 +8,7 @@
 
 #include <memory>
 #include <set>
+#include <unordered_map>
 #include <utility>
 
 #include "base/bind.h"
@@ -20,6 +21,7 @@
 #include "components/storage_monitor/storage_info.h"
 #include "components/storage_monitor/storage_monitor.h"
 #include "content/public/browser/browser_thread.h"
+#include "extensions/browser/api/system_display/display_info_provider.h"
 #include "extensions/browser/api/system_storage/storage_info_provider.h"
 #include "extensions/browser/extensions_browser_client.h"
 #include "extensions/common/api/system_display.h"
@@ -33,6 +35,8 @@ using api::system_storage::StorageUnitInfo;
 using content::BrowserThread;
 using storage_monitor::StorageMonitor;
 
+typedef std::set<const content::BrowserContext*> BrowserContextSet;
+
 namespace system_display = api::system_display;
 namespace system_storage = api::system_storage;
 
@@ -49,8 +53,7 @@ bool IsSystemStorageEvent(const std::string& event_name) {
 
 // Event router for systemInfo API. It is a singleton instance shared by
 // multiple profiles.
-class SystemInfoEventRouter : public display::DisplayObserver,
-                              public storage_monitor::RemovableStorageObserver {
+class SystemInfoEventRouter : public storage_monitor::RemovableStorageObserver {
  public:
   static SystemInfoEventRouter* GetInstance();
 
@@ -58,16 +61,16 @@ class SystemInfoEventRouter : public display::DisplayObserver,
   ~SystemInfoEventRouter() override;
 
   // Add/remove event listener for the |event_name| event.
-  void AddEventListener(const std::string& event_name);
-  void RemoveEventListener(const std::string& event_name);
+  void AddEventListener(const content::BrowserContext* context,
+                        const std::string& event_name);
+  void RemoveEventListener(const content::BrowserContext* context,
+                           const std::string& event_name);
 
- private:
-  // display::DisplayObserver:
-  void OnDisplayAdded(const display::Display& new_display) override;
-  void OnDisplayRemoved(const display::Display& old_display) override;
-  void OnDisplayMetricsChanged(const display::Display& display,
-                               uint32_t metrics) override;
+  // |context| is the pointer to BrowserContext of one SystemInfoAPI instance.
+  // Remove event listeners which the SystemInfoAPI instance adds.
+  void ShutdownSystemInfoAPI(const content::BrowserContext* context);
 
+ private:
   // RemovableStorageObserver implementation.
   void OnRemovableStorageAttached(
       const storage_monitor::StorageInfo& info) override;
@@ -80,11 +83,12 @@ class SystemInfoEventRouter : public display::DisplayObserver,
                      const std::string& event_name,
                      std::unique_ptr<base::ListValue> args);
 
-  // Called to dispatch the systemInfo.display.onDisplayChanged event.
-  void OnDisplayChanged();
+  void AddEventListenerInternal(const std::string& event_name);
+  void RemoveEventListenerInternal(const std::string& event_name);
 
-  // Used to record the event names being watched.
-  std::multiset<std::string> watching_event_set_;
+  // Maps event names to the set of BrowserContexts which have SystemInfoAPI
+  // instances that listen to that event.
+  std::unordered_map<std::string, BrowserContextSet> watched_events_;
 
   bool has_storage_monitor_observer_;
 
@@ -111,54 +115,39 @@ SystemInfoEventRouter::~SystemInfoEventRouter() {
   }
 }
 
-void SystemInfoEventRouter::AddEventListener(const std::string& event_name) {
+void SystemInfoEventRouter::AddEventListener(
+    const content::BrowserContext* context,
+    const std::string& event_name) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  watching_event_set_.insert(event_name);
-  if (watching_event_set_.count(event_name) > 1)
-    return;
+  BrowserContextSet& context_set = watched_events_[event_name];
 
-  if (IsDisplayChangedEvent(event_name)) {
-    display::Screen* screen = display::Screen::GetScreen();
-    if (screen)
-      screen->AddObserver(this);
-  }
+  // Indicate whether there has been any listener listening to the
+  // |event_name| event.
+  const bool not_watched_before = context_set.empty();
 
-  if (IsSystemStorageEvent(event_name)) {
-    if (!has_storage_monitor_observer_) {
-      has_storage_monitor_observer_ = true;
-      DCHECK(StorageMonitor::GetInstance()->IsInitialized());
-      StorageMonitor::GetInstance()->AddObserver(this);
-    }
-  }
+  context_set.insert(context);
+  if (not_watched_before)
+    AddEventListenerInternal(event_name);
 }
 
-void SystemInfoEventRouter::RemoveEventListener(const std::string& event_name) {
+void SystemInfoEventRouter::RemoveEventListener(
+    const content::BrowserContext* context,
+    const std::string& event_name) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  auto it = watching_event_set_.find(event_name);
-  if (it != watching_event_set_.end()) {
-    watching_event_set_.erase(it);
-    if (watching_event_set_.count(event_name) > 0)
+  BrowserContextSet& context_set = watched_events_[event_name];
+  if (!context_set.count(context))
     return;
-  }
-
-  if (IsDisplayChangedEvent(event_name)) {
-    display::Screen* screen = display::Screen::GetScreen();
-    if (screen)
-      screen->RemoveObserver(this);
-  }
+  context_set.erase(context);
+  if (context_set.empty())
+    RemoveEventListenerInternal(event_name);
+}
 
-  if (IsSystemStorageEvent(event_name)) {
-    const std::string& other_event_name =
-        (event_name == system_storage::OnDetached::kEventName)
-            ? system_storage::OnAttached::kEventName
-            : system_storage::OnDetached::kEventName;
-    if (watching_event_set_.count(other_event_name) == 0) {
-      StorageMonitor::GetInstance()->RemoveObserver(this);
-      has_storage_monitor_observer_ = false;
-    }
-  }
+void SystemInfoEventRouter::ShutdownSystemInfoAPI(
+    const content::BrowserContext* context) {
+  for (const auto& map_iter : watched_events_)
+    RemoveEventListener(context, map_iter.first);
 }
 
 void SystemInfoEventRouter::OnRemovableStorageAttached(
@@ -183,42 +172,57 @@ void SystemInfoEventRouter::OnRemovableStorageDetached(
                 system_storage::OnDetached::kEventName, std::move(args));
 }
 
-void SystemInfoEventRouter::OnDisplayAdded(
-    const display::Display& new_display) {
-  OnDisplayChanged();
+void SystemInfoEventRouter::DispatchEvent(
+    events::HistogramValue histogram_value,
+    const std::string& event_name,
+    std::unique_ptr<base::ListValue> args) {
+  ExtensionsBrowserClient::Get()->BroadcastEventToRenderers(
+      histogram_value, event_name, std::move(args));
 }
 
-void SystemInfoEventRouter::OnDisplayRemoved(
-    const display::Display& old_display) {
-  OnDisplayChanged();
-}
+void SystemInfoEventRouter::AddEventListenerInternal(
+    const std::string& event_name) {
+  if (IsDisplayChangedEvent(event_name))
+    DisplayInfoProvider::Get()->StartObserving();
 
-void SystemInfoEventRouter::OnDisplayMetricsChanged(
-    const display::Display& display,
-    uint32_t metrics) {
-  OnDisplayChanged();
+  if (!has_storage_monitor_observer_ && IsSystemStorageEvent(event_name)) {
+    has_storage_monitor_observer_ = true;
+    DCHECK(StorageMonitor::GetInstance()->IsInitialized());
+    StorageMonitor::GetInstance()->AddObserver(this);
+  }
 }
 
-void SystemInfoEventRouter::OnDisplayChanged() {
-  std::unique_ptr<base::ListValue> args(new base::ListValue());
-  DispatchEvent(events::SYSTEM_DISPLAY_ON_DISPLAY_CHANGED,
-                system_display::OnDisplayChanged::kEventName, std::move(args));
+void SystemInfoEventRouter::RemoveEventListenerInternal(
+    const std::string& event_name) {
+  if (IsDisplayChangedEvent(event_name))
+    DisplayInfoProvider::Get()->StopObserving();
+
+  if (IsSystemStorageEvent(event_name)) {
+    const std::string& other_event_name =
+        (event_name == system_storage::OnDetached::kEventName)
+            ? system_storage::OnAttached::kEventName
+            : system_storage::OnDetached::kEventName;
+    auto map_iter = watched_events_.find(other_event_name);
+    if ((map_iter == watched_events_.end()) || (map_iter->second).empty()) {
+      StorageMonitor::GetInstance()->RemoveObserver(this);
+      has_storage_monitor_observer_ = false;
+    }
+  }
 }
 
-void SystemInfoEventRouter::DispatchEvent(
-    events::HistogramValue histogram_value,
-    const std::string& event_name,
-    std::unique_ptr<base::ListValue> args) {
-  ExtensionsBrowserClient::Get()->BroadcastEventToRenderers(
-      histogram_value, event_name, std::move(args));
+void AddEventListener(const content::BrowserContext* context,
+                      const std::string& event_name) {
+  SystemInfoEventRouter::GetInstance()->AddEventListener(context, event_name);
 }
 
-void AddEventListener(const std::string& event_name) {
-  SystemInfoEventRouter::GetInstance()->AddEventListener(event_name);
+void RemoveEventListener(const content::BrowserContext* context,
+                         const std::string& event_name) {
+  SystemInfoEventRouter::GetInstance()->RemoveEventListener(context,
+                                                            event_name);
 }
 
-void RemoveEventListener(const std::string& event_name) {
-  SystemInfoEventRouter::GetInstance()->RemoveEventListener(event_name);
+void ShutdownSystemInfoAPI(const content::BrowserContext* context) {
+  SystemInfoEventRouter::GetInstance()->ShutdownSystemInfoAPI(context);
 }
 
 }  // namespace
@@ -246,23 +250,24 @@ SystemInfoAPI::~SystemInfoAPI() {
 
 void SystemInfoAPI::Shutdown() {
   EventRouter::Get(browser_context_)->UnregisterObserver(this);
+  ShutdownSystemInfoAPI(browser_context_);
 }
 
 void SystemInfoAPI::OnListenerAdded(const EventListenerInfo& details) {
   if (IsSystemStorageEvent(details.event_name)) {
-    StorageMonitor::GetInstance()->EnsureInitialized(
-        base::Bind(&AddEventListener, details.event_name));
+    StorageMonitor::GetInstance()->EnsureInitialized(base::BindRepeating(
+        &AddEventListener, browser_context_, details.event_name));
   } else {
-    AddEventListener(details.event_name);
+    AddEventListener(browser_context_, details.event_name);
   }
 }
 
 void SystemInfoAPI::OnListenerRemoved(const EventListenerInfo& details) {
   if (IsSystemStorageEvent(details.event_name)) {
-    StorageMonitor::GetInstance()->EnsureInitialized(
-        base::Bind(&RemoveEventListener, details.event_name));
+    StorageMonitor::GetInstance()->EnsureInitialized(base::BindRepeating(
+        &RemoveEventListener, browser_context_, details.event_name));
   } else {
-    RemoveEventListener(details.event_name);
+    RemoveEventListener(browser_context_, details.event_name);
   }
 }