Enhance chrome.app.window API for shelf integration with restore support

chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc

 // Copyright (c) 2012 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/ui/ash/launcher/app_shortcut_launcher_item_controller.h"
 
 #include <stddef.h>
 
 #include "ash/wm/window_util.h"
 #include "chrome/browser/chromeos/arc/arc_support_host.h"
@@ skipping 91 matching lines @@
   for (size_t i = 0; i < content.size(); i++) {
     Browser* browser = chrome::FindBrowserWithWebContents(content[i]);
     if (browser && browser->window()->GetNativeWindow()->IsVisible())
       return true;
   }
   return false;
 }
 
 void AppShortcutLauncherItemController::Launch(ash::LaunchSource source,
                                                int event_flags) {
-  launcher_controller()->LaunchApp(app_id(), source, event_flags);
+  launcher_controller()->LaunchAppWithLaunchId(app_id(), launch_id(), source,
+                                               event_flags);

[stevenjb, 2016/12/09 17:00:57]

Why use WithLaunchId with an empty launch_id() ?

[Andra Paraschiv, 2016/12/12 14:28:22]

Launch id should be non-empty if the window was created with showInShelf=true
and the id in CreateWindowOptions was set. When the window is pinned, then
closed, the launch_id is stored in the AppShorcutLauncherItem
(https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/launcher/chrome_la...)

[stevenjb, 2016/12/13 19:31:14]

Let me be more clear:

Why not use launcher_controller()->LaunchApp()?

[Andra Paraschiv, 2016/12/14 14:59:38]

In this case, if we use LaunchApp instead of LaunchAppWithLaunchId, the
launch_id parameter that will be passed down the following call list will be
empty:

AppShorcutLauncherItemController::Launch
ChromeLauncherController::LaunchApp(app_id, ...)
LauncherControllerHelper::LaunchApp(app_id, ...)
LauncherControllerHelper::LaunchAppWithLaunchId(app_id, ""/*launch_id*/, ...) <-
in this method, the launch_id value will be used to set the launch_id
application parameter (params.launch_id). This parameter value will be used to
set the launchData.id. 

In order to restore a pinned window that was closed before, we should pass the
launch_id parameter value (that can be non-empty for showInShelf=true windows)
down through the call list to the onLaunched event launchData.id -
https://developer.chrome.com/apps/app_runtime#event-onLaunched. This way, if the
id is not empty, only the current pinned window will be restored and not the
whole set of windows that have the same app_id.

Thank you.

[stevenjb, 2016/12/14 18:40:21]

Ugh. My apologies. I was somehow reading that as LaunchId() not launch_id().
Sorry about that, too many code reviews.

[Andra Paraschiv, 2016/12/15 10:20:15]

There's no problem, Steven, it's good we have this discussions based on the
review in order to understand which it's the right way to include the launch_id
in the codebase.

Thanks, and I also added a unit test for the restore part in this CL -
https://codereview.chromium.org/2297633002/ .   

 }
 
 ash::ShelfItemDelegate::PerformedAction
 AppShortcutLauncherItemController::Activate(ash::LaunchSource source) {
   content::WebContents* content = GetLRUApplication();
   if (!content) {
     if (IsV2App()) {
       // Ideally we come here only once. After that ShellLauncherItemController
       // will take over when the shell window gets opened. However there are
       // apps which take a lot of time for pre-processing (like the files app)
@@ skipping 249 matching lines @@
 
 bool AppShortcutLauncherItemController::AllowNextLaunchAttempt() {
   if (last_launch_attempt_.is_null() ||
       last_launch_attempt_ + base::TimeDelta::FromMilliseconds(
           kClickSuppressionInMS) < base::Time::Now()) {
     last_launch_attempt_ = base::Time::Now();
     return true;
   }
   return false;
 }