Moved methods that don't rely on internal state out of ChromeLauncherController.

Moved methods that don't rely on internal state out of ChromeLauncherController.

BUG=618445
Committed: https://crrev.com/9c77e2215c1a904b40ef05bf85cc865cf760b39d
Cr-Commit-Position: refs/heads/master@{#402482}

[StarAZ, 2016-06-23 14:37:15]

staraz@google.com changed reviewers:
+ skuhne@chromium.org, staraz@google.com, tapted@chromium.org

[StarAZ, 2016-06-23 14:37:16]

Please review my changes.

skuhne@: Please review all changes under chrome/browser/ui/ash/launcher/ since
you're the owner.

tapted@: Please review chrome/browser/ui/ash/app_list/app_list_controller_ash.cc
and 
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc
since you're the owner of these files.

[mfomitchev, 2016-06-23 16:00:26]

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

[mfomitchev, 2016-06-23 16:00:27]

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

https://codereview.chromium.org/2090133003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h:13: namespace
util {
I'd get rid of the namespaces. I think this should be in the same namespace as
ChromeLauncherController, i.e. the global namespace. chrome::util doesn't
currently exist, and creating a namespace just for this class doesn't seem
warranted. Also, if you take a look at other _util files in
chrome/browser/ui/ash - none of them are in a specialized namepsace.

https://codereview.chromium.org/2090133003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h:19: const
extensions::Extension* GetExtensionForAppID(Profile* profile,
nit: make profile the second argument. Typically the more "important" arguments
are first. Profile is a global state that doesn't change much, whereas the
app_id actually corresponds to the thing you are querying here.

https://codereview.chromium.org/2090133003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h:23:
AppListControllerDelegate::Pinnable GetPinnable(Profile* profile,
Ditto

https://codereview.chromium.org/2090133003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h:23:
AppListControllerDelegate::Pinnable GetPinnable(Profile* profile,

Perhaps rename GetPinnable to GetPinnableForAppID to make the usage more clear
now that this is not in ChromeLauncherController.

[StarAZ, 2016-06-23 18:33:46]

Description was changed from

==========
Moved methods that don't rely on internal state out of ChromeLauncherController.

BUG=618445
==========

to

==========
Moved methods that don't rely on internal state out of ChromeLauncherController.

BUG=618445
==========

[StarAZ, 2016-06-23 18:33:46]

staraz@google.com changed reviewers:
- skuhne@chromium.org, staraz@google.com, tapted@chromium.org

[StarAZ, 2016-06-23 19:35:02]

staraz@google.com changed reviewers:
+ msw@chromium.org, staraz@google.com

[StarAZ, 2016-06-23 19:35:02]

staraz@google.com changed required reviewers:
+ msw@chromium.org

[StarAZ, 2016-06-23 19:35:03]

msw@chromium.org: Please review changes

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

https://codereview.chromium.org/2090133003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h:13: namespace
util {
On 2016/06/23 16:00:27, mfomitchev wrote:
> I'd get rid of the namespaces. I think this should be in the same namespace as
> ChromeLauncherController, i.e. the global namespace. chrome::util doesn't
> currently exist, and creating a namespace just for this class doesn't seem
> warranted. Also, if you take a look at other _util files in
> chrome/browser/ui/ash - none of them are in a specialized namepsace.

Acknowledged.

https://codereview.chromium.org/2090133003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h:19: const
extensions::Extension* GetExtensionForAppID(Profile* profile,
On 2016/06/23 16:00:27, mfomitchev wrote:
> nit: make profile the second argument. Typically the more "important"
arguments
> are first. Profile is a global state that doesn't change much, whereas the
> app_id actually corresponds to the thing you are querying here.

Acknowledged.

https://codereview.chromium.org/2090133003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h:23:
AppListControllerDelegate::Pinnable GetPinnable(Profile* profile,
On 2016/06/23 16:00:27, mfomitchev wrote:
> 
> Perhaps rename GetPinnable to GetPinnableForAppID to make the usage more clear
> now that this is not in ChromeLauncherController.

Acknowledged.

[mfomitchev, 2016-06-23 19:41:57]

https://codereview.chromium.org/2090133003/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h (right):

https://codereview.chromium.org/2090133003/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h:9: #include
"chrome/browser/ui/browser.h"
Don't include browser.h and extension.h here, just forward-declare Browser and
Extension (and include .h files in the .cc if needed). See
https://www.chromium.org/developers/coding-style#TOC-Forward-declarations-vs....

[StarAZ, 2016-06-23 20:12:42]

https://codereview.chromium.org/2090133003/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h (right):

https://codereview.chromium.org/2090133003/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h:9: #include
"chrome/browser/ui/browser.h"
On 2016/06/23 19:41:57, mfomitchev wrote:
> Don't include browser.h and extension.h here, just forward-declare Browser and
> Extension (and include .h files in the .cc if needed). See
>
https://www.chromium.org/developers/coding-style#TOC-Forward-declarations-vs....

Done.

[mfomitchev, 2016-06-23 20:19:02]

https://codereview.chromium.org/2090133003/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.cc (right):

https://codereview.chromium.org/2090133003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.cc:8: #include
"ash/shell.h"
Looks like we are including too much here. Can you please get rid of the
includes that are not needed?

[StarAZ, 2016-06-23 20:49:49]

https://codereview.chromium.org/2090133003/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.cc (right):

https://codereview.chromium.org/2090133003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.cc:8: #include
"ash/shell.h"
On 2016/06/23 20:19:02, mfomitchev wrote:
> Looks like we are including too much here. Can you please get rid of the
> includes that are not needed?

Done.

[mfomitchev, 2016-06-23 22:33:15]

LGTM once you get the bots green (Getprofile -> GetProfile)

[tapted, 2016-06-23 23:47:29]

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

[tapted, 2016-06-23 23:47:30]

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:44: 
remove blank line

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.cc (right):

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.cc:1: //
Copyright (c) 2016 The Chromium Authors. All rights reserved.
remove (c)

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.cc:19: #include
"content/public/common/mojo_shell_connection.h"
is this used for anything? Some other things above seem unused too - e.g.
scoped_user_pref_update.h - can you do a pass over them?

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h (right):

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h:1: // Copyright
(c) 2016 The Chromium Authors. All rights reserved.
nit: remove (c)

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc
(right):

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:177:
app_->id(), ChromeLauncherController::instance()->GetProfile()) ==
It's probably more correct to pass |profile_| here. Then does that let you
remove the chrome_launcher_controller.h #include? (as well as the has-instance()
check above)

(Or do we need special handling for someone trying to pin from the App Info
dialog from chrome://extensions on a teleported window?)

https://codereview.chromium.org/2090133003/diff/140001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/2090133003/diff/140001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:560:
'browser/ui/ash/launcher/chrome_launcher_controller_util.cc',
nit: sort below _impl

[StarAZ, 2016-06-24 14:15:17]

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:44: 
On 2016/06/23 23:47:30, tapted wrote:
> remove blank line

Done.

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.cc (right):

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.cc:19: #include
"content/public/common/mojo_shell_connection.h"
On 2016/06/23 23:47:30, tapted wrote:
> is this used for anything? Some other things above seem unused too - e.g.
> scoped_user_pref_update.h - can you do a pass over them?

Done.

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h (right):

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h:1: // Copyright
(c) 2016 The Chromium Authors. All rights reserved.
On 2016/06/23 23:47:30, tapted wrote:
> nit: remove (c)

Done.

https://codereview.chromium.org/2090133003/diff/140001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/2090133003/diff/140001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:560:
'browser/ui/ash/launcher/chrome_launcher_controller_util.cc',
On 2016/06/23 23:47:30, tapted wrote:
> nit: sort below _impl

Done.

[tapted, 2016-06-27 00:25:36]

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc
(right):

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:177:
app_->id(), ChromeLauncherController::instance()->GetProfile()) ==
On 2016/06/23 23:47:30, tapted wrote:
> It's probably more correct to pass |profile_| here. Then does that let you
> remove the chrome_launcher_controller.h #include? (as well as the
has-instance()
> check above)
> 
> (Or do we need special handling for someone trying to pin from the App Info
> dialog from chrome://extensions on a teleported window?)

ping?  (did you try this out?)

[Alex Z., 2016-06-27 02:51:47]

On 2016/06/27 00:25:36, tapted wrote:
>
https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/view...
> File chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc
> (right):
> 
>
https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/view...
> chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:177:
> app_->id(), ChromeLauncherController::instance()->GetProfile()) ==
> On 2016/06/23 23:47:30, tapted wrote:
> > It's probably more correct to pass |profile_| here. Then does that let you
> > remove the chrome_launcher_controller.h #include? (as well as the
> has-instance()
> > check above)
> > 
> > (Or do we need special handling for someone trying to pin from the App Info
> > dialog from chrome://extensions on a teleported window?)
> 
> ping?  (did you try this out?)

I haven't. I will try it tomorrow when I get to office.

[StarAZ, 2016-06-27 14:22:47]

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc
(right):

https://codereview.chromium.org/2090133003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:177:
app_->id(), ChromeLauncherController::instance()->GetProfile()) ==
On 2016/06/23 23:47:30, tapted wrote:
> It's probably more correct to pass |profile_| here. Then does that let you
> remove the chrome_launcher_controller.h #include? (as well as the
has-instance()
> check above)
> 
> (Or do we need special handling for someone trying to pin from the App Info
> dialog from chrome://extensions on a teleported window?)

Done.

[msw, 2016-06-27 18:50:26]

lgtm with nits; sorry for the delay.

https://codereview.chromium.org/2090133003/diff/250001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2090133003/diff/250001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:42: #include
"chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
nit: remove (no longer needed)

https://codereview.chromium.org/2090133003/diff/250001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:87: #include
"extensions/browser/extension_registry.h"
nit: remove (no longer needed)

https://codereview.chromium.org/2090133003/diff/250001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2090133003/diff/250001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1336:
launcher_controller_->GetProfile()));
nit: use profile() (or cache this for the 4 uses here).

https://codereview.chromium.org/2090133003/diff/250001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3383:
GetPinnableForAppID(app_id, launcher_controller_->GetProfile()));
nit: use profile().

https://codereview.chromium.org/2090133003/diff/250001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h (right):

https://codereview.chromium.org/2090133003/diff/250001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h:5: #ifndef
CHROME_BROWSER_UI_ASH_LAUNCHER_CHROME_LAUNCHER_CONTROLLER_UTIL_H_
nit: this is ok for now, but we might want to find better homes for these misc
CLC functions. (ie. IsBrowserFromActiveUser -> multi_user_util.*).

https://codereview.chromium.org/2090133003/diff/250001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h:23: //
TODO(khmel): Find better home for Pinnable enum.
nit: move this comment to the enum decl; add a function comment.

https://codereview.chromium.org/2090133003/diff/250001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc
(right):

https://codereview.chromium.org/2090133003/diff/250001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:175:
app_->id(), profile_) ==
nit: fix line wrapping. Try "git cl format
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc", unless
that touches many more lines.

[tapted, 2016-06-27 23:46:31]

app_list_controller_ash and app_info_footer_panel.cc lgtm

[StarAZ, 2016-06-28 14:48:26]

The CQ bit was checked by staraz@google.com

[StarAZ, 2016-06-28 14:48:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
mfomitchev@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2090133003/#ps270001
(title: "nit changes")

[commit-bot: I haz the power, 2016-06-28 14:48:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 15:49:59]

Description was changed from

==========
Moved methods that don't rely on internal state out of ChromeLauncherController.

BUG=618445
==========

to

==========
Moved methods that don't rely on internal state out of ChromeLauncherController.

BUG=618445
==========

[commit-bot: I haz the power, 2016-06-28 15:50:01]

Committed patchset #15 (id:270001)

[commit-bot: I haz the power, 2016-06-28 15:52:49]

Description was changed from

==========
Moved methods that don't rely on internal state out of ChromeLauncherController.

BUG=618445
==========

to

==========
Moved methods that don't rely on internal state out of ChromeLauncherController.

BUG=618445

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

[commit-bot: I haz the power, 2016-06-28 15:52:51]

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