Remove --enable-mac-views-app-list on Mac

Remove --enable-mac-views-app-list on Mac

This was never hooked up completely, so the chrome/browser code involved
was only ever linked in for tests. Isolate the files properly on Mac. In
preparation for removing them from non-chromeos entirely.

BUG=600915
Committed: https://crrev.com/c87d33843040f336059c92a16b63988815b7e1c9
Cr-Commit-Position: refs/heads/master@{#391758}

[tapted, 2016-05-04 09:53:17]

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

[tapted, 2016-05-04 09:53:17]

Hi Matt, PTAL. Split from http://crrev.com/1861233003

[Matt Giuca, 2016-05-05 00:22:48]

lgtm

[tapted, 2016-05-05 00:23:22]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2016-05-05 00:23:44]

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

[commit-bot: I haz the power, 2016-05-05 00:31:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-05 00:31:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tapted, 2016-05-05 01:10:02]

Description was changed from

==========
Remove --enable-mac-views-app-list on Mac

This was never hooked up completely, so the chrome/browser code involved
was only ever linked in for tests. Isolate the files properly on Mac.

BUG=600915
==========

to

==========
Remove --enable-mac-views-app-list on Mac

This was never hooked up completely, so the chrome/browser code involved
was only ever linked in for tests. Isolate the files properly on Mac. In
preparation for removing them from non-chromeos entirely.

BUG=600915
==========

[tapted, 2016-05-05 01:10:02]

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

[tapted, 2016-05-05 01:10:20]

+msw for

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/ui/BUILD.gn

[msw, 2016-05-05 01:51:07]

lgtm with nits

https://codereview.chromium.org/1951783002/diff/40001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/1951783002/diff/40001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:524: if (!is_mac) {
This is a bit odd (I first thought "Why not if toolkit_views?", but I guess we
don't want to compile this on Mac views either); hopefully it'll just become "if
(is_chromeos)" soon anyway, so I guess it's fine.

https://codereview.chromium.org/1951783002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/1951783002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_mac.mm:510:
AppListServiceCocoaMac::GetInstance()->InitWithProfilePath(initial_profile,
nit: just call Get()?

https://codereview.chromium.org/1951783002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_mac.mm:578:
base::Unretained(AppListServiceCocoaMac::GetInstance())));
nit: just call AppListService::Get()?

https://codereview.chromium.org/1951783002/diff/40001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1951783002/diff/40001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2759: # This assumes the AppList is views-based.
nit: "and Mac uses Cocoa"?

https://codereview.chromium.org/1951783002/diff/40001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2765: ['enable_app_list==1 and chromeos==1', {
optional nit: merge this with your nested chromeos==1 block inside the
enable_app_list==1 block above or vice-versa.

https://codereview.chromium.org/1951783002/diff/40001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1951783002/diff/40001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:2160: # This assumes the AppList is views-based.
ditto nit: "and Mac uses Cocoa"?

https://codereview.chromium.org/1951783002/diff/40001/ui/app_list/app_list_sw...
File ui/app_list/app_list_switches.cc (right):

https://codereview.chromium.org/1951783002/diff/40001/ui/app_list/app_list_sw...
ui/app_list/app_list_switches.cc:55: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(kEnableSyncAppList))
optional nit: just return this value.

[tapted, 2016-05-05 02:53:43]

https://codereview.chromium.org/1951783002/diff/40001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/1951783002/diff/40001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:524: if (!is_mac) {
On 2016/05/05 01:51:06, msw wrote:
> This is a bit odd (I first thought "Why not if toolkit_views?", but I guess we
> don't want to compile this on Mac views either); hopefully it'll just become
"if
> (is_chromeos)" soon anyway, so I guess it's fine.

Yup, in fact ui_app_list_views_sources can then just be folded into
ui_app_list_sources, but we're blocked on having a build where we can test with
enable_app_list=0

https://codereview.chromium.org/1951783002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/1951783002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_mac.mm:510:
AppListServiceCocoaMac::GetInstance()->InitWithProfilePath(initial_profile,
On 2016/05/05 01:51:06, msw wrote:
> nit: just call Get()?

Hehe - I tried that _ages_ ago, but this one has to be called `GetInstance`,
since base::Singleton only declares friendship with `friend Type*
Type::GetInstance();` -- If the method is called `Get()` it won't be allowed to
call Singleton::get() since that's a private member of Singleton.

https://codereview.chromium.org/1951783002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_mac.mm:578:
base::Unretained(AppListServiceCocoaMac::GetInstance())));
On 2016/05/05 01:51:06, msw wrote:
> nit: just call AppListService::Get()?

see above

https://codereview.chromium.org/1951783002/diff/40001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1951783002/diff/40001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2759: # This assumes the AppList is views-based.
On 2016/05/05 01:51:07, msw wrote:
> nit: "and Mac uses Cocoa"?

yep added "but Mac only links browser parts for the Cocoa implementation."

https://codereview.chromium.org/1951783002/diff/40001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2765: ['enable_app_list==1 and chromeos==1', {
On 2016/05/05 01:51:07, msw wrote:
> optional nit: merge this with your nested chromeos==1 block inside the
> enable_app_list==1 block above or vice-versa.

Hum - I guess this was from a time when there was a distinct `enable_arc`, but
that's gone. app_list_chromeos_arc_sources should just be folded into
app_list_chromeos_sources. Merged the 'dependencies' bit. (Done.).

https://codereview.chromium.org/1951783002/diff/40001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1951783002/diff/40001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:2160: # This assumes the AppList is views-based.
On 2016/05/05 01:51:07, msw wrote:
> ditto nit: "and Mac uses Cocoa"?

Done.

https://codereview.chromium.org/1951783002/diff/40001/ui/app_list/app_list_sw...
File ui/app_list/app_list_switches.cc (right):

https://codereview.chromium.org/1951783002/diff/40001/ui/app_list/app_list_sw...
ui/app_list/app_list_switches.cc:55: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(kEnableSyncAppList))
On 2016/05/05 01:51:07, msw wrote:
> optional nit: just return this value.

Done.

[tapted, 2016-05-05 03:58:44]

The CQ bit was checked by tapted@chromium.org

[tapted, 2016-05-05 03:58:44]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
mgiuca@chromium.org
Link to the patchset: https://codereview.chromium.org/1951783002/#ps60001
(title: "msw comments")

[commit-bot: I haz the power, 2016-05-05 03:58:57]

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

[commit-bot: I haz the power, 2016-05-05 05:00:37]

Description was changed from

==========
Remove --enable-mac-views-app-list on Mac

This was never hooked up completely, so the chrome/browser code involved
was only ever linked in for tests. Isolate the files properly on Mac. In
preparation for removing them from non-chromeos entirely.

BUG=600915
==========

to

==========
Remove --enable-mac-views-app-list on Mac

This was never hooked up completely, so the chrome/browser code involved
was only ever linked in for tests. Isolate the files properly on Mac. In
preparation for removing them from non-chromeos entirely.

BUG=600915
==========

[commit-bot: I haz the power, 2016-05-05 05:00:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-05 05:01:34]

Description was changed from

==========
Remove --enable-mac-views-app-list on Mac

This was never hooked up completely, so the chrome/browser code involved
was only ever linked in for tests. Isolate the files properly on Mac. In
preparation for removing them from non-chromeos entirely.

BUG=600915
==========

to

==========
Remove --enable-mac-views-app-list on Mac

This was never hooked up completely, so the chrome/browser code involved
was only ever linked in for tests. Isolate the files properly on Mac. In
preparation for removing them from non-chromeos entirely.

BUG=600915

Committed: https://crrev.com/c87d33843040f336059c92a16b63988815b7e1c9
Cr-Commit-Position: refs/heads/master@{#391758}
==========

[commit-bot: I haz the power, 2016-05-05 05:01:37]

Patchset 4 (id:??) landed as
https://crrev.com/c87d33843040f336059c92a16b63988815b7e1c9
Cr-Commit-Position: refs/heads/master@{#391758}