Changes in app.window and app.runtime to support lock screen note taking

chrome/browser/chromeos/lock_screen_apps/state_controller.cc

 // Copyright 2017 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 "chrome/browser/chromeos/lock_screen_apps/state_controller.h"
 
 #include <utility>
 
 #include "ash/public/interfaces/constants.mojom.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/memory/ptr_util.h"
 #include "chrome/browser/chromeos/lock_screen_apps/app_manager_impl.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chromeos/chromeos_switches.h"
 #include "components/session_manager/core/session_manager.h"
 #include "content/public/common/service_manager_connection.h"
+#include "extensions/browser/app_window/app_window.h"
+#include "extensions/browser/app_window/native_app_window.h"
+#include "extensions/common/extension.h"
 #include "services/service_manager/public/cpp/connector.h"
 
 using ash::mojom::TrayActionState;
 
 namespace lock_screen_apps {
 
 namespace {
 
 StateController* g_instance = nullptr;
 
 }  // namespace
 
 // static
 bool StateController::IsEnabled() {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
       chromeos::switches::kEnableLockScreenApps);
 }
 
 // static
 StateController* StateController::Get() {
   DCHECK(g_instance || !IsEnabled());
   return g_instance;
 }
 
-StateController::StateController() : binding_(this), session_observer_(this) {
+StateController::StateController()
+    : binding_(this), app_window_observer_(this), session_observer_(this) {
   DCHECK(!g_instance);
   DCHECK(IsEnabled());
 
   g_instance = this;
 }
 
 StateController::~StateController() {
   DCHECK_EQ(g_instance, this);
   g_instance = nullptr;
 }
@@ skipping 58 matching lines @@
   // Update state to launching even if app fails to launch - this is to notify
   // listeners that a lock screen note request was handled.
   UpdateLockScreenNoteState(TrayActionState::kLaunching);
   if (!app_manager_->LaunchNoteTaking())
     UpdateLockScreenNoteState(TrayActionState::kAvailable);
 }
 
 void StateController::OnSessionStateChanged() {
   if (!session_manager::SessionManager::Get()->IsScreenLocked()) {
     app_manager_->Stop();
-    UpdateLockScreenNoteState(TrayActionState::kNotAvailable);
+    ResetNoteTakingWindowAndMoveToNextState(true);

[benwells, 2017/06/13 10:30:46]

Nit: true /* close_window */

[tbarzic, 2017/06/13 19:17:59]

Done.

     return;
   }
 
   // base::Unretained is safe here because |app_manager_| is owned by |this|,
   // and the callback will not be invoked after |app_manager_| goes out of
   // scope.
   app_manager_->Start(
       base::Bind(&StateController::OnNoteTakingAvailabilityChanged,
                  base::Unretained(this)));
   OnNoteTakingAvailabilityChanged();
 }
 
+void StateController::OnAppWindowRemoved(extensions::AppWindow* app_window) {
+  if (note_app_window_ != app_window)
+    return;
+  ResetNoteTakingWindowAndMoveToNextState(false);

[benwells, 2017/06/13 10:30:46]

Nit: false /* close_window */

[tbarzic, 2017/06/13 19:17:59]

Done.

+}
+
+bool StateController::CanCreateAppWindowForAction(

[benwells, 2017/06/13 10:30:46]

This function name is kind of confusing. It is checking that the context, the
extension are correct, but also that a window is currently being launched. This
combination feels strange and the name doesn't cover it all

I'd suggest moving the state check into RegisterAppWindowForAction and renaming
this to IsValidAppForAction.

[tbarzic, 2017/06/13 19:17:59]

So, the main purpose of this method is for ChromeAppWindowClient to check if an
app is allowed to create an app window *before* creating it (notice that
RegisterAppWindowForAction takes app_window as an argument).

So, the idea is to use it as:
if (CanCreateAppWindow(browser_cotnext, app, action)) {
  app_window = CreateAppWindow();
  RegisterAppWindowForAction(app_window, action);
}

Though, replacing CanCreateAppWindowForAction and RegisterAppWindowForAction
with CreateAppWindowForAction(context, app, action, app_delegate) would be a
good alternative (as done in the most recent patch).
WDYT?

+    content::BrowserContext* context,
+    const extensions::Extension* extension,
+    extensions::api::app_runtime::ActionType action) const {
+  if (action != extensions::api::app_runtime::ACTION_TYPE_NEW_NOTE)
+    return false;
+
+  if (GetLockScreenNoteState() != TrayActionState::kLaunching)
+    return false;
+
+  if (!chromeos::ProfileHelper::GetSigninProfile()->IsSameProfile(
+          Profile::FromBrowserContext(context))) {
+    return false;
+  }
+
+  return extension && app_manager_->GetNoteTakingAppId() == extension->id();
+}
+
+bool StateController::RegisterAppWindowForAction(
+    extensions::AppWindow* window,
+    extensions::api::app_runtime::ActionType action) {

[benwells, 2017/06/13 10:30:46]

Is ACTION_TYPE_NEW_NOTE the only type of action this will ever be called with?

[tbarzic, 2017/06/13 19:17:59]

Currently, yes (as NEW_NOTE is the only action that exists).

+  if (!CanCreateAppWindowForAction(window->browser_context(),
+                                   window->GetExtension(), action)) {
+    return false;
+  }
+
+  note_app_window_ = window;
+  app_window_observer_.Add(extensions::AppWindowRegistry::Get(
+      chromeos::ProfileHelper::GetSigninProfile()));
+  UpdateLockScreenNoteState(TrayActionState::kActive);
+  return true;
+}
+
 void StateController::MoveToBackground() {
-  TrayActionState state = GetLockScreenNoteState();
-  if (state != TrayActionState::kActive && state != TrayActionState::kLaunching)
+  if (GetLockScreenNoteState() != TrayActionState::kActive)
     return;
   UpdateLockScreenNoteState(TrayActionState::kBackground);
 }
 
 void StateController::MoveToForeground() {
   if (GetLockScreenNoteState() != TrayActionState::kBackground)
     return;
   UpdateLockScreenNoteState(TrayActionState::kActive);
 }
 
-void StateController::SetLockScreenNoteStateForTesting(
-    ash::mojom::TrayActionState state) {
-  lock_screen_note_state_ = state;
-}
-
 void StateController::OnNoteTakingAvailabilityChanged() {
-  if (!app_manager_->IsNoteTakingAppAvailable()) {
-    UpdateLockScreenNoteState(TrayActionState::kNotAvailable);
+  if (!app_manager_->IsNoteTakingAppAvailable() ||
+      (note_app_window_ && note_app_window_->GetExtension()->id() !=
+                               app_manager_->GetNoteTakingAppId())) {
+    ResetNoteTakingWindowAndMoveToNextState(true);
     return;
   }
 
   if (GetLockScreenNoteState() == TrayActionState::kNotAvailable)
     UpdateLockScreenNoteState(TrayActionState::kAvailable);
 }
 
+void StateController::ResetNoteTakingWindowAndMoveToNextState(
+    bool close_window) {
+  app_window_observer_.RemoveAll();
+
+  if (note_app_window_) {
+    if (close_window && note_app_window_->GetBaseWindow())
+      note_app_window_->GetBaseWindow()->Close();
+    note_app_window_ = nullptr;
+  }
+
+  UpdateLockScreenNoteState(app_manager_->IsNoteTakingAppAvailable()
+                                ? TrayActionState::kAvailable
+                                : TrayActionState::kNotAvailable);
+}
+
 bool StateController::UpdateLockScreenNoteState(TrayActionState state) {
   const TrayActionState old_state = GetLockScreenNoteState();
   if (old_state == state)
     return false;
 
   lock_screen_note_state_ = state;
   NotifyLockScreenNoteStateChanged();
   return true;
 }
 
 void StateController::NotifyLockScreenNoteStateChanged() {
   for (auto& observer : observers_)
     observer.OnLockScreenNoteStateChanged(lock_screen_note_state_);
 
   tray_action_ptr_->UpdateLockScreenNoteState(lock_screen_note_state_);
 }
 
 }  // namespace lock_screen_apps