Allow Chrome apps to create Ash Panels (apps v2)

Allow Chrome apps to create Ash Panels (apps v2)

Adds ShellPanelAsh and window_type=panel support to ShellWindow

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

[stevenjb, 2012-11-14 23:27:25]

This ready for a preliminary review.

The implementation lacks polish, but it should allow us to get started with test
apps.

A big question I have is whether or not 'type' is the right way to specify panel
layout, and whether or not we want to put that behind a flag, or in an
experimental namespace (not sure if we could do that for a parameter?). Feedback
on that appreciated.

[jeremya, 2012-11-15 01:09:25]

Some general questions:

- how will frameless windows/draggable regions come into this? Is that
supportable in panels?
- do all the app window API calls make sense on panels?
- is there a plan for porting this to other OSes than CrOS?

https://codereview.chromium.org/11363250/diff/4001/ash/shell/panel_window.cc
File ash/shell/panel_window.cc (right):

https://codereview.chromium.org/11363250/diff/4001/ash/shell/panel_window.cc#...
ash/shell/panel_window.cc:79: return new PanelFrameView(widget, true);
It's not clear what this parameter means; perhaps use an enum?

https://codereview.chromium.org/11363250/diff/4001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/11363250/diff/4001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:172: if (params.window_type !=
WINDOW_TYPE_PANEL)
Shouldn't this be == then?

https://codereview.chromium.org/11363250/diff/4001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.h (right):

https://codereview.chromium.org/11363250/diff/4001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.h:50: };
Either this should go inside CreateParams or the Frame enum should come out.

https://codereview.chromium.org/11363250/diff/4001/chrome/common/extensions/a...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/11363250/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/app_window.idl:49: //  'shell' - a shell window
with just a close button (default)
Shell windows now have minimize/maximize buttons too.

[stevenjb (google-dont-use), 2012-11-15 01:15:44]

On Wed, Nov 14, 2012 at 5:09 PM, <jeremya@chromium.org> wrote:

> Some general questions:
>
> - how will frameless windows/draggable regions come into this? Is that
> supportable in panels?
>
I think they should support this, but probably in a follow-up CL


> - do all the app window API calls make sense on panels?
>
Not sure, possibly not. Some properties like 'maximize' certainly do not.


> - is there a plan for porting this to other OSes than CrOS?
>
It depends on what you many b "this". Ash Panels (now AKA Utility Windows)
will only be supported on Ash. At some point we will likely want to add v2
hooks to support Windows Panels.

This reminds me that I need to rename the class to ShellUtilityWindowAsh.

[flackr, 2012-11-15 16:04:17]

https://codereview.chromium.org/11363250/diff/4001/ash/shell/panel_window.cc
File ash/shell/panel_window.cc (right):

https://codereview.chromium.org/11363250/diff/4001/ash/shell/panel_window.cc#...
ash/shell/panel_window.cc:79: return new PanelFrameView(widget, true);
On 2012/11/15 01:09:25, jeremya wrote:
> It's not clear what this parameter means; perhaps use an enum?

Or comment it: /* show_header */ true

https://codereview.chromium.org/11363250/diff/4001/chrome/browser/ui/ash/shel...
File chrome/browser/ui/ash/shell_panel_ash.h (right):

https://codereview.chromium.org/11363250/diff/4001/chrome/browser/ui/ash/shel...
chrome/browser/ui/ash/shell_panel_ash.h:8: #include <string>
I don't see any std::string's, is this necessary?

[Ben Goodger (Google), 2012-11-15 18:25:01]

On Wed, Nov 14, 2012 at 5:09 PM, <jeremya@chromium.org> wrote:

> Some general questions:
>
> - how will frameless windows/draggable regions come into this? Is that
> supportable in panels?
>
Out of scope for the first cut, at least until we figure out the UX a bit
more.


> - is there a plan for porting this to other OSes than CrOS?
>

Eventually, but once we get the core UX figured out a bit better.

-Ben

[stevenjb, 2012-11-16 23:42:40]

PTAL:
* Addressed feedback
* Renamed ShellPanelAsh -> ShellUtilityWindowAsh
("Utility Window" is the new term for Ash Panels, to differentiate them from the
other Panel implementation. I didn't rename the ash code, there is less chance
confusion there, and others potentially working on it)
* Fixed PanelLayoutManager to correctly draw the container frame (this code
needs some re-factoring I think, but at least it now works as designed)

https://chromiumcodereview.appspot.com/11363250/diff/4001/ash/shell/panel_win...
File ash/shell/panel_window.cc (right):

https://chromiumcodereview.appspot.com/11363250/diff/4001/ash/shell/panel_win...
ash/shell/panel_window.cc:79: return new PanelFrameView(widget, true);
On 2012/11/15 16:04:17, flackr wrote:
> On 2012/11/15 01:09:25, jeremya wrote:
> > It's not clear what this parameter means; perhaps use an enum?
> 
> Or comment it: /* show_header */ true
Replaced with enum.

https://chromiumcodereview.appspot.com/11363250/diff/4001/chrome/browser/ui/a...
File chrome/browser/ui/ash/shell_panel_ash.h (right):

https://chromiumcodereview.appspot.com/11363250/diff/4001/chrome/browser/ui/a...
chrome/browser/ui/ash/shell_panel_ash.h:8: #include <string>
On 2012/11/15 16:04:17, flackr wrote:
> I don't see any std::string's, is this necessary?
Should be string16.h, changed

https://chromiumcodereview.appspot.com/11363250/diff/4001/chrome/browser/ui/e...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://chromiumcodereview.appspot.com/11363250/diff/4001/chrome/browser/ui/e...
chrome/browser/ui/extensions/shell_window.cc:172: if (params.window_type !=
WINDOW_TYPE_PANEL)
On 2012/11/15 01:09:25, jeremya wrote:
> Shouldn't this be == then?
Yes. Done.

https://chromiumcodereview.appspot.com/11363250/diff/4001/chrome/browser/ui/e...
File chrome/browser/ui/extensions/shell_window.h (right):

https://chromiumcodereview.appspot.com/11363250/diff/4001/chrome/browser/ui/e...
chrome/browser/ui/extensions/shell_window.h:50: };
On 2012/11/15 01:09:25, jeremya wrote:
> Either this should go inside CreateParams or the Frame enum should come out.

Done.

https://chromiumcodereview.appspot.com/11363250/diff/4001/chrome/common/exten...
File chrome/common/extensions/api/app_window.idl (right):

https://chromiumcodereview.appspot.com/11363250/diff/4001/chrome/common/exten...
chrome/common/extensions/api/app_window.idl:49: //  'shell' - a shell window
with just a close button (default)
On 2012/11/15 01:09:25, jeremya wrote:
> Shell windows now have minimize/maximize buttons too.

Done.

[jeremya, 2012-11-19 00:04:23]

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
File chrome/browser/ui/ash/shell_utility_window_ash.cc (right):

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
chrome/browser/ui/ash/shell_utility_window_ash.cc:49: window_->Init(params);
Bounds should refer to content bounds, not window bounds. (i.e. if the user
specifies width:200,height:200, then window.innerWidth should be 200 and
window.innerHeight should be 200 when the page is created.)

The other window implementations aren't like that yet, but I have patches in the
works to fix them.

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
File chrome/browser/ui/ash/shell_utility_window_ash.h (right):

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
chrome/browser/ui/ash/shell_utility_window_ash.h:25: class PanelHost;
Not used?

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
chrome/browser/ui/ash/shell_utility_window_ash.h:29: // PanelViewAura is used to
display HTML in a Panel window.
Update comment.

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
chrome/browser/ui/ash/shell_utility_window_ash.h:35: const
ShellWindow::CreateParams& create_params);
Indentation.

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:165: if (params.window_type ==
WINDOW_TYPE_PANEL)
#if defined(USE_ASH) here too.

Perhaps on non-ash we should DCHECK(window_type != WINDOW_TYPE_PANEL)?

[stevenjb, 2012-11-19 21:38:43]

PTAL

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
File chrome/browser/ui/ash/shell_utility_window_ash.cc (right):

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
chrome/browser/ui/ash/shell_utility_window_ash.cc:49: window_->Init(params);
On 2012/11/19 00:04:24, jeremya wrote:
> Bounds should refer to content bounds, not window bounds. (i.e. if the user
> specifies width:200,height:200, then window.innerWidth should be 200 and
> window.innerHeight should be 200 when the page is created.)
> 
> The other window implementations aren't like that yet, but I have patches in
the
> works to fix them.

I looked at how this is done in the _views impl in
http://codereview.chromium.org/11369237/. Unfortunately the frame height for
PanelFrameView is dependent upon the size of the assets and difficult to
calculate before it is instantiated.

Instead, I am updating the bounds after initializing the window, when the frame
exists, which seems to work fine.

Done.

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
File chrome/browser/ui/ash/shell_utility_window_ash.h (right):

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
chrome/browser/ui/ash/shell_utility_window_ash.h:25: class PanelHost;
On 2012/11/19 00:04:24, jeremya wrote:
> Not used?

Done.

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
chrome/browser/ui/ash/shell_utility_window_ash.h:29: // PanelViewAura is used to
display HTML in a Panel window.
On 2012/11/19 00:04:24, jeremya wrote:
> Update comment.

Done.

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
chrome/browser/ui/ash/shell_utility_window_ash.h:35: const
ShellWindow::CreateParams& create_params);
On 2012/11/19 00:04:24, jeremya wrote:
> Indentation.

Done.

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://chromiumcodereview.appspot.com/11363250/diff/15001/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:165: if (params.window_type ==
WINDOW_TYPE_PANEL)
On 2012/11/19 00:04:24, jeremya wrote:
> #if defined(USE_ASH) here too.
> 
> Perhaps on non-ash we should DCHECK(window_type != WINDOW_TYPE_PANEL)?

This logic is actually valid on any platform; panels in general do not want to
be activated when shown.

I moved the logic to determine which type of window to create into
NativeShellWindow::Create which is really where it belongs (and removes the
awkward ASH dependency).

[flackr, 2012-11-19 21:59:33]

LGTM. Seems to be something weird going on with the side by side view on
codereview, had to review using the patch form.

[stevenjb, 2012-11-19 22:17:00]

On 2012/11/19 21:59:33, flackr wrote:
> LGTM. Seems to be something weird going on with the side by side view on
> codereview, had to review using the patch form.

Ugh, I've seen that happen before. I'll re-upload the patch shortly.

[stevenjb, 2012-11-19 22:21:18]

OK, diffs should work now. Thanks for letting me know.

[stevenjb, 2012-11-19 23:36:01]

I forgot that I had already done all of the heavy lifting to support multiple
ids for the same app in the launcher, so I went ahead and added that to this CL.

[stevenjb, 2012-11-19 23:38:01]

+sky@ for 	 chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc
and OWNER lgtm for chrome/browser/ui/ash and ash/wm

[sky, 2012-11-20 02:05:05]

Can you add test coverage of the changes to PanelLayoutManager?

https://codereview.chromium.org/11363250/diff/22003/ash/wm/panel_frame_view.cc
File ash/wm/panel_frame_view.cc (right):

https://codereview.chromium.org/11363250/diff/22003/ash/wm/panel_frame_view.c...
ash/wm/panel_frame_view.cc:38: return gfx::Insets(0, 0, title_height, 0);
Seems like this should call into FramePainter since it tracks this information.

https://codereview.chromium.org/11363250/diff/22003/ash/wm/panel_layout_manag...
File ash/wm/panel_layout_manager.cc (right):

https://codereview.chromium.org/11363250/diff/22003/ash/wm/panel_layout_manag...
ash/wm/panel_layout_manager.cc:250: panel,
WINDOW_VISIBILITY_ANIMATION_TYPE_MINIMIZE);
I believe WINDOW_VISIBILITY_ANIMATION_TYPE_MINIMIZE finds the location in the
launcher, but I don't believe there is an item in the launcher for panel. You
sure this does the right thing?

https://codereview.chromium.org/11363250/diff/22003/ash/wm/panel_layout_manag...
ash/wm/panel_layout_manager.cc:259: wm::ActivateWindow(panel);
Isn't this done else where?

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc (right):

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc:27: return
base::IntToString(shell_window->session_id().id());
Do you know this id is never going to conflict with a a non-panel?

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc:31: bool
AppLauncherIdIsForApp(const std::string& app_launcher_id,
Nuke this since I don't believe its used.

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/she...
File chrome/browser/ui/ash/shell_utility_window_ash.h (right):

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/she...
chrome/browser/ui/ash/shell_utility_window_ash.h:31: class ShellUtilityWindowAsh
: public NativeShellWindow,
Isn't there a better way to share code here?

How does this differ than a browser of type panel?

[stevenjb, 2012-11-20 21:20:57]

https://codereview.chromium.org/11363250/diff/22003/ash/wm/panel_frame_view.cc
File ash/wm/panel_frame_view.cc (right):

https://codereview.chromium.org/11363250/diff/22003/ash/wm/panel_frame_view.c...
ash/wm/panel_frame_view.cc:38: return gfx::Insets(0, 0, title_height, 0);
On 2012/11/20 02:05:05, sky wrote:
> Seems like this should call into FramePainter since it tracks this
information.
Done.

https://codereview.chromium.org/11363250/diff/22003/ash/wm/panel_layout_manag...
File ash/wm/panel_layout_manager.cc (right):

https://codereview.chromium.org/11363250/diff/22003/ash/wm/panel_layout_manag...
ash/wm/panel_layout_manager.cc:250: panel,
WINDOW_VISIBILITY_ANIMATION_TYPE_MINIMIZE);
On 2012/11/20 02:05:05, sky wrote:
> I believe WINDOW_VISIBILITY_ANIMATION_TYPE_MINIMIZE finds the location in the
> launcher, but I don't believe there is an item in the launcher for panel. You
> sure this does the right thing?

Ash panels (now aka Utility Windows) do have a launcher icon, that's kind of the
whole point of this :)

We don't call SetChildWindowVisibilityChangesAnimated() for panel_container, so
this doesn't actually trigger any animation, but I went ahead and tested that it
does work if we decide to turn the animation on.

https://codereview.chromium.org/11363250/diff/22003/ash/wm/panel_layout_manag...
ash/wm/panel_layout_manager.cc:259: wm::ActivateWindow(panel);
On 2012/11/20 02:05:05, sky wrote:
> Isn't this done else where?

I did some investigating and determined that this was never getting called,
which seemed wrong. I changed the logic in ShellWindow to call
GetBaseWindow()->Restore() if the window was minimized, which caused this to get
triggered correctly.

In doing so I also decided that we shouldn't be calling ActivateWindow here,
since panels do not activate by default when shown (e.g. when triggered from an
app), we should be doing the activation explicitly in
ShellWindowLauncherController.

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc (right):

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc:27: return
base::IntToString(shell_window->session_id().id());
On 2012/11/20 02:05:05, sky wrote:
> Do you know this id is never going to conflict with a a non-panel?
This uses SessionID which is globally unique across a session.

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc:31: bool
AppLauncherIdIsForApp(const std::string& app_launcher_id,
On 2012/11/20 02:05:05, sky wrote:
> Nuke this since I don't believe its used.

Done.

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/she...
File chrome/browser/ui/ash/shell_utility_window_ash.h (right):

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/she...
chrome/browser/ui/ash/shell_utility_window_ash.h:31: class ShellUtilityWindowAsh
: public NativeShellWindow,
On 2012/11/20 02:05:05, sky wrote:
> Isn't there a better way to share code here?
> 
> How does this differ than a browser of type panel?

Probably, yes. I've been thinking about the best way to do this. Options
include:

0. Keep this independent of ShellWindowViews, the code duplication is fairly
small and it keeps the amount of class hierarchy minimal.
1. Make ShellWindowViews panel-aware and don't create a separate class for
utility windows.
2. Make this class inherit from ShellWindowViews.
3. Create a common class, NativeShellWindowViews, and have ShellWindowViews and
ShellUtilityWindowAsh inherit from that.

I'm leaning towards 3. I'm not particularly fond of 1.

Jeremy, thoughts? 

I'm not sure I understand the second question; ShellWindow is an Apps v2 window
and is pretty much unrelated to Browser. Apps v2 panels, aka Utility Windows,
will also be independent of Browser. Eventually we hope to provide enough hooks
for apps v1 panels on Ash to also use Ash Utility Windows, but that is a
separate task that we can take on once we decide whether or not we actually like
the Utility Window design.

[stevenjb, 2012-11-20 21:21:40]

+jamescook@ for changes to FramePainter

[James Cook, 2012-11-20 22:08:27]

On 2012/11/20 21:21:40, stevenjb (chromium) wrote:
> +jamescook@ for changes to FramePainter

ash/wm/frame_painter.* LGTM with one optional nit

[James Cook, 2012-11-20 22:08:41]

https://codereview.chromium.org/11363250/diff/15004/ash/wm/frame_painter.cc
File ash/wm/frame_painter.cc (right):

https://codereview.chromium.org/11363250/diff/15004/ash/wm/frame_painter.cc#n...
ash/wm/frame_painter.cc:498: return (bottom_y - 1) +
kHeaderContentSeparatorSize;
nit: Consider documenting that the -1 is because the art is created so the
bottom pixel overlaps the separator line.

[sky, 2012-11-20 22:15:51]

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc (right):

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc:27: return
base::IntToString(shell_window->session_id().id());
On 2012/11/20 21:20:58, stevenjb (chromium) wrote:
> On 2012/11/20 02:05:05, sky wrote:
> > Do you know this id is never going to conflict with a a non-panel?
> This uses SessionID which is globally unique across a session.
> 

Indeed. But the return value from this is used in a map shared with the value
returned from 28. My question is can the two ever conflict?

[stevenjb, 2012-11-20 22:29:10]

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc (right):

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc:27: return
base::IntToString(shell_window->session_id().id());
On 2012/11/20 22:15:51, sky wrote:
> On 2012/11/20 21:20:58, stevenjb (chromium) wrote:
> > On 2012/11/20 02:05:05, sky wrote:
> > > Do you know this id is never going to conflict with a a non-panel?
> > This uses SessionID which is globally unique across a session.
> > 
> 
> Indeed. But the return value from this is used in a map shared with the value
> returned from 28. My question is can the two ever conflict?

Ahh. Apologies, excellent point. I'll put a prefix on each of them.

[sky, 2012-11-21 00:39:41]

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/she...
File chrome/browser/ui/ash/shell_utility_window_ash.h (right):

https://codereview.chromium.org/11363250/diff/22003/chrome/browser/ui/ash/she...
chrome/browser/ui/ash/shell_utility_window_ash.h:31: class ShellUtilityWindowAsh
: public NativeShellWindow,
On 2012/11/20 21:20:58, stevenjb (chromium) wrote:
> On 2012/11/20 02:05:05, sky wrote:
> > Isn't there a better way to share code here?
> > 
> > How does this differ than a browser of type panel?
> 
> Probably, yes. I've been thinking about the best way to do this. Options
> include:
> 
> 0. Keep this independent of ShellWindowViews, the code duplication is fairly
> small and it keeps the amount of class hierarchy minimal.
> 1. Make ShellWindowViews panel-aware and don't create a separate class for
> utility windows.
> 2. Make this class inherit from ShellWindowViews.
> 3. Create a common class, NativeShellWindowViews, and have ShellWindowViews
and
> ShellUtilityWindowAsh inherit from that.
> 
> I'm leaning towards 3. I'm not particularly fond of 1.
> 
> Jeremy, thoughts? 
> 
> I'm not sure I understand the second question; ShellWindow is an Apps v2
window
> and is pretty much unrelated to Browser. Apps v2 panels, aka Utility Windows,
> will also be independent of Browser. Eventually we hope to provide enough
hooks
> for apps v1 panels on Ash to also use Ash Utility Windows, but that is a
> separate task that we can take on once we decide whether or not we actually
like
> the Utility Window design.
> 

I like 3 too.

My second question was around whether you could get what you want from a
browser, but it sounds like no.

https://codereview.chromium.org/11363250/diff/15004/ash/wm/panel_layout_manag...
File ash/wm/panel_layout_manager_unittest.cc (right):

https://codereview.chromium.org/11363250/diff/15004/ash/wm/panel_layout_manag...
ash/wm/panel_layout_manager_unittest.cc:285: 
nit: remove one of these lines.

[stevenjb, 2012-11-29 02:30:38]

OK, this has been re-factored with the final result of
https://codereview.chromium.org/11280173/.

Some other changes made this CL a bit simpler, and supporting both panel and
non-panel app windows in NativeAppWindowViews turned out to be less awkward than
I had feared.

PTAL

https://codereview.chromium.org/11363250/diff/15004/ash/wm/panel_layout_manag...
File ash/wm/panel_layout_manager_unittest.cc (right):

https://codereview.chromium.org/11363250/diff/15004/ash/wm/panel_layout_manag...
ash/wm/panel_layout_manager_unittest.cc:285: 
On 2012/11/21 00:39:42, sky wrote:
> nit: remove one of these lines.

Done.

https://codereview.chromium.org/11363250/diff/27001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc (right):

https://codereview.chromium.org/11363250/diff/27001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc:26: return
StringPrintf("panel:%d", shell_window->session_id().id());
Fixed this to make panel identifiers unique. We do not rely on this id matching
shell_window->session_id() anywhere.

[jeremya, 2012-11-29 02:49:04]

https://codereview.chromium.org/11363250/diff/27001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/11363250/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/api/app_window/app_window_api.cc:150:
create_params.window_type = ShellWindow::WINDOW_TYPE_PANEL;
If these panels are only allowed for Talk, we should throw an exception here. Is
that the case?

https://codereview.chromium.org/11363250/diff/27001/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/11363250/diff/27001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/shell_window.cc:91: ShellWindow* window = new
ShellWindow(params.window_type, profile, extension);
Probably better to read this out in Init() rather than adding an argument to the
constructor?

https://codereview.chromium.org/11363250/diff/27001/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/native_app_window_views.cc (right):

https://codereview.chromium.org/11363250/diff/27001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/native_app_window_views.cc:205: // Private
method. TODO(stevenjb): Move this.
Move it where? Why?

https://codereview.chromium.org/11363250/diff/27001/chrome/common/extensions/...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/11363250/diff/27001/chrome/common/extensions/...
chrome/common/extensions/api/app_window.idl:54: //  'panel' - a panel, managed
by the OS (Currently Ash only)
If this is ash-only, the app should be able to detect whether it's available.

[stevenjb, 2012-11-29 03:11:25]

https://codereview.chromium.org/11363250/diff/27001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/11363250/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/api/app_window/app_window_api.cc:150:
create_params.window_type = ShellWindow::WINDOW_TYPE_PANEL;
On 2012/11/29 02:49:05, jeremya wrote:
> If these panels are only allowed for Talk, we should throw an exception here.
Is
> that the case?

In apps v2 panels are not intended to be restricted to Talk, but initially they
should be behind the experimental flag, will add that.

https://codereview.chromium.org/11363250/diff/27001/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/11363250/diff/27001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/shell_window.cc:91: ShellWindow* window = new
ShellWindow(params.window_type, profile, extension);
On 2012/11/29 02:49:05, jeremya wrote:
> Probably better to read this out in Init() rather than adding an argument to
the
> constructor?
Agreed, done.

https://codereview.chromium.org/11363250/diff/27001/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/native_app_window_views.cc (right):

https://codereview.chromium.org/11363250/diff/27001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/native_app_window_views.cc:205: // Private
method. TODO(stevenjb): Move this.
On 2012/11/29 02:49:05, jeremya wrote:
> Move it where? Why?
Clarified the comment. Moving this to match the declaration order makes the diff
seem a lot bigger than it is, I'll do it in a follow-up by itself.

https://codereview.chromium.org/11363250/diff/27001/chrome/common/extensions/...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/11363250/diff/27001/chrome/common/extensions/...
chrome/common/extensions/api/app_window.idl:54: //  'panel' - a panel, managed
by the OS (Currently Ash only)
On 2012/11/29 02:49:05, jeremya wrote:
> If this is ash-only, the app should be able to detect whether it's available.
I modified the comment to also say 'experimental'.
I think there is an open question whether to support a feature like panel
windows only in Ash beyond the 'experimental' stage, at which point we should
determine if/how to query for feature support. Sound reasonable?

[jeremya, 2012-11-29 03:33:04]

lgtm

[James Cook, 2012-11-29 16:52:00]

LGTM for ash/wm/*

https://codereview.chromium.org/11363250/diff/14020/ash/wm/panel_layout_manag...
File ash/wm/panel_layout_manager_unittest.cc (right):

https://codereview.chromium.org/11363250/diff/14020/ash/wm/panel_layout_manag...
ash/wm/panel_layout_manager_unittest.cc:264: TEST_F(PanelLayoutManagerTest,
MinimizeRestorePanel) {
Thanks for adding a test!

[Ben Goodger (Google), 2012-11-29 17:15:38]

https://codereview.chromium.org/11363250/diff/14020/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/native_app_window_views.cc (right):

https://codereview.chromium.org/11363250/diff/14020/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/native_app_window_views.cc:326: #if
defined(USE_ASH)
USE_ASH is not exclusive anymore. I think you'll want to use one of the methods
that determines if this widget is running in ash.

[stevenjb, 2012-11-29 18:43:36]

ptal

https://codereview.chromium.org/11363250/diff/14020/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/native_app_window_views.cc (right):

https://codereview.chromium.org/11363250/diff/14020/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/native_app_window_views.cc:326: #if
defined(USE_ASH)
On 2012/11/29 17:15:39, Ben Goodger (Google) wrote:
> USE_ASH is not exclusive anymore. I think you'll want to use one of the
methods
> that determines if this widget is running in ash.

Done.

[Ben Goodger (Google), 2012-11-29 18:46:16]

lgtm

[commit-bot: I haz the power, 2012-11-29 19:43:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11363250/13023

[commit-bot: I haz the power, 2012-11-30 17:53:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11363250/20087

[commit-bot: I haz the power, 2012-11-30 18:50:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11363250/28018

[commit-bot: I haz the power, 2012-11-30 21:37:17]

Change committed as 170552