arc: Support running Arc app in shelf

arc: Support running Arc app in shelf.

This contains refactoring of legacy app window controllers
in order to support an abstract app and base implementation
for running Arc tasks.

BUG=596750
TEST=Manually tested on device with activated experimental
     feature for Arc multi-window on. Running Arc tasks
     are reflected in Shelf and behaves similar to legacy
     extension based platform apps.
TEST=Extended unit_tests pass.
TEST=test_that ${minnie} suite:arc-bvt-cq pass

Committed: https://crrev.com/9cdc060a54f28c4fb3a3915919332197a588c568
Cr-Commit-Position: refs/heads/master@{#383528}

[khmel, 2016-03-22 22:23:50]

Description was changed from

==========
arc: Support running Arc app in shelf

BUG=596750
==========

to

==========
arc: Support running Arc app in shelf.

This contains refactoring of legacy app window controllers
in order to support an abstract app and base implementation
for running Arc tasks.

BUG=596750
TEST=Manually tested on device with activated experimental
     feature for Arc multi-window on. Running Arc tasks
     are reflected in Shelf and behaves similar to legacy
     extension based platform apps.
==========

[khmel, 2016-03-23 01:33:34]

Description was changed from

==========
arc: Support running Arc app in shelf.

This contains refactoring of legacy app window controllers
in order to support an abstract app and base implementation
for running Arc tasks.

BUG=596750
TEST=Manually tested on device with activated experimental
     feature for Arc multi-window on. Running Arc tasks
     are reflected in Shelf and behaves similar to legacy
     extension based platform apps.
==========

to

==========
arc: Support running Arc app in shelf.

This contains refactoring of legacy app window controllers
in order to support an abstract app and base implementation
for running Arc tasks.

BUG=596750
TEST=Manually tested on device with activated experimental
     feature for Arc multi-window on. Running Arc tasks
     are reflected in Shelf and behaves similar to legacy
     extension based platform apps.
TEST=Extended unit_tests pass.
==========

[khmel, 2016-03-23 15:29:29]

khmel@chromium.org changed reviewers:
+ skuhne@chromium.org, xiyuan@chromium.org

[khmel, 2016-03-23 15:29:30]

Hi Stefan and Xiyuan,

PTAL

[xiyuan, 2016-03-23 23:22:53]

Mr4D should drive the review since he is more familiar with the launcher code.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:49:
WindowHasNativeWindow(window));
nit: we can use lambda

e.g.

auto iter = std::find_if(windows_.begin(), windows_.end(),
                         [window](ui::BaseWindow* base_window) {
                           return base_window->GetNativeWindow() == window;
                         });

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:61:
WindowHasNativeWindow(window));
nit: use lambda ?

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:73: iter
!= windows_.end(); ++iter) {
nit: We can use range loop now.

i.e.
for (const auto& window : windows_)

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:98: iter
!= windows_to_close.end(); ++iter) {
nit: range loop?

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:123:
DCHECK(type() == TYPE_APP);
nit: DCHECK_EQ(TYPE_APP, type());

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:142:
return nullptr;
ArcAppWindowLauncherItemController does not overide this. So it will have no
menu. Is it intentional? Or we should just leave old impl here?

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:146:
DCHECK(type() == TYPE_APP);
nit: DCHECK_EQ(TYPE_APP, type());

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:155:
DCHECK(type() == TYPE_APP);
nit: DCHECK_EQ(TYPE_APP, type());

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:73: typedef
std::list<ui::BaseWindow*> WindowList;
nit: using WindowList = std::list<ui::BaseWindow*>;

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:132: if
(window->type() != ui::wm::WINDOW_TYPE_NORMAL)
Does ARC++ provide some way to observe its window? Observing all
WINDOW_TYPE_NORMAL window seems a bit wasteful.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:172:
base::Unretained(this), task_id));
WeakPtr instead of base::Unretained(this) ?

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:239:
owner()->SetItemController(shelf_id, controller);
We should probably update CreateAppLauncherItem and SetItemController to make it
clear about the ownership of |controller| is transferred here.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h:45: typedef
std::map<std::string, ArcAppWindowLauncherItemController*>
nit: prefer "using" to "typedef"

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:39:
iter != windows().end(); ++iter) {
nit: range loop?

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:50: class
ArcAppWindowLauncherController;
nit: not used?

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:664:
views::Widget* CreateAppWindow(std::string* window_app_id) {
Can we use const std::string& instead of std::string*?

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h:69:
typedef std::map<aura::Window*, std::string> WindowToAppShelfIdMap;
nit: using

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc:88:
iter != windows().end(); ++iter) {
nit: range loop

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h:47:
typedef std::map<ui::BaseWindow*, extensions::AppWindow*> WindowToAppWindow;
nit: using

https://codereview.chromium.org/1823923002/diff/40001/components/arc/test/fak...
File components/arc/test/fake_app_instance.h (right):

https://codereview.chromium.org/1823923002/diff/40001/components/arc/test/fak...
components/arc/test/fake_app_instance.h:115: typedef std::map<int32_t,
scoped_ptr<Request>> TaskIdToInfo;
nit: using

https://codereview.chromium.org/1823923002/diff/40001/components/arc/test/fak...
components/arc/test/fake_app_instance.h:127: 
nit: nuke empty line

[khmel, 2016-03-24 16:30:38]

Thanks Xiyuan for review, 

Mr4D, your turn? :)

PTAL

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:49:
WindowHasNativeWindow(window));
On 2016/03/23 23:22:53, xiyuan wrote:
> nit: we can use lambda
> 
> e.g.
> 
> auto iter = std::find_if(windows_.begin(), windows_.end(),
>                          [window](ui::BaseWindow* base_window) {
>                            return base_window->GetNativeWindow() == window;
>                          });

Thanks for idea, also use helper function to reduce code dup.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:61:
WindowHasNativeWindow(window));
On 2016/03/23 23:22:53, xiyuan wrote:
> nit: use lambda ?

Done.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:73: iter
!= windows_.end(); ++iter) {
On 2016/03/23 23:22:52, xiyuan wrote:
> nit: We can use range loop now.
> 
> i.e.
> for (const auto& window : windows_)

Done.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:98: iter
!= windows_to_close.end(); ++iter) {
On 2016/03/23 23:22:52, xiyuan wrote:
> nit: range loop?

Done.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:123:
DCHECK(type() == TYPE_APP);
On 2016/03/23 23:22:53, xiyuan wrote:
> nit: DCHECK_EQ(TYPE_APP, type());

Done.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:142:
return nullptr;
On 2016/03/23 23:22:52, xiyuan wrote:
> ArcAppWindowLauncherItemController does not overide this. So it will have no
> menu. Is it intentional? Or we should just leave old impl here?

For better reading it is better remove it here.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:146:
DCHECK(type() == TYPE_APP);
On 2016/03/23 23:22:53, xiyuan wrote:
> nit: DCHECK_EQ(TYPE_APP, type());

Done.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:155:
DCHECK(type() == TYPE_APP);
On 2016/03/23 23:22:53, xiyuan wrote:
> nit: DCHECK_EQ(TYPE_APP, type());

Done.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h:73: typedef
std::list<ui::BaseWindow*> WindowList;
On 2016/03/23 23:22:53, xiyuan wrote:
> nit: using WindowList = std::list<ui::BaseWindow*>;

Done.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:132: if
(window->type() != ui::wm::WINDOW_TYPE_NORMAL)
On 2016/03/23 23:22:53, xiyuan wrote:
> Does ARC++ provide some way to observe its window? Observing all
> WINDOW_TYPE_NORMAL window seems a bit wasteful.

At this moment no, At my first implementation I added observer to exo/components
where Windows for Arc are actually created but was asked to remove it and use
window listeners instead (similar to components/arc/ime/arc_ime_service.cc).
IMHO it may be good TODO to implement class to observe Arc windows once it is
used in several places.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:172:
base::Unretained(this), task_id));
On 2016/03/23 23:22:53, xiyuan wrote:
> WeakPtr instead of base::Unretained(this) ?

Done.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:239:
owner()->SetItemController(shelf_id, controller);
On 2016/03/23 23:22:53, xiyuan wrote:
> We should probably update CreateAppLauncherItem and SetItemController to make
it
> clear about the ownership of |controller| is transferred here.

Actually owner of this is ShelfItemDelegateManager and we have comment already:
// Controller items in this map are owned by |ShelfItemDelegateManager|.
I agree that ownership is not clear in this case, but this is as ash implemented
and changing this probably out of this scope.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h:45: typedef
std::map<std::string, ArcAppWindowLauncherItemController*>
On 2016/03/23 23:22:53, xiyuan wrote:
> nit: prefer "using" to "typedef"

Done.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:39:
iter != windows().end(); ++iter) {
On 2016/03/23 23:22:53, xiyuan wrote:
> nit: range loop?

Done.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:50: class
ArcAppWindowLauncherController;
On 2016/03/23 23:22:53, xiyuan wrote:
> nit: not used?

Done.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:664:
views::Widget* CreateAppWindow(std::string* window_app_id) {
On 2016/03/23 23:22:53, xiyuan wrote:
> Can we use const std::string& instead of std::string*?

I would do this, but std::string* is required for
exo::ShellSurface::SetApplicationId (string* is used as aura::Window property)

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.h:69:
typedef std::map<aura::Window*, std::string> WindowToAppShelfIdMap;
On 2016/03/23 23:22:53, xiyuan wrote:
> nit: using

Done.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc:88:
iter != windows().end(); ++iter) {
On 2016/03/23 23:22:53, xiyuan wrote:
> nit: range loop

Done.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h:47:
typedef std::map<ui::BaseWindow*, extensions::AppWindow*> WindowToAppWindow;
On 2016/03/23 23:22:53, xiyuan wrote:
> nit: using

Done.

https://codereview.chromium.org/1823923002/diff/40001/components/arc/test/fak...
File components/arc/test/fake_app_instance.h (right):

https://codereview.chromium.org/1823923002/diff/40001/components/arc/test/fak...
components/arc/test/fake_app_instance.h:115: typedef std::map<int32_t,
scoped_ptr<Request>> TaskIdToInfo;
On 2016/03/23 23:22:53, xiyuan wrote:
> nit: using

Done.

https://codereview.chromium.org/1823923002/diff/40001/components/arc/test/fak...
components/arc/test/fake_app_instance.h:127: 
On 2016/03/23 23:22:53, xiyuan wrote:
> nit: nuke empty line

Done.

[xiyuan, 2016-03-24 16:47:16]

LGTM but let's also hear it from Mr4D.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:132: if
(window->type() != ui::wm::WINDOW_TYPE_NORMAL)
On 2016/03/24 16:30:37, khmel wrote:
> On 2016/03/23 23:22:53, xiyuan wrote:
> > Does ARC++ provide some way to observe its window? Observing all
> > WINDOW_TYPE_NORMAL window seems a bit wasteful.
> 
> At this moment no, At my first implementation I added observer to
exo/components
> where Windows for Arc are actually created but was asked to remove it and use
> window listeners instead (similar to components/arc/ime/arc_ime_service.cc).
> IMHO it may be good TODO to implement class to observe Arc windows once it is
> used in several places.

Acknowledged.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:239:
owner()->SetItemController(shelf_id, controller);
On 2016/03/24 16:30:37, khmel wrote:
> On 2016/03/23 23:22:53, xiyuan wrote:
> > We should probably update CreateAppLauncherItem and SetItemController to
make
> it
> > clear about the ownership of |controller| is transferred here.
> 
> Actually owner of this is ShelfItemDelegateManager and we have comment
already:
> // Controller items in this map are owned by |ShelfItemDelegateManager|.
> I agree that ownership is not clear in this case, but this is as ash
implemented
> and changing this probably out of this scope.

Acknowledged.

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:664:
views::Widget* CreateAppWindow(std::string* window_app_id) {
On 2016/03/24 16:30:38, khmel wrote:
> On 2016/03/23 23:22:53, xiyuan wrote:
> > Can we use const std::string& instead of std::string*?
> 
> I would do this, but std::string* is required for
> exo::ShellSurface::SetApplicationId (string* is used as aura::Window property)

What is the difference if we do exo::ShellSurface::SetApplicationId(...,
&window_app_id)? ShellSurface already does something like this.

[khmel, 2016-03-24 16:58:42]

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:664:
views::Widget* CreateAppWindow(std::string* window_app_id) {
On 2016/03/24 16:47:16, xiyuan wrote:
> On 2016/03/24 16:30:38, khmel wrote:
> > On 2016/03/23 23:22:53, xiyuan wrote:
> > > Can we use const std::string& instead of std::string*?
> > 
> > I would do this, but std::string* is required for
> > exo::ShellSurface::SetApplicationId (string* is used as aura::Window
property)
> 
> What is the difference if we do exo::ShellSurface::SetApplicationId(...,
> &window_app_id)? ShellSurface already does something like this.

This is code:
void ShellSurface::SetApplicationId(aura::Window* window,
                                    std::string* application_id) {
  window->SetProperty(kApplicationIdKey, application_id);
}

std::string* is pointer and someone must own this.


ShellSurface does
  SetApplicationId(widget_->GetNativeWindow(), &application_id_);
where application_id_ is owned by ShellSurface itself.

If I use const std::string& (actually std::string&) someone may think to pass
local variable that can go out of scope and be deleted while in use.

[xiyuan, 2016-03-24 17:03:48]

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/1823923002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:664:
views::Widget* CreateAppWindow(std::string* window_app_id) {
On 2016/03/24 16:58:42, khmel wrote:
> On 2016/03/24 16:47:16, xiyuan wrote:
> > On 2016/03/24 16:30:38, khmel wrote:
> > > On 2016/03/23 23:22:53, xiyuan wrote:
> > > > Can we use const std::string& instead of std::string*?
> > > 
> > > I would do this, but std::string* is required for
> > > exo::ShellSurface::SetApplicationId (string* is used as aura::Window
> property)
> > 
> > What is the difference if we do exo::ShellSurface::SetApplicationId(...,
> > &window_app_id)? ShellSurface already does something like this.
> 
> This is code:
> void ShellSurface::SetApplicationId(aura::Window* window,
>                                     std::string* application_id) {
>   window->SetProperty(kApplicationIdKey, application_id);
> }
> 
> std::string* is pointer and someone must own this.
> 
> 
> ShellSurface does
>   SetApplicationId(widget_->GetNativeWindow(), &application_id_);
> where application_id_ is owned by ShellSurface itself.
> 
> If I use const std::string& (actually std::string&) someone may think to pass
> local variable that can go out of scope and be deleted while in use.

Passing it as a pointer does not change the fact that this can happen. But since
we cannot use "const", it's okay to use pointer.

[Mr4D (OOO till 08-26), 2016-03-24 23:03:46]

I looked at everything up and down several times and it looks good.

(Next time please: dissect this into smaller patches.. - I know I tend to do the
same thing, but... :) )

Please address the few comments and then lgtm!

https://codereview.chromium.org/1823923002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/1823923002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:19:
#include "ui/wm/public/activation_client.h"
You can remove the include since it isn't used anymore.

https://codereview.chromium.org/1823923002/diff/60001/components/arc/test/fak...
File components/arc/test/fake_app_instance.cc (right):

https://codereview.chromium.org/1823923002/diff/60001/components/arc/test/fak...
components/arc/test/fake_app_instance.cc:151: }
Just checking - does a "non callback" in case of an error stall the test?

[khmel, 2016-03-24 23:44:17]

Description was changed from

==========
arc: Support running Arc app in shelf.

This contains refactoring of legacy app window controllers
in order to support an abstract app and base implementation
for running Arc tasks.

BUG=596750
TEST=Manually tested on device with activated experimental
     feature for Arc multi-window on. Running Arc tasks
     are reflected in Shelf and behaves similar to legacy
     extension based platform apps.
TEST=Extended unit_tests pass.
==========

to

==========
arc: Support running Arc app in shelf.

This contains refactoring of legacy app window controllers
in order to support an abstract app and base implementation
for running Arc tasks.

BUG=596750
TEST=Manually tested on device with activated experimental
     feature for Arc multi-window on. Running Arc tasks
     are reflected in Shelf and behaves similar to legacy
     extension based platform apps.
TEST=Extended unit_tests pass.
TEST=test_that ${minnie} suite:arc-bvt-cq pass
==========

[khmel, 2016-03-24 23:56:52]

khmel@chromium.org changed reviewers:
+ dcheng@chromium.org, lhchavez@chromium.org

[khmel, 2016-03-24 23:56:54]

Thanks Stefan for review!
I will keep your suggestion in mind and will try to split CL to smaller parts.
But this particular one was not so easy to split.

Adding Luis and Daniel.
You may want not to go through whole CL,
Following is required your approval:
components/arc/common/app.mojom
components/arc/test/fake_app_instance.cc
components/arc/test/fake_app_instance.h

and mojo call is used from:
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc

Thanks,
PTAL

https://codereview.chromium.org/1823923002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/1823923002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:19:
#include "ui/wm/public/activation_client.h"
On 2016/03/24 23:03:46, Mr4D wrote:
> You can remove the include since it isn't used anymore.

Done.

https://codereview.chromium.org/1823923002/diff/60001/components/arc/test/fak...
File components/arc/test/fake_app_instance.cc (right):

https://codereview.chromium.org/1823923002/diff/60001/components/arc/test/fak...
components/arc/test/fake_app_instance.cc:151: }
On 2016/03/24 23:03:46, Mr4D wrote:
> Just checking - does a "non callback" in case of an error stall the test?

I like your question :) Actually it does not stall but mojo is not happy and
crashes with assert that callback is not called. So, we have to call. In case of
not-found, I call empty package name and activity.

[Luis Héctor Chávez, 2016-03-25 16:12:25]

lgtm with one nit.

https://codereview.chromium.org/1823923002/diff/80001/components/arc/test/fak...
File components/arc/test/fake_app_instance.h (right):

https://codereview.chromium.org/1823923002/diff/80001/components/arc/test/fak...
components/arc/test/fake_app_instance.h:89: void SetTaskInfo(int task_id,
nit: int32_t for consistency.

[dcheng, 2016-03-26 09:02:31]

mojom LGTM

https://codereview.chromium.org/1823923002/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/1823923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:32: return
std::find_if(windows_.begin(), windows_.end(),
Nit: #include <algorithm> for find_if.

https://codereview.chromium.org/1823923002/diff/80001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/1823923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc:87:
for (const auto& window : windows()) {
Nit: const auto*

https://codereview.chromium.org/1823923002/diff/80001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/1823923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h:36:
const ui::Event& eent) override;
Nit: event

[khmel, 2016-03-28 17:17:31]

Thank you for review.

https://codereview.chromium.org/1823923002/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/1823923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:32: return
std::find_if(windows_.begin(), windows_.end(),
On 2016/03/26 09:02:31, dcheng wrote:
> Nit: #include <algorithm> for find_if.

Done.

https://codereview.chromium.org/1823923002/diff/80001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/1823923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc:87:
for (const auto& window : windows()) {
On 2016/03/26 09:02:31, dcheng wrote:
> Nit: const auto*

Done.

https://codereview.chromium.org/1823923002/diff/80001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/1823923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h:36:
const ui::Event& eent) override;
On 2016/03/26 09:02:31, dcheng wrote:
> Nit: event

Done.

https://codereview.chromium.org/1823923002/diff/80001/components/arc/test/fak...
File components/arc/test/fake_app_instance.h (right):

https://codereview.chromium.org/1823923002/diff/80001/components/arc/test/fak...
components/arc/test/fake_app_instance.h:89: void SetTaskInfo(int task_id,
On 2016/03/25 16:12:25, Luis Héctor Chávez wrote:
> nit: int32_t for consistency.

Done.

[khmel, 2016-03-28 17:17:56]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-03-28 17:17:57]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
skuhne@chromium.org, lhchavez@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1823923002/#ps100001
(title: "rebased + few nits")

[commit-bot: I haz the power, 2016-03-28 17:18:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1823923002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1823923002/100001

[commit-bot: I haz the power, 2016-03-28 18:47:25]

Description was changed from

==========
arc: Support running Arc app in shelf.

This contains refactoring of legacy app window controllers
in order to support an abstract app and base implementation
for running Arc tasks.

BUG=596750
TEST=Manually tested on device with activated experimental
     feature for Arc multi-window on. Running Arc tasks
     are reflected in Shelf and behaves similar to legacy
     extension based platform apps.
TEST=Extended unit_tests pass.
TEST=test_that ${minnie} suite:arc-bvt-cq pass
==========

to

==========
arc: Support running Arc app in shelf.

This contains refactoring of legacy app window controllers
in order to support an abstract app and base implementation
for running Arc tasks.

BUG=596750
TEST=Manually tested on device with activated experimental
     feature for Arc multi-window on. Running Arc tasks
     are reflected in Shelf and behaves similar to legacy
     extension based platform apps.
TEST=Extended unit_tests pass.
TEST=test_that ${minnie} suite:arc-bvt-cq pass
==========

[commit-bot: I haz the power, 2016-03-28 18:47:26]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-28 18:49:12]

Description was changed from

==========
arc: Support running Arc app in shelf.

This contains refactoring of legacy app window controllers
in order to support an abstract app and base implementation
for running Arc tasks.

BUG=596750
TEST=Manually tested on device with activated experimental
     feature for Arc multi-window on. Running Arc tasks
     are reflected in Shelf and behaves similar to legacy
     extension based platform apps.
TEST=Extended unit_tests pass.
TEST=test_that ${minnie} suite:arc-bvt-cq pass
==========

to

==========
arc: Support running Arc app in shelf.

This contains refactoring of legacy app window controllers
in order to support an abstract app and base implementation
for running Arc tasks.

BUG=596750
TEST=Manually tested on device with activated experimental
     feature for Arc multi-window on. Running Arc tasks
     are reflected in Shelf and behaves similar to legacy
     extension based platform apps.
TEST=Extended unit_tests pass.
TEST=test_that ${minnie} suite:arc-bvt-cq pass

Committed: https://crrev.com/9cdc060a54f28c4fb3a3915919332197a588c568
Cr-Commit-Position: refs/heads/master@{#383528}
==========

[commit-bot: I haz the power, 2016-03-28 18:49:14]

Patchset 6 (id:??) landed as
https://crrev.com/9cdc060a54f28c4fb3a3915919332197a588c568
Cr-Commit-Position: refs/heads/master@{#383528}