Add app_list_presenterr pointer in app_list_presenter_delegate_mus to use dismiss function

Duplicated Issue: 2434573004
Add app_list_presenterr pointer in app_list_presenter_delegate_mus to use dismiss function

Add mouse/touch capture function.
Exploring how to close single widget.

BUG=611447

[mfomitchev, 2016-09-23 19:47:14]

mfomitchev@chromium.org changed reviewers:
+ mfomitchev@chromium.org

[mfomitchev, 2016-09-23 19:47:15]

https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right):

https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:14: #include
"ash/common/shelf/wm_shelf.h"
We shouldn't depend on any of the ash classes. With Mustash, ash and chrome are
running in a separate processes. This delegate is in the chrome (browser)
process, so it shouldn't depend on anything in ash or try to communicate with
any ash classes directly.

https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:56: // 
views::WindowManagerConnection::Get()
I think we do want this?

https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:88:
->AddPointerWatcher(this, true);
Any reason we passing true for |wants_moves|? I think are only interested in
touches and mouse clicks.

https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:103:
presenter_->Dismiss();
This will cause an infinite loop, since Dismiss() calls OnDismiss(). Leave this
the way it was. Instead call presenter_->Dismiss() in OnPointerEventObserved()

https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:128:
this->OnDismissed();
presenter_->Dismiss()

[thanhph, 2016-09-23 19:53:35]

On 2016/09/23 19:47:15, mfomitchev wrote:
>
https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
> File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc
(right):
> 
>
https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
> chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:14: #include
> "ash/common/shelf/wm_shelf.h"
> We shouldn't depend on any of the ash classes. With Mustash, ash and chrome
are
> running in a separate processes. This delegate is in the chrome (browser)
> process, so it shouldn't depend on anything in ash or try to communicate with
> any ash classes directly.

Good point. I forgot to remove this.

> 
>
https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
> chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:56: // 
> views::WindowManagerConnection::Get()
> I think we do want this?

Yes..I'll add this back. I don't know how to test this function yet.
> 
>
https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
> chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:88:
> ->AddPointerWatcher(this, true);
> Any reason we passing true for |wants_moves|? I think are only interested in
> touches and mouse clicks.

That makes sense, will make the code faster by setting it to false.

> 
>
https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
> chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:103:
> presenter_->Dismiss();
> This will cause an infinite loop, since Dismiss() calls OnDismiss(). Leave
this
> the way it was. Instead call presenter_->Dismiss() in OnPointerEventObserved()
> 
>
https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
> chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:128:
> this->OnDismissed();
> presenter_->Dismiss()

my function is OnDismissed not OnDismiss so it'll be ok. But I'll remove
function AppListPresenterDelegateMus::OnDismissed() to enhance clarity. Thanks!

[thanhph, 2016-09-29 18:12:08]

thanhph@chromium.org changed reviewers:
+ thanhph@chromium.org

[thanhph, 2016-09-29 18:12:08]

this patch allows closing the app list widget when users click outside of the
widget. Tested on chrome emulator for mouse and touch input

https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right):

https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:14: #include
"ash/common/shelf/wm_shelf.h"
On 2016/09/23 19:47:15, mfomitchev wrote:
> We shouldn't depend on any of the ash classes. With Mustash, ash and chrome
are
> running in a separate processes. This delegate is in the chrome (browser)
> process, so it shouldn't depend on anything in ash or try to communicate with
> any ash classes directly.


Good point. I forget to remove these ash includes.

https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:56: // 
views::WindowManagerConnection::Get()
On 2016/09/23 19:47:15, mfomitchev wrote:
> I think we do want this?

Yes..I'll add this back.

https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:88:
->AddPointerWatcher(this, true);
On 2016/09/23 19:47:15, mfomitchev wrote:
> Any reason we passing true for |wants_moves|? I think are only interested in
> touches and mouse clicks.

That makes sense, will make the code faster by setting it to false

https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:103:
presenter_->Dismiss();
On 2016/09/23 19:47:15, mfomitchev wrote:
> This will cause an infinite loop, since Dismiss() calls OnDismiss(). Leave
this
> the way it was. Instead call presenter_->Dismiss() in OnPointerEventObserved()


my function is OnDismissed not OnDismiss so it'll be ok. But I'll remove
function AppListPresenterDelegateMus::OnDismissed() to enhance clarity.

https://codereview.chromium.org/2364173002/diff/1/chrome/browser/ui/ash/app_l...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:128:
this->OnDismissed();
On 2016/09/23 19:47:15, mfomitchev wrote:
> presenter_->Dismiss()

Thanks.. This is cleaner.

[mfomitchev, 2016-09-29 18:38:26]

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right):

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:11: #include
"ui/views/pointer_watcher.h"
We already include this in .h

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:42: :
view_delegate_factory_(view_delegate_factory), presenter_(presenter) {}
Nit: Would be better to have the argument order to match the init order (which
should also match the declaration order in .h). app_list_presenter_delegate.cc
has presenter_ first, so would be good to do the same here for consistency.

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:45: // Remove
mouse/touch monitor for this delegate
Nit: This comment isn't particularly helpful, I'd just remove it.

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:58: LOG(ERROR)
<< "\n\n!!!!!. Initializing AppListPresenterDelegateMus\n\n\n";
remove

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:76: // add
mouse-touch capture function to this delegate
Nit: This comment isn't particularly helpful, I'd just remove it.

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:82: //
TODO(mfomitchev): Setup dismissing on mouse/touch gesture anywhere outside
You can now remove this TODO :)

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:98: 
remove blank line

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:108: if
(event.type() == ui::ET_TOUCH_PRESSED ||
Nit: Add a comment in the beginning saying that the purpose of this is to
dismiss app list on a mouse click or touch outside of the app list window.

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:112: } else if
(!target->GetWindowBoundsInScreen().ApproximatelyEqual(
Explain to me again why can't we just compare target against view_->GetWidget()?

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right):

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:9: #include
"ui/app_list/presenter/app_list_presenter.h"
You already forward declare this, why do you also need to #include? Generally
speaking, avoid includes in .h if you can forward declare instead - this reduces
linking time.

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:11: #include
"ui/views/mus/pointer_watcher_event_router.h"
Why do we need this?

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:45: void
OnPointerEventObserved(const ui::PointerEvent& event,
// views::PointerWatcher:
Also move to private.

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:59:
app_list::AppListPresenter* presenter_;
// Not owned. Pointer is guaranteed to be valid while this object is alive.

[thanhph, 2016-09-29 19:43:05]

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right):

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:11: #include
"ui/views/pointer_watcher.h"
On 2016/09/29 18:38:25, mfomitchev wrote:
> We already include this in .h

Good catch. Thanks!

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:42: :
view_delegate_factory_(view_delegate_factory), presenter_(presenter) {}
On 2016/09/29 18:38:25, mfomitchev wrote:
> Nit: Would be better to have the argument order to match the init order (which
> should also match the declaration order in .h). app_list_presenter_delegate.cc
> has presenter_ first, so would be good to do the same here for consistency.

I agree. I still don't know why I get the below error when I put the variable
presenter_ before view_delegate_factory_

chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:41:8: error:
field 'presenter_' will be initialized after field 'view_delegate_factory_'
[-Werror,-Wreorder]
    :  presenter_(presenter), view_delegate_factory_(view_delegate_factory) {}

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:45: // Remove
mouse/touch monitor for this delegate
On 2016/09/29 18:38:25, mfomitchev wrote:
> Nit: This comment isn't particularly helpful, I'd just remove it.


Ok. Removed!

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:58: LOG(ERROR)
<< "\n\n!!!!!. Initializing AppListPresenterDelegateMus\n\n\n";
On 2016/09/29 18:38:25, mfomitchev wrote:
> remove

done!

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:76: // add
mouse-touch capture function to this delegate
On 2016/09/29 18:38:26, mfomitchev wrote:
> Nit: This comment isn't particularly helpful, I'd just remove it.

removed. thanks!

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:82: //
TODO(mfomitchev): Setup dismissing on mouse/touch gesture anywhere outside
On 2016/09/29 18:38:25, mfomitchev wrote:
> You can now remove this TODO :)

nice!

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:108: if
(event.type() == ui::ET_TOUCH_PRESSED ||
On 2016/09/29 18:38:26, mfomitchev wrote:
> Nit: Add a comment in the beginning saying that the purpose of this is to
> dismiss app list on a mouse click or touch outside of the app list window.

done! thanks!

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:112: } else if
(!target->GetWindowBoundsInScreen().ApproximatelyEqual(
On 2016/09/29 18:38:25, mfomitchev wrote:
> Explain to me again why can't we just compare target against
view_->GetWidget()?

It works indeed. I change the code to target != view_->GetWidget(), which looks
cleaner. thanks!

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right):

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:9: #include
"ui/app_list/presenter/app_list_presenter.h"
On 2016/09/29 18:38:26, mfomitchev wrote:
> You already forward declare this, why do you also need to #include? Generally
> speaking, avoid includes in .h if you can forward declare instead - this
reduces
> linking time.

I'm not sure where I forward declare this. Can you elaborate? If I remove the
include app_list_presenter.h, I get the error:

chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:111:17: error:
member access into incomplete type 'app_list::AppListPresenter'
      presenter_->Dismiss();
                ^

In app_list_service_ash.cc we include app_list_presenter_delegate.h but not
app_list_presenter.h

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:11: #include
"ui/views/mus/pointer_watcher_event_router.h"
On 2016/09/29 18:38:26, mfomitchev wrote:
> Why do we need this?

We need this for receiving mouse/touch callback events in the function
OnPointerEventObserved.

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:45: void
OnPointerEventObserved(const ui::PointerEvent& event,
On 2016/09/29 18:38:26, mfomitchev wrote:
> // views::PointerWatcher:
> Also move to private.

done. thanks!

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:59:
app_list::AppListPresenter* presenter_;
On 2016/09/29 18:38:26, mfomitchev wrote:
> // Not owned. Pointer is guaranteed to be valid while this object is alive.

done. thanks!

[mfomitchev, 2016-09-29 20:00:36]

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right):

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:42: :
view_delegate_factory_(view_delegate_factory), presenter_(presenter) {}
On 2016/09/29 19:43:04, thanhph wrote:
> On 2016/09/29 18:38:25, mfomitchev wrote:
> > Nit: Would be better to have the argument order to match the init order
(which
> > should also match the declaration order in .h).
app_list_presenter_delegate.cc
> > has presenter_ first, so would be good to do the same here for consistency.
> 
> I agree. I still don't know why I get the below error when I put the variable
> presenter_ before view_delegate_factory_
> 
> chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:41:8: error:
> field 'presenter_' will be initialized after field 'view_delegate_factory_'
> [-Werror,-Wreorder]
>     :  presenter_(presenter), view_delegate_factory_(view_delegate_factory) {}

The declaration order (in .h) needs to match the initialisation order in the
constructor.

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right):

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:9: #include
"ui/app_list/presenter/app_list_presenter.h"
On 2016/09/29 19:43:05, thanhph wrote:
> On 2016/09/29 18:38:26, mfomitchev wrote:
> > You already forward declare this, why do you also need to #include?
Generally
> > speaking, avoid includes in .h if you can forward declare instead - this
> reduces
> > linking time.
> 
> I'm not sure where I forward declare this. Can you elaborate? If I remove the
> include app_list_presenter.h, I get the error:
> 
> chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:111:17:
error:
> member access into incomplete type 'app_list::AppListPresenter'
>       presenter_->Dismiss();
>                 ^
> 
> In app_list_service_ash.cc we include app_list_presenter_delegate.h but not
> app_list_presenter.h 
> 

Put the include in the .cc. Forward declaration is what you have on line 16.
Take a look at this for more detail:
http://programmers.stackexchange.com/questions/195806/forward-declaration-vs-...

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:11: #include
"ui/views/mus/pointer_watcher_event_router.h"
On 2016/09/29 19:43:04, thanhph wrote:
> On 2016/09/29 18:38:26, mfomitchev wrote:
> > Why do we need this?
> 
> We need this for receiving mouse/touch callback events in the function
> OnPointerEventObserved.

Oh, ok, I see we are using PointerWatcherEventRouter in .cc. Same as above, move
the include to .cc.

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:45: void
OnPointerEventObserved(const ui::PointerEvent& event,
On 2016/09/29 19:43:04, thanhph wrote:
> On 2016/09/29 18:38:26, mfomitchev wrote:
> > // views::PointerWatcher:
> > Also move to private.
> 
> done. thanks!

You forgot the comment.

https://codereview.chromium.org/2364173002/diff/60001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right):

https://codereview.chromium.org/2364173002/diff/60001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:101: //
dismiss app list on a mouse click or touch outside of the app list window.
Nit: Capital D

https://codereview.chromium.org/2364173002/diff/60001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:106: } else if
(target != view_->GetWidget()) {
Unify the two conditions to turn this into a simple if w/o else clause.

https://codereview.chromium.org/2364173002/diff/60001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:107: // else
close the app list widget if clicks elsewhere
Nit: Don't think you need this anymore.

[thanhph, 2016-09-29 20:42:51]

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right):

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:42: :
view_delegate_factory_(view_delegate_factory), presenter_(presenter) {}
On 2016/09/29 20:00:35, mfomitchev wrote:
> On 2016/09/29 19:43:04, thanhph wrote:
> > On 2016/09/29 18:38:25, mfomitchev wrote:
> > > Nit: Would be better to have the argument order to match the init order
> (which
> > > should also match the declaration order in .h).
> app_list_presenter_delegate.cc
> > > has presenter_ first, so would be good to do the same here for
consistency.
> > 
> > I agree. I still don't know why I get the below error when I put the
variable
> > presenter_ before view_delegate_factory_
> > 
> > chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:41:8:
error:
> > field 'presenter_' will be initialized after field 'view_delegate_factory_'
> > [-Werror,-Wreorder]
> >     :  presenter_(presenter), view_delegate_factory_(view_delegate_factory)
{}
> 
> The declaration order (in .h) needs to match the initialisation order in the
> constructor.

done. thanks!

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:98: 
On 2016/09/29 18:38:26, mfomitchev wrote:
> remove blank line

ok

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right):

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:9: #include
"ui/app_list/presenter/app_list_presenter.h"
On 2016/09/29 20:00:36, mfomitchev wrote:
> On 2016/09/29 19:43:05, thanhph wrote:
> > On 2016/09/29 18:38:26, mfomitchev wrote:
> > > You already forward declare this, why do you also need to #include?
> Generally
> > > speaking, avoid includes in .h if you can forward declare instead - this
> > reduces
> > > linking time.
> > 
> > I'm not sure where I forward declare this. Can you elaborate? If I remove
the
> > include app_list_presenter.h, I get the error:
> > 
> > chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:111:17:
> error:
> > member access into incomplete type 'app_list::AppListPresenter'
> >       presenter_->Dismiss();
> >                 ^
> > 
> > In app_list_service_ash.cc we include app_list_presenter_delegate.h but not
> > app_list_presenter.h 
> > 
> 
> Put the include in the .cc. Forward declaration is what you have on line 16.
> Take a look at this for more detail:
>
http://programmers.stackexchange.com/questions/195806/forward-declaration-vs-...

I understand. Thanks!

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:11: #include
"ui/views/mus/pointer_watcher_event_router.h"
On 2016/09/29 20:00:36, mfomitchev wrote:
> On 2016/09/29 19:43:04, thanhph wrote:
> > On 2016/09/29 18:38:26, mfomitchev wrote:
> > > Why do we need this?
> > 
> > We need this for receiving mouse/touch callback events in the function
> > OnPointerEventObserved.
> 
> Oh, ok, I see we are using PointerWatcherEventRouter in .cc. Same as above,
move
> the include to .cc.

Ok. Done!

https://codereview.chromium.org/2364173002/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:45: void
OnPointerEventObserved(const ui::PointerEvent& event,
On 2016/09/29 20:00:36, mfomitchev wrote:
> On 2016/09/29 19:43:04, thanhph wrote:
> > On 2016/09/29 18:38:26, mfomitchev wrote:
> > > // views::PointerWatcher:
> > > Also move to private.
> > 
> > done. thanks!
> 
> You forgot the comment.

My bad. Comment is added!

https://codereview.chromium.org/2364173002/diff/60001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right):

https://codereview.chromium.org/2364173002/diff/60001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:101: //
dismiss app list on a mouse click or touch outside of the app list window.
On 2016/09/29 20:00:36, mfomitchev wrote:
> Nit: Capital D

done!

https://codereview.chromium.org/2364173002/diff/60001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:106: } else if
(target != view_->GetWidget()) {
On 2016/09/29 20:00:36, mfomitchev wrote:
> Unify the two conditions to turn this into a simple if w/o else clause.

done. thanks!

https://codereview.chromium.org/2364173002/diff/60001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:107: // else
close the app list widget if clicks elsewhere
On 2016/09/29 20:00:36, mfomitchev wrote:
> Nit: Don't think you need this anymore.

removed this comment!

[mfomitchev, 2016-09-29 21:21:22]

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right):

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:6: #include
"ui/app_list/presenter/app_list_presenter.h"
insert blank line
See
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:46:
views::WindowManagerConnection::Get()
If Init() is never called, I think this will trip over the DCHECK here
https://cs.chromium.org/chromium/src/ui/views/mus/pointer_watcher_event_route...

Maybe we can get rid of that DCHECK? Otherwise we should make sure we only
remove the watcher if we've previously added it. E.g. by moving the
AddPointerWatcher() to the constructor.

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:77:
->AddPointerWatcher(this, true);
I don't think we want moves? So we need to pass false here, otherwise the
callback will be unnecessarily spammy.

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:106: if
(!target or target != view_->GetWidget()) {
|| instead of "or"
Also no {} brackets for 1-line if bodies.

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right):

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:25: explicit
AppListPresenterDelegateMus(
remove explicit

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:39: 
remove blank line

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:44: // Receive
callback events when users click/touch on a widget
The comment should be just 
// views::PointerWatcher:
to highlight that this is the start of the methods overridden from
views::PointerWatcher. See line 31 for example.

[thanhph, 2016-09-29 23:08:00]

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right):

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:6: #include
"ui/app_list/presenter/app_list_presenter.h"
On 2016/09/29 21:21:22, mfomitchev wrote:
> insert blank line
> See
> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

done! Let me know if it's incorrect.

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:46:
views::WindowManagerConnection::Get()
On 2016/09/29 21:21:22, mfomitchev wrote:
> If Init() is never called, I think this will trip over the DCHECK here
>
https://cs.chromium.org/chromium/src/ui/views/mus/pointer_watcher_event_route...
> 
> Maybe we can get rid of that DCHECK? Otherwise we should make sure we only
> remove the watcher if we've previously added it. E.g. by moving the
> AddPointerWatcher() to the constructor.

Good catch! I haven't been able to start the app list widget without running
Init but there's possibly a bypass way as you mentioned. I moved
AddPointerWatcher() to the constructor just in case. Thanks!

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:77:
->AddPointerWatcher(this, true);
On 2016/09/29 21:21:21, mfomitchev wrote:
> I don't think we want moves? So we need to pass false here, otherwise the
> callback will be unnecessarily spammy.

I agree. I don't know why this value got reverted back to true. Anyway, changed
it back to false

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:106: if
(!target or target != view_->GetWidget()) {
On 2016/09/29 21:21:21, mfomitchev wrote:
> || instead of "or"
> Also no {} brackets for 1-line if bodies.

good catch. Thanks!

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right):

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:25: explicit
AppListPresenterDelegateMus(
On 2016/09/29 21:21:22, mfomitchev wrote:
> remove explicit

ok done!

https://codereview.chromium.org/2364173002/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:44: // Receive
callback events when users click/touch on a widget
On 2016/09/29 21:21:22, mfomitchev wrote:
> The comment should be just 
> // views::PointerWatcher:
> to highlight that this is the start of the methods overridden from
> views::PointerWatcher. See line 31 for example.


cool thanks!

[mfomitchev, 2016-09-30 16:36:14]

Thanks! This is getting pretty close.

https://codereview.chromium.org/2364173002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (left):

https://codereview.chromium.org/2364173002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:81:
DCHECK(is_visible_);
Just noticed this was removed. This code should stay.

https://codereview.chromium.org/2364173002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right):

https://codereview.chromium.org/2364173002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:51:
views::WindowManagerConnection::Get()
Sorry.. this is probably not a huge deal in practice, but doing a trip to app
list delegate for every single click/touch anywhere on the screen feels bad. Can
we instead add the watcher when the app list is shown (in OnShown) and remove it
when it is dismissed (in OnDismissed)? And then we also need to remove it in the
destructor if is_visible is true.

https://codereview.chromium.org/2364173002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:104: //
Dismiss app list on a mouse click or touch outside of the app list window.
If you go with my suggestion for adding/removing pointer watcher, we can also
add DCHECK(is_visible_) here.

https://codereview.chromium.org/2364173002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right):

https://codereview.chromium.org/2364173002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:28: 
remove blank line

[thanhph, 2016-09-30 17:49:23]

https://codereview.chromium.org/2364173002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right):

https://codereview.chromium.org/2364173002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:51:
views::WindowManagerConnection::Get()
On 2016/09/30 16:36:14, mfomitchev wrote:
> Sorry.. this is probably not a huge deal in practice, but doing a trip to app
> list delegate for every single click/touch anywhere on the screen feels bad.
Can
> we instead add the watcher when the app list is shown (in OnShown) and remove
it
> when it is dismissed (in OnDismissed)? And then we also need to remove it in
the
> destructor if is_visible is true.

By adding log output in constructor and destructor, I notice the trip to app
list delegate only got called once when starting the widget and upon object
exit) (also not get called when we click inside the app list widget). but your
approach is good when the program has a weird behavior that destructor doesn't
get called. I also flip the flag is_visible_ where appropriate (Constructor,
Destructor, OnShown, OnDismissed) according to this approach. Thanks!

https://codereview.chromium.org/2364173002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:104: //
Dismiss app list on a mouse click or touch outside of the app list window.
On 2016/09/30 16:36:14, mfomitchev wrote:
> If you go with my suggestion for adding/removing pointer watcher, we can also
> add DCHECK(is_visible_) here.

done, thanks! Is it a good practice to add more DCHECK?

https://codereview.chromium.org/2364173002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right):

https://codereview.chromium.org/2364173002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:28: 
On 2016/09/30 16:36:14, mfomitchev wrote:
> remove blank line

done!

[mfomitchev, 2016-09-30 18:25:46]

https://codereview.chromium.org/2364173002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right):

https://codereview.chromium.org/2364173002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:51:
views::WindowManagerConnection::Get()
On 2016/09/30 17:49:23, thanhph wrote:
> On 2016/09/30 16:36:14, mfomitchev wrote:
> > Sorry.. this is probably not a huge deal in practice, but doing a trip to
app
> > list delegate for every single click/touch anywhere on the screen feels bad.
> Can
> > we instead add the watcher when the app list is shown (in OnShown) and
remove
> it
> > when it is dismissed (in OnDismissed)? And then we also need to remove it in
> the
> > destructor if is_visible is true.
> 
> By adding log output in constructor and destructor, I notice the trip to app
> list delegate only got called once when starting the widget and upon object
> exit) (also not get called when we click inside the app list widget). but your
> approach is good when the program has a weird behavior that destructor doesn't
> get called. I also flip the flag is_visible_ where appropriate (Constructor,
> Destructor, OnShown, OnDismissed) according to this approach. Thanks!

Oh, I didn't realize that the delegate gets destroyed when the app list is
hidden. If that is the case, this is much less of an issue, feel free to
disregard my comment.

https://codereview.chromium.org/2364173002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:104: //
Dismiss app list on a mouse click or touch outside of the app list window.
On 2016/09/30 17:49:23, thanhph wrote:
> On 2016/09/30 16:36:14, mfomitchev wrote:
> > If you go with my suggestion for adding/removing pointer watcher, we can
also
> > add DCHECK(is_visible_) here.
> 
> done, thanks! Is it a good practice to add more DCHECK?

Yeah, if there is an invariant you expect to hold,l DCHECK both catches bugs and
serves as a concies replacement for a comment.

[mfomitchev, 2016-09-30 18:41:20]

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right):

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:45: if
(!is_visible_) {
This will always be true

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:46:
views::WindowManagerConnection::Get()
Shouldn't do this here, the idea was to not register the watcher unless the app
list is shown.

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:49:
is_visible_ = true;
Why?

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:59:
is_visible_ = false;
We don't need to update is_visible_ when destroying the object.

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:94: if
(!is_visible_) {
I don't think OnShown can ever be called when is_visible is true? If so, can you
replace if with DCHECK?

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:103: if
(is_visible_) {
Why did you replace DCHECK with if? OnDismissed should never be called when
is_visible is false.

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:117: void
AppListPresenterDelegateMus::OnPointerEventObserved(
Nit: Move this method to after GetVisibilityAnimationOffset. The order of the
methods in .h and .cc ideally should match.

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right):

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:14: class
AppListPresenter;
Forward declarations should be ordered alphabetically (I'd hope git cl format
would catch that)

[thanhph, 2016-09-30 19:03:52]

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right):

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:45: if
(!is_visible_) {
On 2016/09/30 18:41:19, mfomitchev wrote:
> This will always be true

reverted

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:46:
views::WindowManagerConnection::Get()
On 2016/09/30 18:41:19, mfomitchev wrote:
> Shouldn't do this here, the idea was to not register the watcher unless the
app
> list is shown.

reverted

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:49:
is_visible_ = true;
On 2016/09/30 18:41:19, mfomitchev wrote:
> Why?

reverted

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:59:
is_visible_ = false;
On 2016/09/30 18:41:19, mfomitchev wrote:
> We don't need to update is_visible_ when destroying the object. 

reverted

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:94: if
(!is_visible_) {
On 2016/09/30 18:41:19, mfomitchev wrote:
> I don't think OnShown can ever be called when is_visible is true? If so, can
you
> replace if with DCHECK?

reverted

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:103: if
(is_visible_) {
On 2016/09/30 18:41:19, mfomitchev wrote:
> Why did you replace DCHECK with if? OnDismissed should never be called when
> is_visible is false.

Good to know this. Thanks!

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:117: void
AppListPresenterDelegateMus::OnPointerEventObserved(
On 2016/09/30 18:41:19, mfomitchev wrote:
> Nit: Move this method to after GetVisibilityAnimationOffset. The order of the
> methods in .h and .cc ideally should match.

done!

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right):

https://codereview.chromium.org/2364173002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:14: class
AppListPresenter;
On 2016/09/30 18:41:19, mfomitchev wrote:
> Forward declarations should be ordered alphabetically (I'd hope git cl format
> would catch that)

good catch. git cl format doesn't catch this i think or for this case. I run it
everytime before code commit.

[mfomitchev, 2016-09-30 19:05:25]

https://codereview.chromium.org/2364173002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (left):

https://codereview.chromium.org/2364173002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:81:
DCHECK(is_visible_);
Keep the DCHECK

[thanhph, 2016-09-30 19:09:09]

https://codereview.chromium.org/2364173002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (left):

https://codereview.chromium.org/2364173002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:81:
DCHECK(is_visible_);
On 2016/09/30 19:05:25, mfomitchev wrote:
> Keep the DCHECK

added back.

[varkha, 2016-10-05 19:23:41]

Description was changed from

==========
Add app_list_presenterr pointer in app_list_presenter_delegate_mus to use
dismiss function

Add mouse/touch capture function

Exploring how to close single widget

BUG=closing all the app instead of single widge at the momment
==========

to

==========
Add app_list_presenterr pointer in app_list_presenter_delegate_mus to use
dismiss function

Add mouse/touch capture function.
Exploring how to close single widget.

BUG=611447
==========

[mfomitchev, 2016-10-11 15:49:13]

https://codereview.chromium.org/2364173002/diff/180001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus_unittest.cc
(right):

https://codereview.chromium.org/2364173002/diff/180001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus_unittest.cc:37:
class AppListViewDelegateFactoryImpl
I'd move this next to AppListTestViewDelegate and call it
AppListTestViewDelegateFactory

https://codereview.chromium.org/2364173002/diff/180001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus_unittest.cc:54:
// DISALLOW_COPY_AND_ASSIGN(AppListViewDelegateFactoryImpl);
You want this on pretty much all classes.

https://codereview.chromium.org/2364173002/diff/180001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus_unittest.cc:62:
view_delegate_factory)
Rather than taking AppListViewDelegateFactory as an argument, I'd just create
AppListViewDelegateFactoryImpl/AppListTestViewDelegateFactory internally. It's
not like you are going to be using this with different factories.

https://codereview.chromium.org/2364173002/diff/180001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus_unittest.cc:81:
AppListPresenterDelegateMusTest() {}
You can probably initialize presenter_delegate_factory_ here.

presenter_delegate_factory_.reset(new AppListPresenterDelegateFactoryMus(new
AppListViewDelegateFactory()));

or just 
presenter_delegate_factory_.reset(new AppListPresenterDelegateFactoryMus());
if you do what I suggested in line 62

https://codereview.chromium.org/2364173002/diff/180001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus_unittest.cc:134:
// presenter_delegate_factory_.reset(new AppListPresenterDelegateFactoryMus(
You probably need to std::move the unique_ptr, but I'd do this differently - see
comments above.

https://codereview.chromium.org/2364173002/diff/180001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus_unittest.cc:137:
// presenter_delegate_factory_.reset(temp);
You actually want to create AppListPresenterImpl at some point using this
factory. (Maybe you simply haven't got to that point yet).

https://codereview.chromium.org/2364173002/diff/180001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus_unittest.cc:143:
//   content->SetBounds(10, 20, 90, 180);
Why not make the origin (0,0)? The view coord are in widget's coord space. Also,
don't use "magic numbers" - use widget's bounds.

https://codereview.chromium.org/2364173002/diff/180001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus_unittest.cc:160:
//   // widget->Deactivate();
For this test, you probably don't need to activate/deactivate the widget.

[mfomitchev, 2016-10-18 23:50:42]

https://codereview.chromium.org/2364173002/diff/280001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus_unittest.cc
(right):

https://codereview.chromium.org/2364173002/diff/280001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus_unittest.cc:240:
ASSERT_TRUE(WindowManagerConnection::Exists());
Can you put a LOG before this so that we can tell if it is actually executing
this code?

https://codereview.chromium.org/2364173002/diff/280001/chrome/test/views_chro...
File chrome/test/views_chrome_test_suite.cc (right):

https://codereview.chromium.org/2364173002/diff/280001/chrome/test/views_chro...
chrome/test/views_chrome_test_suite.cc:62: return
base::MakeUnique<PlatformTestHelperMus>(callback.Run(), identity);
Can you add a LOG here so that we can tell if this gets executed?

https://codereview.chromium.org/2364173002/diff/280001/chrome/test/views_chro...
File chrome/test/views_chrome_test_suite.h (right):

https://codereview.chromium.org/2364173002/diff/280001/chrome/test/views_chro...
chrome/test/views_chrome_test_suite.h:17: class ViewsChromeTestSuite : public
ViewsTestSuite {
This is not really the right way because we already have a test suite for chrome
unit tests. If we want to use a new one for AppListPresenterDelegateMusTest, we
need to create a new build target.

[thanhph, 2016-10-19 00:02:06]

https://codereview.chromium.org/2364173002/diff/280001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus_unittest.cc
(right):

https://codereview.chromium.org/2364173002/diff/280001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus_unittest.cc:240:
ASSERT_TRUE(WindowManagerConnection::Exists());
On 2016/10/18 23:50:42, mfomitchev wrote:
> Can you put a LOG before this so that we can tell if it is actually executing
> this code?

I just did. Binary unit_tests ran through this line.

https://codereview.chromium.org/2364173002/diff/280001/chrome/test/views_chro...
File chrome/test/views_chrome_test_suite.cc (right):

https://codereview.chromium.org/2364173002/diff/280001/chrome/test/views_chro...
chrome/test/views_chrome_test_suite.cc:62: return
base::MakeUnique<PlatformTestHelperMus>(callback.Run(), identity);
On 2016/10/18 23:50:42, mfomitchev wrote:
> Can you add a LOG here so that we can tell if this gets executed?

I just did. Binary unit_tests ran through this line.

https://codereview.chromium.org/2364173002/diff/280001/chrome/test/views_chro...
File chrome/test/views_chrome_test_suite.h (right):

https://codereview.chromium.org/2364173002/diff/280001/chrome/test/views_chro...
chrome/test/views_chrome_test_suite.h:17: class ViewsChromeTestSuite : public
ViewsTestSuite {
On 2016/10/18 23:50:42, mfomitchev wrote:
> This is not really the right way because we already have a test suite for
chrome
> unit tests. If we want to use a new one for AppListPresenterDelegateMusTest,
we
> need to create a new build target.

We can create new build target if this looks all right. We need to work on a
non-confusing name also.

[thanhph, 2016-10-24 19:13:26]

Description was changed from

==========
Add app_list_presenterr pointer in app_list_presenter_delegate_mus to use
dismiss function

Add mouse/touch capture function.
Exploring how to close single widget.

BUG=611447
==========

to

==========
Duplicated Issue: 2434573004
Add app_list_presenterr pointer in app_list_presenter_delegate_mus to use
dismiss function

Add mouse/touch capture function.
Exploring how to close single widget.

BUG=611447
==========