Remove the old task manager view

Remove the old task manager view

This CL does the following:
- Enables the old task manager backend implementation only on MacOSX cocoa.
- Removes the --disable-new-task-manager flag.
- Removes the old task manager view

BUG=604806
Committed: https://crrev.com/17406972b0d3ab7d1448dda982dc294b6cede9a7
Cr-Commit-Position: refs/heads/master@{#395671}

[afakhry, 2016-04-21 23:44:04]

afakhry@chromium.org changed reviewers:
+ nick@chromium.org

[afakhry, 2016-04-21 23:44:04]

Nick, could you please take a look?

[afakhry, 2016-04-22 18:44:40]

afakhry@chromium.org changed reviewers:
+ thestig@chromium.org

[afakhry, 2016-04-22 18:44:41]

thestig@chromium.org: Please take a look. Thanks!

[ncarter (slow), 2016-04-22 21:47:20]

https://codereview.chromium.org/1912773002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc
(right):

https://codereview.chromium.org/1912773002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc:98:
ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(0, MatchAnyPrint()));
Let's not put these tests on a path to deletion. For example: I don't think the
new task manager has any printing coverage, does it?

We should just port WaitForTaskManagerRows to the new task manager, as we
discussed doing at the outset. I'm putting together a CL to do this. It looks
easy.

[afakhry, 2016-04-22 22:51:19]

https://codereview.chromium.org/1912773002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc
(right):

https://codereview.chromium.org/1912773002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc:98:
ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(0, MatchAnyPrint()));
On 2016/04/22 21:47:20, ncarter wrote:
> Let's not put these tests on a path to deletion. For example: I don't think
the
> new task manager has any printing coverage, does it?
> 
> We should just port WaitForTaskManagerRows to the new task manager, as we
> discussed doing at the outset. I'm putting together a CL to do this. It looks
> easy.

We do have printing coverage for the new task manager:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pri...


:)

[Lei Zhang, 2016-04-22 23:04:18]

Huh, what's this c/b/caps thing that was added in r320357? It is being removed.
Do we care? +cpu / +scottmg.

[scottmg, 2016-04-22 23:11:48]

On 2016/04/22 23:04:18, Lei Zhang wrote:
> Huh, what's this c/b/caps thing that was added in r320357? It is being
removed.
> Do we care? +cpu / +scottmg.

It was going to be part of the new crash reporting system, but yes, can be
removed now after all.

[Lei Zhang, 2016-04-23 00:00:13]

Also +tapted FYI.

[Lei Zhang, 2016-04-23 00:52:32]

https://codereview.chromium.org/1912773002/diff/80001/chrome/browser/task_man...
File chrome/browser/task_management/task_manager_interface.cc (right):

https://codereview.chromium.org/1912773002/diff/80001/chrome/browser/task_man...
chrome/browser/task_management/task_manager_interface.cc:31: if
(g_browser_process->local_state()) {
auto* state = g_browser_process->local_state();
return state ? state->GetBoolean(prefs::kTaskManagerEndProcessEnabled) : true;

https://codereview.chromium.org/1912773002/diff/80001/chrome/browser/task_man...
File chrome/browser/task_manager/task_manager.h (right):

https://codereview.chromium.org/1912773002/diff/80001/chrome/browser/task_man...
chrome/browser/task_manager/task_manager.h:33: //
-----------------------------------------------------------------------------
Do we have a timeline for when this gets removed? If it may be a while, and you
are paranoid others may disregard this warning, you many want to torpedo any
attempts to add this file.

#if !defined(OS_MACOSX)
#error "I sunk your battleship"
#end

Whatever you decide on, do the same in task_manager_browsertest_util.h?

https://codereview.chromium.org/1912773002/diff/80001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1912773002/diff/80001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:2988: # New Task Manager Sources:
not needed anymore?

https://codereview.chromium.org/1912773002/diff/80001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:3059:
'chrome_browser_old_task_manager_macosx_sources': [
nit: omit 'osx', ditto for other gypi files

https://codereview.chromium.org/1912773002/diff/80001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:3504: },
I think this only works because gyp added else-if at some point. It still looks
odd because the indentation is off.

Maybe do what line 3921 does?

['chromeos==1' {
  ...
}],
['OS=="mac"' {
  ...
}],

https://codereview.chromium.org/1912773002/diff/80001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1912773002/diff/80001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:1713:
'browser/task_manager/task_manager_browsertest_util.cc',
A bit weird to have this standalone.

browser/task_manager/task_manager_browsertest_util.* should be grouped together
and browser/task_manager/task_manager_browsertest.cc can be standalone?

https://codereview.chromium.org/1912773002/diff/80001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:2290: 'dependencies': [
indentation is off

[afakhry, 2016-04-25 20:02:51]

https://codereview.chromium.org/1912773002/diff/80001/chrome/browser/task_man...
File chrome/browser/task_management/task_manager_interface.cc (right):

https://codereview.chromium.org/1912773002/diff/80001/chrome/browser/task_man...
chrome/browser/task_management/task_manager_interface.cc:31: if
(g_browser_process->local_state()) {
On 2016/04/23 00:52:32, Lei Zhang wrote:
> auto* state = g_browser_process->local_state();
> return state ? state->GetBoolean(prefs::kTaskManagerEndProcessEnabled) : true;

Yup, shouldn't have just copied and pasted from the other file. Thanks!

Done.

https://codereview.chromium.org/1912773002/diff/80001/chrome/browser/task_man...
File chrome/browser/task_manager/task_manager.h (right):

https://codereview.chromium.org/1912773002/diff/80001/chrome/browser/task_man...
chrome/browser/task_manager/task_manager.h:33: //
-----------------------------------------------------------------------------
On 2016/04/23 00:52:32, Lei Zhang wrote:
> Do we have a timeline for when this gets removed? If it may be a while, and
you
> are paranoid others may disregard this warning, you many want to torpedo any
> attempts to add this file.
> 
> #if !defined(OS_MACOSX)
> #error "I sunk your battleship"
> #end
> 
> Whatever you decide on, do the same in task_manager_browsertest_util.h?

Great idea! Done.

https://codereview.chromium.org/1912773002/diff/80001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1912773002/diff/80001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:2988: # New Task Manager Sources:
On 2016/04/23 00:52:32, Lei Zhang wrote:
> not needed anymore?

Done.

https://codereview.chromium.org/1912773002/diff/80001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:3059:
'chrome_browser_old_task_manager_macosx_sources': [
On 2016/04/23 00:52:32, Lei Zhang wrote:
> nit: omit 'osx', ditto for other gypi files

Done.

https://codereview.chromium.org/1912773002/diff/80001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:3504: },
On 2016/04/23 00:52:32, Lei Zhang wrote:
> I think this only works because gyp added else-if at some point. It still
looks
> odd because the indentation is off.
> 
> Maybe do what line 3921 does?
> 
> ['chromeos==1' {
>   ...
> }],
> ['OS=="mac"' {
>   ...
> }],

Done.

https://codereview.chromium.org/1912773002/diff/80001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1912773002/diff/80001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:1713:
'browser/task_manager/task_manager_browsertest_util.cc',
On 2016/04/23 00:52:32, Lei Zhang wrote:
> A bit weird to have this standalone.
> 
> browser/task_manager/task_manager_browsertest_util.* should be grouped
together
> and browser/task_manager/task_manager_browsertest.cc can be standalone?


browser/task_manager/task_manager_browsertest_util.cc here is added as part of
the target "interactive_ui_tests" which "task_manager_browsertest.cc" doesn't
belong to.

https://codereview.chromium.org/1912773002/diff/80001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:2290: 'dependencies': [
On 2016/04/23 00:52:32, Lei Zhang wrote:
> indentation is off

Done.

[Lei Zhang, 2016-04-25 20:34:24]

lgtm

[afakhry, 2016-04-25 20:36:07]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2016-04-25 20:36:52]

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

[afakhry, 2016-04-25 20:37:14]

The CQ bit was unchecked by afakhry@chromium.org

[tapted, 2016-04-26 01:08:07]

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

[tapted, 2016-04-26 01:08:09]

sorry not lgtm - it looks like this is removing test coverage that shouldn't be
removed.

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/app_background_page_apitest.cc:307: // Old task
manager browser tests should only run on Mac.
I'm not sure this is testing the old task manager - just running a test that
requires the task manager to be running for some reason? I don't really
understand the mechanisms for it though. But, certainly, this test existed
_before_ r229862 when the chrome::ShowTaskManager(..) line was added, so it
would be incorrect to remove the entire test on any platform.

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:2485: // Old task manager
browser tests should only run on Mac.
We should look into these too - someone needs to own the crbug for converting
"old" task manager tests to "new" task manager tests, and it should be cited
here. It should be someone from the team that added the new task manager. You
can't just dump this onto the Mac team when we transition to the new task
manager and want to delete this code.

[tapted, 2016-04-26 01:54:04]

a few more comments too (sorry the gyp/gn stuff is confusing while we're
migrating both views UI and away from GYP at the same time on Mac)

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/task_ma...
File chrome/browser/task_management/task_manager_interface.cc (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_interface.cc:31: auto* state =
g_browser_process->local_state();
nit: auto* -> PrefService*  (I don't think this meets the "code is more readable
with auto" requirement)

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_interface.cc:32: return state ?
state->GetBoolean(prefs::kTaskManagerEndProcessEnabled) : true;
optional-nit: I'd write this `return !state || state->GetBoolean(..)`

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_interface.cc:48:
TaskManager::GetInstance()->model()->NotifyBytesRead(request, bytes_read);
You can already show the new task manager on Mac -- it's currently behind
chrome::ToolkitViewsDialogsEnabled() -- see r386897 and task_manager_mac.mm

I think this should still call a `NewTaskManagerEnabled()` method between the
#if MAC bits, but you can move it out of the switches:: namespace to somewhere
mac-specific -- probably c/b/task_manager/task_manager.h, since you can't
#include c/b/ui/cocoa from here.

Then task_manager_mac.mm should call it as well (and it should just wrap
chrome::ToolkitViewsDialogsEnabled() - but there shouldn't be multiple
task-manager code locations calling chrome::ToolkitViewsDialogsEnabled())

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/task_ma...
File chrome/browser/task_manager/printing_information.cc (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/task_ma...
chrome/browser/task_manager/printing_information.cc:32: #if
defined(ENABLE_PRINT_PREVIEW)
should this file just be excluded via gyp/gn rules?

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/new_task_manager_view.cc (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/new_task_manager_view.cc:391: // Declared in
browser_dialogs.h so others don't need to depend on our header.
There is also 

chrome/browser/ui/views/browser_dialogs_views.h

(it's fine for that to #include new_task_manager_view.h)

But approach is OK too

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3060: 'chrome_browser_old_task_manager_mac_sources':
[
_mac_sources -> _cocoa_sources

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3061: # Old Task Manager Sources enabled only on mac:
.. enabled only for the Mac Cocoa UI.

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3506: ['OS=="mac"', {
'OS=="mac"' -> 'OS=="mac"' and mac_views_browser==0'

(there's a pattern for this in the _ui.gypi and friends, but we haven't needed
it outside of chrome/browser/ui yet)

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:604:
'browser/task_manager/task_manager_browsertest.cc',
These should go in chrome_browser_tests_cocoa_sources so they get excluded when
the `mac_views_browser=1` flag is set

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:1712:
'browser/task_manager/task_manager_browsertest_util.cc',
these should be repeated in chrome_interactive_ui_test_cocoa_sources instead of
being repeated in a separate rule here.

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_tests_un...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_tests_un...
chrome/chrome_tests_unit.gypi:726:
'browser/task_manager/task_manager_unittest.cc',
Move these to chrome_unit_tests_cocoa_sources (with the comment) - we don't need
the extra variable

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_tests_un...
chrome/chrome_tests_unit.gypi:2444: ['enable_task_manager==1', {
It's weird to have this for test targets but not the main browser in
chrome_browser.gypi [we should probably remove enable_task_manager and just use
!android, but that's orthogonal). I guess this CL doesn't make things
significantly worse.

[tapted, 2016-04-26 03:25:25]

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/new_task_manager_view.cc (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/new_task_manager_view.cc:391: // Declared in
browser_dialogs.h so others don't need to depend on our header.
On 2016/04/26 01:54:03, tapted wrote:
> There is also 
> 
> chrome/browser/ui/views/browser_dialogs_views.h
> 
> (it's fine for that to #include new_task_manager_view.h)
> 
> But approach is OK too

Ah, actually it's not ;). With mac_views_browser=1, it will give 

Undefined symbols for architecture x86_64:
  "chrome::HideTaskManager()", referenced from:
      profiles::LockBrowserCloseSuccess(base::FilePath const&) in
libbrowser.a(browser.profile_window.o)
  "chrome::ShowTaskManager(Browser*)", referenced from:
      chrome::OpenTaskManager(Browser*) in
libbrowser_ui.a(browser_ui.browser_commands.o)

But to fix you just need to remove the #if !defined(OS_MACOSX)

Both new_task_manager_view.cc and
chrome/browser/ui/views/browser_dialogs_views.cc are only built on mac if
mac_views_browser=1 is set (which excludes the cocoa implementation), so there
won't be a duplicate symbol.

[tapted, 2016-04-26 05:51:07]

On 2016/04/26 03:25:25, tapted wrote:
>
https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/ui/view...
> File chrome/browser/ui/views/new_task_manager_view.cc (right):
> 
>
https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/ui/view...
> chrome/browser/ui/views/new_task_manager_view.cc:391: // Declared in
> browser_dialogs.h so others don't need to depend on our header.
> On 2016/04/26 01:54:03, tapted wrote:
> > There is also 
> > 
> > chrome/browser/ui/views/browser_dialogs_views.h
> > 
> > (it's fine for that to #include new_task_manager_view.h)
> > 
> > But approach is OK too
> 
> Ah, actually it's not ;). With mac_views_browser=1, it will give 
> 
> Undefined symbols for architecture x86_64:
>   "chrome::HideTaskManager()", referenced from:
>       profiles::LockBrowserCloseSuccess(base::FilePath const&) in
> libbrowser.a(browser.profile_window.o)
>   "chrome::ShowTaskManager(Browser*)", referenced from:
>       chrome::OpenTaskManager(Browser*) in
> libbrowser_ui.a(browser_ui.browser_commands.o)
> 
> But to fix you just need to remove the #if !defined(OS_MACOSX)
> 
> Both new_task_manager_view.cc and
> chrome/browser/ui/views/browser_dialogs_views.cc are only built on mac if
> mac_views_browser=1 is set (which excludes the cocoa implementation), so there
> won't be a duplicate symbol.

oops that's not true for new_task_manager_view.cc since r386897. So these really
do need to move to browser_dialogs_views.cc

[ncarter (slow), 2016-04-26 20:04:35]

It looks like the old tests have the capacity to uncover bugs in the new TM, so
I think it's worth the effort to make the WaitForTaskManagerRows() style tests
work with the new TM, as tapted and others have suggested.

Let's hold off on this CL until we sort that out.

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:2485: // Old task manager
browser tests should only run on Mac.
On 2016/04/26 01:08:08, tapted wrote:
> We should look into these too - someone needs to own the crbug for converting
> "old" task manager tests to "new" task manager tests, and it should be cited
> here. It should be someone from the team that added the new task manager. You
> can't just dump this onto the Mac team when we transition to the new task
> manager and want to delete this code.

There is, actually, new-taskmanager coverage in this file (see the bottom) but
the tests aren't quite as strong (e.g. they don't guarantee that titles
eventually show up in the task manager).

However I fully agree with the point about not losing coverage we have. Prompted
by this discussion, I'm working on making WaitForTaskManagerRows() work against
the new TaskManager too -- https://codereview.chromium.org/1922683003/ .

[afakhry, 2016-04-26 22:22:44]

I'll be holding off on this CL until Nick lands his.

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/app_background_page_apitest.cc:307: // Old task
manager browser tests should only run on Mac.
On 2016/04/26 01:08:08, tapted wrote:
> I'm not sure this is testing the old task manager - just running a test that
> requires the task manager to be running for some reason? I don't really
> understand the mechanisms for it though. But, certainly, this test existed
> _before_ r229862 when the chrome::ShowTaskManager(..) line was added, so it
> would be incorrect to remove the entire test on any platform.

You're right. Thanks for catching that!
However the crash in http://crbug.com/165138 is only relevant to the old task
manager code, but it doesn't hurt to open the new task manager on the other
platforms too.

Done.

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:2485: // Old task manager
browser tests should only run on Mac.
On 2016/04/26 01:08:08, tapted wrote:
> We should look into these too - someone needs to own the crbug for converting
> "old" task manager tests to "new" task manager tests, and it should be cited
> here. It should be someone from the team that added the new task manager. You
> can't just dump this onto the Mac team when we transition to the new task
> manager and want to delete this code.

I'm the one who wrote the new task manager with ncarter@ as the main reviewer. 

- These tests here have corresponding tests for the new implementation (See
here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre...)

- ncarter@ is currently working on a CL to port these old tests to the new
implementation as well.

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/task_ma...
File chrome/browser/task_management/task_manager_interface.cc (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_interface.cc:31: auto* state =
g_browser_process->local_state();
On 2016/04/26 01:54:03, tapted wrote:
> nit: auto* -> PrefService*  (I don't think this meets the "code is more
readable
> with auto" requirement)

Done.

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_interface.cc:32: return state ?
state->GetBoolean(prefs::kTaskManagerEndProcessEnabled) : true;
On 2016/04/26 01:54:03, tapted wrote:
> optional-nit: I'd write this `return !state || state->GetBoolean(..)`

I like it. Done!

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_interface.cc:48:
TaskManager::GetInstance()->model()->NotifyBytesRead(request, bytes_read);
On 2016/04/26 01:54:03, tapted wrote:
> You can already show the new task manager on Mac -- it's currently behind
> chrome::ToolkitViewsDialogsEnabled() -- see r386897 and task_manager_mac.mm
> 
> I think this should still call a `NewTaskManagerEnabled()` method between the
> #if MAC bits, but you can move it out of the switches:: namespace to somewhere
> mac-specific -- probably c/b/task_manager/task_manager.h, since you can't
> #include c/b/ui/cocoa from here.
> 
> Then task_manager_mac.mm should call it as well (and it should just wrap
> chrome::ToolkitViewsDialogsEnabled() - but there shouldn't be multiple
> task-manager code locations calling chrome::ToolkitViewsDialogsEnabled())

Thanks! Done.

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/task_ma...
File chrome/browser/task_manager/printing_information.cc (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/task_ma...
chrome/browser/task_manager/printing_information.cc:32: #if
defined(ENABLE_PRINT_PREVIEW)
On 2016/04/26 01:54:03, tapted wrote:
> should this file just be excluded via gyp/gn rules?

We can do that. This if def matches the already existing in
printing_information.h
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/tas...

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/new_task_manager_view.cc (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/new_task_manager_view.cc:391: // Declared in
browser_dialogs.h so others don't need to depend on our header.
On 2016/04/26 03:25:24, tapted wrote:
> On 2016/04/26 01:54:03, tapted wrote:
> > There is also 
> > 
> > chrome/browser/ui/views/browser_dialogs_views.h
> > 
> > (it's fine for that to #include new_task_manager_view.h)
> > 
> > But approach is OK too
> 
> Ah, actually it's not ;). With mac_views_browser=1, it will give 
> 
> Undefined symbols for architecture x86_64:
>   "chrome::HideTaskManager()", referenced from:
>       profiles::LockBrowserCloseSuccess(base::FilePath const&) in
> libbrowser.a(browser.profile_window.o)
>   "chrome::ShowTaskManager(Browser*)", referenced from:
>       chrome::OpenTaskManager(Browser*) in
> libbrowser_ui.a(browser_ui.browser_commands.o)
> 
> But to fix you just need to remove the #if !defined(OS_MACOSX)
> 
> Both new_task_manager_view.cc and
> chrome/browser/ui/views/browser_dialogs_views.cc are only built on mac if
> mac_views_browser=1 is set (which excludes the cocoa implementation), so there
> won't be a duplicate symbol.

There are no browser_dialogs_views.h.

There will be duplicate symbols if we remove #if !defined(OS_MACOSX) here
(please see
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_...).

I don't think task_manager_mac.mm is excluded when mac_views_browser=1, right?
Isn't starting the views implementation is based on a runtime switch:
switches::kEnableMacViewsDialogs?

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3060: 'chrome_browser_old_task_manager_mac_sources':
[
On 2016/04/26 01:54:04, tapted wrote:
> _mac_sources -> _cocoa_sources

Done.

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3061: # Old Task Manager Sources enabled only on mac:
On 2016/04/26 01:54:04, tapted wrote:
> .. enabled only for the Mac Cocoa UI.

Done.

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3506: ['OS=="mac"', {
On 2016/04/26 01:54:03, tapted wrote:
> 'OS=="mac"' -> 'OS=="mac"' and mac_views_browser==0'
> 
> (there's a pattern for this in the _ui.gypi and friends, but we haven't needed
> it outside of chrome/browser/ui yet)

I'm confused now. Do we want to exclude this at compile time? Or compile both
and enable/disable the views implementation based on the runtime switch
switches::kEnableMacViewsDialogs?

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:604:
'browser/task_manager/task_manager_browsertest.cc',
On 2016/04/26 01:54:04, tapted wrote:
> These should go in chrome_browser_tests_cocoa_sources so they get excluded
when
> the `mac_views_browser=1` flag is set

Done.

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:1712:
'browser/task_manager/task_manager_browsertest_util.cc',
On 2016/04/26 01:54:04, tapted wrote:
> these should be repeated in chrome_interactive_ui_test_cocoa_sources instead
of
> being repeated in a separate rule here.

Done.

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_tests_un...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_tests_un...
chrome/chrome_tests_unit.gypi:726:
'browser/task_manager/task_manager_unittest.cc',
On 2016/04/26 01:54:04, tapted wrote:
> Move these to chrome_unit_tests_cocoa_sources (with the comment) - we don't
need
> the extra variable

Done.

[tapted, 2016-04-26 22:43:50]

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/new_task_manager_view.cc (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/new_task_manager_view.cc:391: // Declared in
browser_dialogs.h so others don't need to depend on our header.
On 2016/04/26 22:22:43, afakhry wrote:
> On 2016/04/26 03:25:24, tapted wrote:
> > On 2016/04/26 01:54:03, tapted wrote:
> > > There is also 
> > > 
> > > chrome/browser/ui/views/browser_dialogs_views.h
> > > 
> > > (it's fine for that to #include new_task_manager_view.h)
> > > 
> > > But approach is OK too
> > 
> > Ah, actually it's not ;). With mac_views_browser=1, it will give 
> > 
> > Undefined symbols for architecture x86_64:
> >   "chrome::HideTaskManager()", referenced from:
> >       profiles::LockBrowserCloseSuccess(base::FilePath const&) in
> > libbrowser.a(browser.profile_window.o)
> >   "chrome::ShowTaskManager(Browser*)", referenced from:
> >       chrome::OpenTaskManager(Browser*) in
> > libbrowser_ui.a(browser_ui.browser_commands.o)
> > 
> > But to fix you just need to remove the #if !defined(OS_MACOSX)
> > 
> > Both new_task_manager_view.cc and
> > chrome/browser/ui/views/browser_dialogs_views.cc are only built on mac if
> > mac_views_browser=1 is set (which excludes the cocoa implementation), so
there
> > won't be a duplicate symbol.
> 
> There are no browser_dialogs_views.h.
> 
> There will be duplicate symbols if we remove #if !defined(OS_MACOSX) here
> (please see
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_...).

Sorry - typo. it should go in browser_dialogs_views.cc[1] with
LoginHandler::Create and  BookmarkEditor::Show - there's a long comment in that
file explaining its purpose. Note its excluded on a mac *cocoa* build so there
shouldn't be duplicate symbols.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

> 
> I don't think task_manager_mac.mm is excluded when mac_views_browser=1, right?
> Isn't starting the views implementation is based on a runtime switch:
> switches::kEnableMacViewsDialogs?

task_manager_mac.mm is excluded at compile time when mac_views_browser=1 is set
in gyp (all of chrome_browser_ui_cocoa_sources is)

Apologies again - I know it's confusing. I'll send you a diff after testing a
local mac_views_browser=1 build

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1912773002/diff/100001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3506: ['OS=="mac"', {
On 2016/04/26 22:22:43, afakhry wrote:
> On 2016/04/26 01:54:03, tapted wrote:
> > 'OS=="mac"' -> 'OS=="mac"' and mac_views_browser==0'
> > 
> > (there's a pattern for this in the _ui.gypi and friends, but we haven't
needed
> > it outside of chrome/browser/ui yet)
> 
> I'm confused now. Do we want to exclude this at compile time? Or compile both
> and enable/disable the views implementation based on the runtime switch
> switches::kEnableMacViewsDialogs? 

so the "old" task manager stuff is only needed for the _cocoa_ mac
implementation -- not the _views_ mac implementation (which should always be the
new task manager). mac_views_browser is the compile time flag controlling this
(there's a builder: go/macviewsbuilder). So if mac_views_browser=1 then there
should be no* cocoa UI dependencies on mac.

*no = well, there's a few, but most of c/b/ui/cocoa is omitted.

[tapted, 2016-04-27 13:01:45]

I'm toying with a diff in http://crrev.com/1924683003 but I ran out of time to
get the compile step green today. Note there's also a recent regression in the
build (http://crbug.com/607017) which should get fixed soon, so if you want to
test with mac_views_browser=1 you'll need to revert r389779

See comments below.

The remaining parts are what to do about the PrerenderBrowserTest and the
WebViewTests that have been #ifdef'd out -- #ifdef MAC doesn't work when there
are builds on mac that don't include the cocoa symbols, which is what
mac_views_browser=1 is.

If they're not testing things specific to the old task manager UI, and there's
coverage of the same things using the new task manager UI, then I think those
tests should just be deleted, rather than #ifdef'd out. If you think they should
be kept, they need to be moved to a separate file that is only in the _cocoa_
gyp variables.

https://codereview.chromium.org/1912773002/diff/120001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/1912773002/diff/120001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_browsertest.cc:1493:
IN_PROC_BROWSER_TEST_P(WebViewTest, TaskManagerExistingWebView) {
These need to be moved to a separate file which is included only in a _cocoa_
variable. Or possibly just deleted if there's coverage with the new task manager
UI. See errors in
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...

https://codereview.chromium.org/1912773002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/1912773002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:2487:
IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, OpenTaskManagerBeforePrerender) {
same here - these need to go to a separate file or just deleted

https://codereview.chromium.org/1912773002/diff/120001/chrome/browser/task_ma...
File chrome/browser/task_management/task_manager_interface.cc (right):

https://codereview.chromium.org/1912773002/diff/120001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_interface.cc:49:
TaskManager::GetInstance()->model()->NotifyBytesRead(request, bytes_read);
This approach doesn't really work from outside of chrome/browser/ui/cocoa, since
that's gone with mac_views_browser=1 and the TaskManager symbols don't exist --
see
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...
for the error.

To fix, I added a call to `bool NotifyOldTaskManagerBytesRead(..)` and gave it
two definitions. See the diffs in http://crrev.com/1924683003

https://codereview.chromium.org/1912773002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc
(right):

https://codereview.chromium.org/1912773002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc:26:
#include "chrome/browser/task_manager/task_manager_browsertest_util.h"
remove - see below

https://codereview.chromium.org/1912773002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc:93:
IN_PROC_BROWSER_TEST_F(PrintPreviewTest, DISABLED_TaskManagerNewPrintPreview) {
These DISABLED_ tests should just be deleted. The code won't link on a
mac_views_browser=1 build. See
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...

[Evan Stade, 2016-05-18 22:19:09]

ping Ahmed --- what's the status of this?

[afakhry, 2016-05-18 22:23:13]

On 2016/05/18 22:19:09, Evan Stade wrote:
> ping Ahmed --- what's the status of this?

I will resume working on it shortly. I'll have to land another one first
https://codereview.chromium.org/1989393003/.

[Evan Stade, 2016-05-18 22:24:37]

On 2016/05/18 22:23:13, afakhry wrote:
> On 2016/05/18 22:19:09, Evan Stade wrote:
> > ping Ahmed --- what's the status of this?
> 
> I will resume working on it shortly. I'll have to land another one first
> https://codereview.chromium.org/1989393003/.

great, thanks

[afakhry, 2016-05-19 17:06:55]

Description was changed from

==========
Remove the old task manager view

This CL does the following:
- Enables the old task manager back end implementation and its tests
  only on MacOSX.
- Removes the --disable-new-task-manager flag.
- Removes the old task manager view

BUG=604806
==========

to

==========
Remove the old task manager view

This CL does the following:
- Enables the old task manager backend implementation only on MacOSX cocoa.
- Removes the --disable-new-task-manager flag.
- Removes the old task manager view

BUG=604806
==========

[afakhry, 2016-05-19 18:29:53]

ncarter, and tapted, Please take a look. The CL is now much easier after Nick
had ported the old TM browsertests to the new TM impl.

[ncarter (slow), 2016-05-19 22:25:08]

lgtm assuming caps/ is totally dead code that we're okay deleting

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/task_ma...
File chrome/browser/task_management/task_manager_tester.cc (right):

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_tester.cc:29: bool
IsNewTaskManagerViewEnabled() {
I would just inline this function. It's only called in two spots, one of which
is already #ifdef'd, and the other of which is a CHECK that may not even be
necessary.

[tapted, 2016-05-20 00:08:45]

The mac_views_browser build is tricky.. I should have some time today (overnight
for you) to update the diff I had going in http://crrev.com/1924683003 -
currently this will break/regress the compile step at go/macviewsbuilder with

Undefined symbols for architecture x86_64:
  "TaskManager::GetInstance()", referenced from:
      task_management::TaskManagerInterface::OnRawBytesRead(net::URLRequest
const&, long long) in task_manager_interface.o
  "TaskManagerModel::NotifyBytesRead(net::URLRequest const&, long long)",
referenced from:
      task_management::TaskManagerInterface::OnRawBytesRead(net::URLRequest
const&, long long) in task_manager_interface.o
  "chrome::HideTaskManager()", referenced from:
      profiles::LockBrowserCloseSuccess(base::FilePath const&) in
profile_window.o
  "chrome::ShowTaskManager(Browser*)", referenced from:
      chrome::OpenTaskManager(Browser*) in browser_commands.o

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/task_ma...
File chrome/browser/task_management/task_manager_interface.cc (right):

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_interface.cc:17: #include
"chrome/browser/task_manager/task_manager.h"
This can't be included here - task_manager.h isn't part of the a mac views
browser build, (but of course OS_MACOSX is defined there).

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_interface.cc:56: if
(!TaskManagerInterface::IsNewTaskManagerEnabled()) {
My suggested fix is in
https://codereview.chromium.org/1924683003/diff/100001/chrome/browser/task_ma...

It adds a method to task_manager_mac.mm (which *isn't* build on
mac_views_browser), which goes like:

bool NotifyOldTaskManagerBytesRead(const net::URLRequest& request,
                                   int64_t bytes_read) {
  if (TaskManager::NewTaskManagerEnabled())
    return false;

  TaskManager::GetInstance()->model()->NotifyBytesRead(request, bytes_read);
  return true;
}

Then NotifyOldTaskManagerBytesRead (which is declared in browser_dialogs.h) gets
called here instead. Another version of NotifyOldTaskManagerBytesRead for the
mac views browser goes in browser_dialogs_views.cc which always returns false.

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/task_ma...
File chrome/browser/task_manager/printing_information.h (right):

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/task_ma...
chrome/browser/task_manager/printing_information.h:8: #if
defined(ENABLE_PRINT_PREVIEW)
Are these guards still needed? If these are moving to a foo_cocoa_sources build
variable, I think we can assume it's the Chrome Mac+Cocoa build and
ENABLE_PRINT_PREVIEW is always true.

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/new_task_manager_view.cc (right):

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/new_task_manager_view.cc:371: ui::TableModel*
ShowTaskManager(Browser* browser) {
These need to go in browser_dialogs_views.cc this allows a mac_views_browser
build to get at them. They can't be guarded by OS_MACOSX

[afakhry, 2016-05-20 00:58:19]

Description was changed from

==========
Remove the old task manager view

This CL does the following:
- Enables the old task manager backend implementation only on MacOSX cocoa.
- Removes the --disable-new-task-manager flag.
- Removes the old task manager view

BUG=604806
==========

to

==========
Remove the old task manager view

This CL does the following:
- Enables the old task manager backend implementation only on MacOSX cocoa.
- Removes the --disable-new-task-manager flag.
- Removes the old task manager view

BUG=604806
CQ_INCLUDE_TRYBOTS=tryserver.chromium.mac:mac_chromium_10.10_macviews
==========

[afakhry, 2016-05-20 02:32:50]

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/task_ma...
File chrome/browser/task_management/task_manager_interface.cc (right):

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_interface.cc:17: #include
"chrome/browser/task_manager/task_manager.h"
On 2016/05/20 00:08:45, tapted wrote:
> This can't be included here - task_manager.h isn't part of the a mac views
> browser build, (but of course OS_MACOSX is defined there).

Done.

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_interface.cc:56: if
(!TaskManagerInterface::IsNewTaskManagerEnabled()) {
On 2016/05/20 00:08:45, tapted wrote:
> My suggested fix is in
>
https://codereview.chromium.org/1924683003/diff/100001/chrome/browser/task_ma...
> 
> It adds a method to task_manager_mac.mm (which *isn't* build on
> mac_views_browser), which goes like:
> 
> bool NotifyOldTaskManagerBytesRead(const net::URLRequest& request,
>                                    int64_t bytes_read) {
>   if (TaskManager::NewTaskManagerEnabled())
>     return false;
> 
>   TaskManager::GetInstance()->model()->NotifyBytesRead(request, bytes_read);
>   return true;
> }
> 
> Then NotifyOldTaskManagerBytesRead (which is declared in browser_dialogs.h)
gets
> called here instead. Another version of NotifyOldTaskManagerBytesRead for the
> mac views browser goes in browser_dialogs_views.cc which always returns false.

Done.

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/task_ma...
File chrome/browser/task_management/task_manager_tester.cc (right):

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_tester.cc:29: bool
IsNewTaskManagerViewEnabled() {
On 2016/05/19 22:25:07, ncarter wrote:
> I would just inline this function. It's only called in two spots, one of which
> is already #ifdef'd, and the other of which is a CHECK that may not even be
> necessary.

Done.

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/task_ma...
File chrome/browser/task_manager/printing_information.h (right):

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/task_ma...
chrome/browser/task_manager/printing_information.h:8: #if
defined(ENABLE_PRINT_PREVIEW)
On 2016/05/20 00:08:45, tapted wrote:
> Are these guards still needed? If these are moving to a foo_cocoa_sources
build
> variable, I think we can assume it's the Chrome Mac+Cocoa build and
> ENABLE_PRINT_PREVIEW is always true.

Done.

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/new_task_manager_view.cc (right):

https://codereview.chromium.org/1912773002/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/new_task_manager_view.cc:371: ui::TableModel*
ShowTaskManager(Browser* browser) {
On 2016/05/20 00:08:45, tapted wrote:
> These need to go in browser_dialogs_views.cc this allows a mac_views_browser
> build to get at them. They can't be guarded by OS_MACOSX

Done.

https://codereview.chromium.org/1912773002/diff/220001/chrome/browser/task_ma...
File chrome/browser/task_management/task_manager_tester.cc (right):

https://codereview.chromium.org/1912773002/diff/220001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_tester.cc:168: return
task_manager::CreateLegacyTaskManagerTester(callback);
I suspect that this might cause problems since the
legacy_task_manager_tester.cc/h are part of the cocoa_sources. Any suggestions?

[tapted, 2016-05-20 09:52:34]

https://codereview.chromium.org/1912773002/diff/220001/chrome/browser/task_ma...
File chrome/browser/task_management/task_manager_tester.cc (right):

https://codereview.chromium.org/1912773002/diff/220001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_tester.cc:168: return
task_manager::CreateLegacyTaskManagerTester(callback);
On 2016/05/20 02:32:50, afakhry wrote:
> I suspect that this might cause problems since the
> legacy_task_manager_tester.cc/h are part of the cocoa_sources. Any
suggestions?

Hm - you're quite right. For some reason when I locally built I didn't get a
link error here, but I did when I ran a tryjob in
https://codereview.chromium.org/1924683003/ . So unfortunately I ran out of time
to probe this further.

I think part of the fix is just to create a version of TaskManagerTester::Create
in legacy_task_manager_tester.cc - one that's just for Cocoa.

Then I .. think this file can just be omitted from Cocoa builds altogether? The
stuff here between #if MAC can just be removed.

Anyway, you should be able to test it out but uploading a CL branched of this
that looks just like https://codereview.chromium.org/1924683003/#ps260001 - if
that gets a green 'compile' run (don't worry about the rest), then I don't think
I have any other objections.

[tapted, 2016-05-23 04:34:56]

https://codereview.chromium.org/1912773002/diff/220001/chrome/browser/task_ma...
File chrome/browser/task_management/task_manager_tester.cc (right):

https://codereview.chromium.org/1912773002/diff/220001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_tester.cc:168: return
task_manager::CreateLegacyTaskManagerTester(callback);
On 2016/05/20 09:52:33, tapted wrote:
> On 2016/05/20 02:32:50, afakhry wrote:
> > I suspect that this might cause problems since the
> > legacy_task_manager_tester.cc/h are part of the cocoa_sources. Any
> suggestions?
> 
> Hm - you're quite right. For some reason when I locally built I didn't get a
> link error here, but I did when I ran a tryjob in
> https://codereview.chromium.org/1924683003/ . So unfortunately I ran out of
time
> to probe this further.
> 
> I think part of the fix is just to create a version of
TaskManagerTester::Create
> in legacy_task_manager_tester.cc - one that's just for Cocoa.
> 
> Then I .. think this file can just be omitted from Cocoa builds altogether?
The
> stuff here between #if MAC can just be removed.
> 
> Anyway, you should be able to test it out but uploading a CL branched of this
> that looks just like https://codereview.chromium.org/1924683003/#ps260001 - if
> that gets a green 'compile' run (don't worry about the rest), then I don't
think
> I have any other objections.

OK - I've updated https://codereview.chromium.org/1924683003/#ps320001 with a
fix. If you patch that in (minus the common.gypi change), I think everything
will be good.

[afakhry, 2016-05-23 18:20:49]

Description was changed from

==========
Remove the old task manager view

This CL does the following:
- Enables the old task manager backend implementation only on MacOSX cocoa.
- Removes the --disable-new-task-manager flag.
- Removes the old task manager view

BUG=604806
CQ_INCLUDE_TRYBOTS=tryserver.chromium.mac:mac_chromium_10.10_macviews
==========

to

==========
Remove the old task manager view

This CL does the following:
- Enables the old task manager backend implementation only on MacOSX cocoa.
- Removes the --disable-new-task-manager flag.
- Removes the old task manager view

BUG=604806
==========

[afakhry, 2016-05-23 18:21:52]

https://codereview.chromium.org/1912773002/diff/220001/chrome/browser/task_ma...
File chrome/browser/task_management/task_manager_tester.cc (right):

https://codereview.chromium.org/1912773002/diff/220001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_tester.cc:168: return
task_manager::CreateLegacyTaskManagerTester(callback);
On 2016/05/20 09:52:33, tapted wrote:
> On 2016/05/20 02:32:50, afakhry wrote:
> > I suspect that this might cause problems since the
> > legacy_task_manager_tester.cc/h are part of the cocoa_sources. Any
> suggestions?
> 
> Hm - you're quite right. For some reason when I locally built I didn't get a
> link error here, but I did when I ran a tryjob in
> https://codereview.chromium.org/1924683003/ . So unfortunately I ran out of
time
> to probe this further.
> 
> I think part of the fix is just to create a version of
TaskManagerTester::Create
> in legacy_task_manager_tester.cc - one that's just for Cocoa.
> 
> Then I .. think this file can just be omitted from Cocoa builds altogether?
The
> stuff here between #if MAC can just be removed.
> 
> Anyway, you should be able to test it out but uploading a CL branched of this
> that looks just like https://codereview.chromium.org/1924683003/#ps260001 -
if
> that gets a green 'compile' run (don't worry about the rest), then I don't
think
> I have any other objections.

You need to build browsertests in order to test this.

I tried to fix this by adding a new file:
legacy_task_manager_tester_mac_views.cc which has a default implementation of
task_manager::CreateLegacyTaskManagerTester(). This file is added to the
chrome_browser_tests_views_non_mac_sources.

Let's see what the trybots will say.

https://codereview.chromium.org/1912773002/diff/220001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_tester.cc:168: return
task_manager::CreateLegacyTaskManagerTester(callback);
On 2016/05/23 04:34:56, tapted wrote:
> On 2016/05/20 09:52:33, tapted wrote:
> > On 2016/05/20 02:32:50, afakhry wrote:
> > > I suspect that this might cause problems since the
> > > legacy_task_manager_tester.cc/h are part of the cocoa_sources. Any
> > suggestions?
> > 
> > Hm - you're quite right. For some reason when I locally built I didn't get a
> > link error here, but I did when I ran a tryjob in
> > https://codereview.chromium.org/1924683003/ . So unfortunately I ran out of
> time
> > to probe this further.
> > 
> > I think part of the fix is just to create a version of
> TaskManagerTester::Create
> > in legacy_task_manager_tester.cc - one that's just for Cocoa.
> > 
> > Then I .. think this file can just be omitted from Cocoa builds altogether?
> The
> > stuff here between #if MAC can just be removed.
> > 
> > Anyway, you should be able to test it out but uploading a CL branched of
this
> > that looks just like https://codereview.chromium.org/1924683003/#ps260001 -
if
> > that gets a green 'compile' run (don't worry about the rest), then I don't
> think
> > I have any other objections.
> 
> OK - I've updated https://codereview.chromium.org/1924683003/#ps320001 with a
> fix. If you patch that in (minus the common.gypi change), I think everything
> will be good.

Done.

[tapted, 2016-05-23 21:04:27]

Awesome lgtm - thanks for bearing with me for the mac_views_browser stuff!

https://codereview.chromium.org/1912773002/diff/280001/chrome/browser/task_ma...
File chrome/browser/task_management/task_manager_tester.h (right):

https://codereview.chromium.org/1912773002/diff/280001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_tester.h:55: // Always creates a
tester for the non-legacy TaskManager.
nit: indent

https://codereview.chromium.org/1912773002/diff/280001/chrome/browser/task_ma...
File chrome/browser/task_manager/task_manager_tester_nonmac.cc (right):

https://codereview.chromium.org/1912773002/diff/280001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_tester_nonmac.cc:32: 
nit: remove extra blank line

[afakhry, 2016-05-24 17:40:27]

Thanks!

https://codereview.chromium.org/1912773002/diff/280001/chrome/browser/task_ma...
File chrome/browser/task_management/task_manager_tester.h (right):

https://codereview.chromium.org/1912773002/diff/280001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_tester.h:55: // Always creates a
tester for the non-legacy TaskManager.
On 2016/05/23 21:04:26, tapted wrote:
> nit: indent

Done.

https://codereview.chromium.org/1912773002/diff/280001/chrome/browser/task_ma...
File chrome/browser/task_manager/task_manager_tester_nonmac.cc (right):

https://codereview.chromium.org/1912773002/diff/280001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_tester_nonmac.cc:32: 
On 2016/05/23 21:04:26, tapted wrote:
> nit: remove extra blank line

Done.

[afakhry, 2016-05-24 17:40:53]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2016-05-24 17:40:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, nick@chromium.org, tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1912773002/#ps300001
(title: "Nits")

[commit-bot: I haz the power, 2016-05-24 17:41:24]

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

[commit-bot: I haz the power, 2016-05-24 19:16:28]

Description was changed from

==========
Remove the old task manager view

This CL does the following:
- Enables the old task manager backend implementation only on MacOSX cocoa.
- Removes the --disable-new-task-manager flag.
- Removes the old task manager view

BUG=604806
==========

to

==========
Remove the old task manager view

This CL does the following:
- Enables the old task manager backend implementation only on MacOSX cocoa.
- Removes the --disable-new-task-manager flag.
- Removes the old task manager view

BUG=604806
==========

[commit-bot: I haz the power, 2016-05-24 19:16:30]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-05-24 19:18:45]

Description was changed from

==========
Remove the old task manager view

This CL does the following:
- Enables the old task manager backend implementation only on MacOSX cocoa.
- Removes the --disable-new-task-manager flag.
- Removes the old task manager view

BUG=604806
==========

to

==========
Remove the old task manager view

This CL does the following:
- Enables the old task manager backend implementation only on MacOSX cocoa.
- Removes the --disable-new-task-manager flag.
- Removes the old task manager view

BUG=604806

Committed: https://crrev.com/17406972b0d3ab7d1448dda982dc294b6cede9a7
Cr-Commit-Position: refs/heads/master@{#395671}
==========

[commit-bot: I haz the power, 2016-05-24 19:18:49]

Patchset 16 (id:??) landed as
https://crrev.com/17406972b0d3ab7d1448dda982dc294b6cede9a7
Cr-Commit-Position: refs/heads/master@{#395671}