Add support for managing active state of platform apps

Add support for managing active state of platform apps


BUG=131878
TEST=Existing and new LauncherPlatformAppBrowserTest.*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=141457

[dimich, 2012-06-09 00:53:17]

Is there a design doc or anything describing what is being implemented? Just
curious. The title sounds too generic for what the patch seems to be?

[DaveMoore, 2012-06-09 15:54:20]

On 2012/06/09 00:53:17, dimich wrote:
> Is there a design doc or anything describing what is being implemented? Just
> curious. The title sounds too generic for what the patch seems to be?

The functionality itself isn't that complicated. The patch sprawled a bit
because for consistency I wanted the GetNativeWindow() method to be const. That
can be done separately if it helps.

This is just about showing the active bar under the icon when a platform app
window is active. We currently only do it for browser windows, and this extends
that UX for platform apps, which can be launcher either from the app list or
from a shortcut. Coming next will be something that lets the user refocus a
running platform app when the icon is clicked.

I'm not sure of the visibility of this document, but this describes what we're
doing:
https://docs.google.com/a/google.com/document/d/1s6rvMfDvS6l2MbSub4vijZEMxrW0...

[sky, 2012-06-11 17:47:38]

http://codereview.chromium.org/10534079/diff/10003/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10534079/diff/10003/ash/shell.cc#newcode661
ash/shell.cc:661: aura::client::SetActivationClient(root_window, NULL);
You're moving this because you want the launcher destroyed first, right? Can you
add that to the comment.

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/browser_...
chrome/browser/ui/browser_window.h:99: virtual gfx::NativeWindow
GetNativeWindow() const = 0;
As discussed over IM this shouldn't be const.

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/launcher/chrome_launcher_controller.cc (right):

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/launcher/chrome_launcher_controller.cc:100: }
Remove observers from any windows in window_to_id_map_?

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/launcher/chrome_launcher_controller.cc:546:
ash::LauncherID id = window_to_id_map_[window];
window->RemoveObserver(this) ?

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/launcher/chrome_launcher_controller.cc:546:
ash::LauncherID id = window_to_id_map_[window];
DCHECK(window_to_id_map_.find(window) != window_to_id_map_.end())?

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/launcher/chrome_launcher_controller.h (right):

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/launcher/chrome_launcher_controller.h:204:
aura::Window* active, aura::Window* old_active) OVERRIDE;
nit: each arg on its own line.

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/launcher/chrome_launcher_controller.h:283:
WindowToIDMap window_to_id_map_;
Add a description for this. Especially mention its only used for shell windows.

[DaveMoore, 2012-06-11 18:13:40]

http://codereview.chromium.org/10534079/diff/10003/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10534079/diff/10003/ash/shell.cc#newcode661
ash/shell.cc:661: aura::client::SetActivationClient(root_window, NULL);
On 2012/06/11 17:47:39, sky wrote:
> You're moving this because you want the launcher destroyed first, right? Can
you
> add that to the comment.

Done.

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/browser_...
chrome/browser/ui/browser_window.h:99: virtual gfx::NativeWindow
GetNativeWindow() const = 0;
On 2012/06/11 17:47:39, sky wrote:
> As discussed over IM this shouldn't be const.

Done.

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/launcher/chrome_launcher_controller.cc (right):

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/launcher/chrome_launcher_controller.cc:100: }
On 2012/06/11 17:47:39, sky wrote:
> Remove observers from any windows in window_to_id_map_?

Done.

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/launcher/chrome_launcher_controller.cc:546:
ash::LauncherID id = window_to_id_map_[window];
On 2012/06/11 17:47:39, sky wrote:
> window->RemoveObserver(this) ?

Done.

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/launcher/chrome_launcher_controller.h (right):

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/launcher/chrome_launcher_controller.h:204:
aura::Window* active, aura::Window* old_active) OVERRIDE;
On 2012/06/11 17:47:39, sky wrote:
> nit: each arg on its own line.

Done.

http://codereview.chromium.org/10534079/diff/10003/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/launcher/chrome_launcher_controller.h:283:
WindowToIDMap window_to_id_map_;
On 2012/06/11 17:47:39, sky wrote:
> Add a description for this. Especially mention its only used for shell
windows.

Done.

[sky, 2012-06-11 19:10:41]

Remove GetNativeWindow from BrowserWindow and LGTM

[Mihai Parparita -not on Chrome, 2012-06-13 20:10:13]

http://codereview.chromium.org/10534079/diff/8014/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/10534079/diff/8014/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:109: void
ShellWindow::OnNativeClose() {
Not sure I understand the rationale behind adding OnNativeClose (and moving the
ShellWindowRegistry update to here). It doesn't seem to be called from anywhere
but the ShellWindowViews subclass. Also, it's easy to forget to call it (e.g.
ShellWindowGtk::Close still uses "delete this").

[DaveMoore, 2012-06-13 20:20:27]

http://codereview.chromium.org/10534079/diff/8014/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/10534079/diff/8014/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:109: void
ShellWindow::OnNativeClose() {
The reason is that if you call RemoveShellWindow from the destructor you will
only have access to ShellWindow's state, and not that of the subclasses. The
fact that it's not called from ShellWindowGtk is an oversite.

On 2012/06/13 20:10:13, Mihai Parparita wrote:
> Not sure I understand the rationale behind adding OnNativeClose (and moving
the
> ShellWindowRegistry update to here). It doesn't seem to be called from
anywhere
> but the ShellWindowViews subclass. Also, it's easy to forget to call it (e.g.
> ShellWindowGtk::Close still uses "delete this").

[Mihai Parparita -not on Chrome, 2012-06-13 20:25:56]

On Wed, Jun 13, 2012 at 1:20 PM, <davemoore@chromium.org> wrote:
>
> The reason is that if you call RemoveShellWindow from the destructor you
> will only have access to ShellWindow's state, and not that of the
> subclasses. The fact that it's not called from ShellWindowGtk is an
> oversite.


Makes sense. I can fix ShellWindowGtk (I'm editing that file anyway).

Mihai