Make old task manager tests work against new task manager.

Make old task manager tests work against new task manager.

Re-enable some disabled tests.

BUG=606963, 444945, 528282
Committed: https://crrev.com/399f1066416a5987a10a1bc1c3b0b7eaf40f88b5
Cr-Commit-Position: refs/heads/master@{#392071}

[afakhry, 2016-04-26 02:31:31]

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

[afakhry, 2016-04-26 02:31:32]

https://codereview.chromium.org/1922683003/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (left):

https://codereview.chromium.org/1922683003/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:4014: base::string16());
Why do we have to remove these? They test that the WebContentsTags are
added/removed correctly to the TagsManager.

[ncarter (slow), 2016-04-26 21:55:21]

Description was changed from

==========
Make old task manager tests work against new task manager.

BUG=
==========

to

==========
Make old task manager tests work against new task manager.

BUG=606963
==========

[ncarter (slow), 2016-04-26 22:02:07]

https://codereview.chromium.org/1922683003/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (left):

https://codereview.chromium.org/1922683003/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:4014: base::string16());
On 2016/04/26 02:31:32, afakhry wrote:
> Why do we have to remove these? They test that the WebContentsTags are
> added/removed correctly to the TagsManager.

Are these tests not redundant to
PrerenderBrowserTest.OpenTaskManagerBeforePrerender and
PrerenderBrowserTest.OpenTaskManagerAfterPrerender? It seems like they do pretty
much the same thing, with the asserts at slightly different layers.

Maybe we can merge the tag asserts into the other tests? Let me know which
asserts you think are important to keep.

[ncarter (slow), 2016-04-26 22:06:41]

Description was changed from

==========
Make old task manager tests work against new task manager.

BUG=606963
==========

to

==========
Make old task manager tests work against new task manager.

BUG=606963, 444945
==========

[ncarter (slow), 2016-04-26 22:06:59]

Description was changed from

==========
Make old task manager tests work against new task manager.

BUG=606963, 444945
==========

to

==========
Make old task manager tests work against new task manager.

Re-enable some disabled tests.

BUG=606963, 444945
==========

[afakhry, 2016-04-26 22:15:45]

https://codereview.chromium.org/1922683003/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (left):

https://codereview.chromium.org/1922683003/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:4014: base::string16());
On 2016/04/26 22:02:07, ncarter wrote:
> On 2016/04/26 02:31:32, afakhry wrote:
> > Why do we have to remove these? They test that the WebContentsTags are
> > added/removed correctly to the TagsManager.
> 
> Are these tests not redundant to
> PrerenderBrowserTest.OpenTaskManagerBeforePrerender and
> PrerenderBrowserTest.OpenTaskManagerAfterPrerender? It seems like they do
pretty
> much the same thing, with the asserts at slightly different layers.
> 
> Maybe we can merge the tag asserts into the other tests? Let me know which
> asserts you think are important to keep.

I think of them as two different category of tests. These are for the backend,
and the ones above (old ones) are for the UI expectations.

The benefit of these is that we can simple detect regressions in the back end
independently from the UI.

[ncarter (slow), 2016-04-26 23:00:26]

https://codereview.chromium.org/1922683003/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (left):

https://codereview.chromium.org/1922683003/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:4014: base::string16());
On 2016/04/26 22:15:45, afakhry wrote:
> On 2016/04/26 22:02:07, ncarter wrote:
> > On 2016/04/26 02:31:32, afakhry wrote:
> > > Why do we have to remove these? They test that the WebContentsTags are
> > > added/removed correctly to the TagsManager.
> > 
> > Are these tests not redundant to
> > PrerenderBrowserTest.OpenTaskManagerBeforePrerender and
> > PrerenderBrowserTest.OpenTaskManagerAfterPrerender? It seems like they do
> pretty
> > much the same thing, with the asserts at slightly different layers.
> > 
> > Maybe we can merge the tag asserts into the other tests? Let me know which
> > asserts you think are important to keep.
> 
> I think of them as two different category of tests. These are for the backend,
> and the ones above (old ones) are for the UI expectations.
> 
> The benefit of these is that we can simple detect regressions in the back end
> independently from the UI.

Asserting against PrerenderTitlePrefix seems like it's about UI expectation,
though, right? IIRC this test was written as a "new task manager" version of the
old test.

What bug could these tests catch, that the OpenTaskManager... tests wouldn't
also catch?

[afakhry, 2016-04-27 01:36:21]

https://codereview.chromium.org/1922683003/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (left):

https://codereview.chromium.org/1922683003/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:4014: base::string16());
On 2016/04/26 23:00:26, ncarter wrote:
> On 2016/04/26 22:15:45, afakhry wrote:
> > On 2016/04/26 22:02:07, ncarter wrote:
> > > On 2016/04/26 02:31:32, afakhry wrote:
> > > > Why do we have to remove these? They test that the WebContentsTags are
> > > > added/removed correctly to the TagsManager.
> > > 
> > > Are these tests not redundant to
> > > PrerenderBrowserTest.OpenTaskManagerBeforePrerender and
> > > PrerenderBrowserTest.OpenTaskManagerAfterPrerender? It seems like they do
> > pretty
> > > much the same thing, with the asserts at slightly different layers.
> > > 
> > > Maybe we can merge the tag asserts into the other tests? Let me know which
> > > asserts you think are important to keep.
> > 
> > I think of them as two different category of tests. These are for the
backend,
> > and the ones above (old ones) are for the UI expectations.
> > 
> > The benefit of these is that we can simple detect regressions in the back
end
> > independently from the UI.
> 
> Asserting against PrerenderTitlePrefix seems like it's about UI expectation,
> though, right? IIRC this test was written as a "new task manager" version of
the
> old test.
> 
> What bug could these tests catch, that the OpenTaskManager... tests wouldn't
> also catch?

mmm, I hate it when I'm wrong, but probably nothing. :D 

Probably not related to these tests, but could it be possible that we have a
regression where there are more tags in the tags manager without any
corresponding tasks reported by the WebContentsTaskProvider?

[ncarter (slow), 2016-04-27 16:55:36]

On 2016/04/27 01:36:21, afakhry wrote:
>
https://codereview.chromium.org/1922683003/diff/1/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_browsertest.cc (left):
> 
>
https://codereview.chromium.org/1922683003/diff/1/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_browsertest.cc:4014: base::string16());
> On 2016/04/26 23:00:26, ncarter wrote:
> > On 2016/04/26 22:15:45, afakhry wrote:
> > > On 2016/04/26 22:02:07, ncarter wrote:
> > > > On 2016/04/26 02:31:32, afakhry wrote:
> > > > > Why do we have to remove these? They test that the WebContentsTags are
> > > > > added/removed correctly to the TagsManager.
> > > > 
> > > > Are these tests not redundant to
> > > > PrerenderBrowserTest.OpenTaskManagerBeforePrerender and
> > > > PrerenderBrowserTest.OpenTaskManagerAfterPrerender? It seems like they
do
> > > pretty
> > > > much the same thing, with the asserts at slightly different layers.
> > > > 
> > > > Maybe we can merge the tag asserts into the other tests? Let me know
which
> > > > asserts you think are important to keep.
> > > 
> > > I think of them as two different category of tests. These are for the
> backend,
> > > and the ones above (old ones) are for the UI expectations.
> > > 
> > > The benefit of these is that we can simple detect regressions in the back
> end
> > > independently from the UI.
> > 
> > Asserting against PrerenderTitlePrefix seems like it's about UI expectation,
> > though, right? IIRC this test was written as a "new task manager" version of
> the
> > old test.
> > 
> > What bug could these tests catch, that the OpenTaskManager... tests wouldn't
> > also catch?
> 
> mmm, I hate it when I'm wrong, but probably nothing. :D 
> 
> Probably not related to these tests, but could it be possible that we have a
> regression where there are more tags in the tags manager without any
> corresponding tasks reported by the WebContentsTaskProvider?

If the tests cost nothing to run and maintain, I'd be fine with keeping them.
But I think it's easier on readers of this file, to not have to ponder the
differences between the two styles of taskmanager test. And browser_tests is
slow enough that it's good citizenship to trim redundant tests, since even
simple browsertests cost a few seconds to run.

[ncarter (slow), 2016-04-27 16:57:15]

Missing override

[ncarter (slow), 2016-04-27 17:17:45]

Ahmed, I'm looking for your thoughts on how best to implement
TaskManagerBrowserTestUtil

https://codereview.chromium.org/1922683003/diff/80001/chrome/browser/task_man...
File chrome/browser/task_manager/task_manager_browsertest_util.cc (right):

https://codereview.chromium.org/1922683003/diff/80001/chrome/browser/task_man...
chrome/browser/task_manager/task_manager_browsertest_util.cc:44: // (1) Grab the
view's model directly.
This is what the code does today. It gets the job done, but has the following
issue:

 - #including ui/views from here is forbidden by checkdeps -- I assume for good
reason? (Is that even compiled on the mac?)

 - We need friend declarations in both the model and the view. All we really
need the view for, is to hijack its TaskManagerTableModel instance. (What we
really want, I guess, is a method like
TaskManagerTableModel::GetInstanceForTests())

 - We still have to implement TaskManagerObserver, because we don't have another
way to listen for updates. This is kind of lame.

 - The main reason I want to use the real Model, is so that we are looking at
rows that match what is being painted on the screen. E.g. for tests that look at
V8 memory, I'd like the visible TaskManager to actually show the columns.
However, we bypass the model anyways when grabbing the stats, because all we get
from the model is text, and parsing that out to a number seems like a bad idea.

https://codereview.chromium.org/1922683003/diff/80001/chrome/browser/task_man...
chrome/browser/task_manager/task_manager_browsertest_util.cc:45: // (2)
Instantiate our own TaskManagerTableModel instance and ignore
I think this is probably the best approach for what I'm trying to accomplish.

The main downside I see is that the visibility state of the real TaskManager
becomes totally irrelevant to the functioning of the tests (and a good many of
the tests occur in pairs, that only differ because of show/hide behavior. This
scenario matters so much less in the new TM, since we have the tags approach, so
maybe this is just okay.)

I guess this also means we'd need to provide a stub implementation of
TableViewDelegate? How hard will that be?

https://codereview.chromium.org/1922683003/diff/80001/chrome/browser/task_man...
chrome/browser/task_manager/task_manager_browsertest_util.cc:50: // (4) Make
TaskManagerTableModel support multiple observers, and
This turns out to be pretty gross, since SetObserver() is dictated by
ui::TableModel. I could still back it with an ObserverList internally, and
provide a {Add|Remove}ObserverForTesting() private methods for friends, but it's
more intrusive than I had imagined.

[ncarter (slow), 2016-04-27 21:31:02]

Description was changed from

==========
Make old task manager tests work against new task manager.

Re-enable some disabled tests.

BUG=606963, 444945
==========

to

==========
Make old task manager tests work against new task manager.

Re-enable some disabled tests.

BUG=606963, 444945, 528282
==========

[ncarter (slow), 2016-04-27 21:57:39]

This is ready for real code review. Please have a look.

[afakhry, 2016-04-28 00:03:13]

https://codereview.chromium.org/1922683003/diff/80001/chrome/browser/task_man...
File chrome/browser/task_manager/task_manager_browsertest_util.cc (right):

https://codereview.chromium.org/1922683003/diff/80001/chrome/browser/task_man...
chrome/browser/task_manager/task_manager_browsertest_util.cc:44: // (1) Grab the
view's model directly.
On 2016/04/27 17:17:45, ncarter wrote:
> This is what the code does today. It gets the job done, but has the following
> issue:
> 
>  - #including ui/views from here is forbidden by checkdeps -- I assume for
good
> reason? (Is that even compiled on the mac?)
> 
>  - We need friend declarations in both the model and the view. All we really
> need the view for, is to hijack its TaskManagerTableModel instance. (What we
> really want, I guess, is a method like
> TaskManagerTableModel::GetInstanceForTests())
> 
>  - We still have to implement TaskManagerObserver, because we don't have
another
> way to listen for updates. This is kind of lame.
> 
>  - The main reason I want to use the real Model, is so that we are looking at
> rows that match what is being painted on the screen. E.g. for tests that look
at
> V8 memory, I'd like the visible TaskManager to actually show the columns.
> However, we bypass the model anyways when grabbing the stats, because all we
get
> from the model is text, and parsing that out to a number seems like a bad
idea.

I think this is the best approach since we are actually testing the real model.
I like the way you implemented it in the last patch.

https://codereview.chromium.org/1922683003/diff/80001/chrome/browser/task_man...
chrome/browser/task_manager/task_manager_browsertest_util.cc:45: // (2)
Instantiate our own TaskManagerTableModel instance and ignore
On 2016/04/27 17:17:45, ncarter wrote:
> I think this is probably the best approach for what I'm trying to accomplish.
> 
> The main downside I see is that the visibility state of the real TaskManager
> becomes totally irrelevant to the functioning of the tests (and a good many of
> the tests occur in pairs, that only differ because of show/hide behavior. This
> scenario matters so much less in the new TM, since we have the tags approach,
so
> maybe this is just okay.)
> 
> I guess this also means we'd need to provide a stub implementation of
> TableViewDelegate? How hard will that be?

Your interceptor approach is better.

https://codereview.chromium.org/1922683003/diff/80001/chrome/browser/task_man...
chrome/browser/task_manager/task_manager_browsertest_util.cc:50: // (4) Make
TaskManagerTableModel support multiple observers, and
On 2016/04/27 17:17:45, ncarter wrote:
> This turns out to be pretty gross, since SetObserver() is dictated by
> ui::TableModel. I could still back it with an ObserverList internally, and
> provide a {Add|Remove}ObserverForTesting() private methods for friends, but
it's
> more intrusive than I had imagined.

Yes, this doesn't sound like a clean solution.

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (left):

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:4008: #if
defined(ENABLE_TASK_MANAGER)
Why did we need an ifdef here while we didn't need one up there in the old
tests?

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_manager_impl.h (right):

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.h:96: friend class
TaskManagerTester;
It doesn't look like you need this friendship.

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
File chrome/browser/task_manager/task_manager_browsertest_util.cc (right):

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.cc:26: #include
"chrome/browser/ui/task_manager/task_manager_table_model.h"
I don't see you changed any DEPS. Does that mean you don't have to?

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.cc:43: ui::TableModel*
model,
The naming of the variables can be confusing. Would it be better to name it
|model_to_intercept|.

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.cc:274: model_.reset(
There are too many things that are called |model_| in this file! :D
Can we call this |task_manager_tester_| instead?

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.cc:391: return
base::WrapUnique(
Are we not allowed to use std::make_unique()?

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/ui/task...
File chrome/browser/ui/task_manager/task_manager_table_model.cc (right):

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/ui/task...
chrome/browser/ui/task_manager/task_manager_table_model.cc:276: if
(!g_instance_for_testing)
That's weird! Can that happen? Can we instantiate a new model while another one
is still being used? Seems like an error not be to ignored.

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

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc:112:
DISABLED_TaskManagerExistingPrintPreview) {
Have you tried re-enabling these tests? Maybe they pass now?

[ncarter (slow), 2016-05-02 18:33:28]

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (left):

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:4008: #if
defined(ENABLE_TASK_MANAGER)
On 2016/04/28 00:03:13, afakhry wrote:
> Why did we need an ifdef here while we didn't need one up there in the old
> tests?

Good question, I can't figure this out. I gather this test doesn't compile on
OS_ANDROID?

Anyhow, I've added #if defined(ENABLE_TASK_MANAGER) to the other tests, since
you could set that in your gn config.

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_manager_impl.h (right):

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.h:96: friend class
TaskManagerTester;
On 2016/04/28 00:03:13, afakhry wrote:
> It doesn't look like you need this friendship.

Done.

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
File chrome/browser/task_manager/task_manager_browsertest_util.cc (right):

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.cc:26: #include
"chrome/browser/ui/task_manager/task_manager_table_model.h"
On 2016/04/28 00:03:13, afakhry wrote:
> I don't see you changed any DEPS. Does that mean you don't have to?

It's ui/views (to get the view) that had the deps problem. The model is in
ui/task_manager.

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.cc:43: ui::TableModel*
model,
On 2016/04/28 00:03:13, afakhry wrote:
> The naming of the variables can be confusing. Would it be better to name it
> |model_to_intercept|.

Done.

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.cc:274: model_.reset(
On 2016/04/28 00:03:13, afakhry wrote:
> There are too many things that are called |model_| in this file! :D
> Can we call this |task_manager_tester_| instead?

Done.

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.cc:391: return
base::WrapUnique(
On 2016/04/28 00:03:13, afakhry wrote:
> Are we not allowed to use std::make_unique()?

make_unique is not in C++11, it's C++14. Also, WrapUnique and make_unique
operate very differently. WrapUnique takes a T*, whereas make_unique takes a set
of args for a T ctor, and calls "operator new" for you.

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/ui/task...
File chrome/browser/ui/task_manager/task_manager_table_model.cc (right):

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/ui/task...
chrome/browser/ui/task_manager/task_manager_table_model.cc:276: if
(!g_instance_for_testing)
On 2016/04/28 00:03:13, afakhry wrote:
> That's weird! Can that happen? Can we instantiate a new model while another
one
> is still being used? Seems like an error not be to ignored.

I've added a comment to g_instance_for_testing describing the assumptions.

The view is a singleton, but there's nothing that prevents the model from having
multiple instances: for example, we were debating instantiating a second model
to drive TaskManagerTesterImpl. Since this is just a testing hook, I didn't want
to impose any limits on the behavior of TaskManagerTableModel or its unittests.
Especially since the model not being a singleton is a nice attribute of the new
task manager.

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

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc:112:
DISABLED_TaskManagerExistingPrintPreview) {
On 2016/04/28 00:03:13, afakhry wrote:
> Have you tried re-enabling these tests? Maybe they pass now?

These tests have been historically pretty flaky. I'd want to really dig into
them before reenabling.

I actually think the tags-based tests you've added for printing are better tests
overall. Maybe the thing to do here is to merge these into those.

[afakhry, 2016-05-02 18:58:18]

lgtm with a couple of minor nits.

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
File chrome/browser/task_manager/task_manager_browsertest_util.cc (right):

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.cc:26: #include
"chrome/browser/ui/task_manager/task_manager_table_model.h"
On 2016/05/02 18:33:28, ncarter wrote:
> On 2016/04/28 00:03:13, afakhry wrote:
> > I don't see you changed any DEPS. Does that mean you don't have to?
> 
> It's ui/views (to get the view) that had the deps problem. The model is in
> ui/task_manager.

Oh cool. Glad I moved there earlier! :)

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.cc:274: model_.reset(
On 2016/05/02 18:33:28, ncarter wrote:
> On 2016/04/28 00:03:13, afakhry wrote:
> > There are too many things that are called |model_| in this file! :D
> > Can we call this |task_manager_tester_| instead?
> 
> Done.

Mmmm, seems you missed that one?

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.cc:391: return
base::WrapUnique(
On 2016/05/02 18:33:28, ncarter wrote:
> On 2016/04/28 00:03:13, afakhry wrote:
> > Are we not allowed to use std::make_unique()?
> 
> make_unique is not in C++11, it's C++14. Also, WrapUnique and make_unique
> operate very differently. WrapUnique takes a T*, whereas make_unique takes a
set
> of args for a T ctor, and calls "operator new" for you.

Yes, I know, but thanks for the explanation anyways! :)
I was just curious. Should have just checked http://chromium-cpp.appspot.com/.

https://codereview.chromium.org/1922683003/diff/160001/chrome/browser/ui/task...
File chrome/browser/ui/task_manager/task_manager_table_model.cc (right):

https://codereview.chromium.org/1922683003/diff/160001/chrome/browser/ui/task...
chrome/browser/ui/task_manager/task_manager_table_model.cc:38: // practice this
will be model driving the visible task manager dialog.
... will be 'the' model driving ...

[ncarter (slow), 2016-05-02 23:09:57]

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
File chrome/browser/task_manager/task_manager_browsertest_util.cc (right):

https://codereview.chromium.org/1922683003/diff/140001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.cc:274: model_.reset(
On 2016/05/02 18:58:17, afakhry wrote:
> On 2016/05/02 18:33:28, ncarter wrote:
> > On 2016/04/28 00:03:13, afakhry wrote:
> > > There are too many things that are called |model_| in this file! :D
> > > Can we call this |task_manager_tester_| instead?
> > 
> > Done.
> 
> Mmmm, seems you missed that one?

Apparently I need to hit Ctrl+S before uploading. Thanks for catching that.
Done.

https://codereview.chromium.org/1922683003/diff/160001/chrome/browser/ui/task...
File chrome/browser/ui/task_manager/task_manager_table_model.cc (right):

https://codereview.chromium.org/1922683003/diff/160001/chrome/browser/ui/task...
chrome/browser/ui/task_manager/task_manager_table_model.cc:38: // practice this
will be model driving the visible task manager dialog.
On 2016/05/02 18:58:17, afakhry wrote:
> ... will be 'the' model driving ...

Done.

[ncarter (slow), 2016-05-02 23:12:15]

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

[ncarter (slow), 2016-05-02 23:12:16]

+sky

[sky, 2016-05-02 23:58:18]

What files do you need me to review?

[ncarter (slow), 2016-05-03 00:23:56]

On 2016/05/02 23:58:18, sky wrote:
> What files do you need me to review?

Sorry, I need chrome/ OWNERS approval for the non-chrome/task_manager parts.

Please also look at task_manager_table_model.h. An alternative to the hook we're
adding there would be to add a DEPS exclusion to allow
task_manager_browsertest_util.cc to #include
chrome/browser/ui/views/new_task_manager_view.h, which is forbidden because of
ui/views rules. I'm curious what your preference is.

[sky, 2016-05-03 13:28:19]

https://codereview.chromium.org/1922683003/diff/180001/chrome/browser/task_ma...
File chrome/browser/task_manager/task_manager_browsertest_util.cc (right):

https://codereview.chromium.org/1922683003/diff/180001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.cc:169:
TaskManagerTableModel::GetInstanceForTesting();
If you need access to members of the created view why don't you make
ShowTaskManager() return a cross platform object that you can ask for the model?

https://codereview.chromium.org/1922683003/diff/180001/chrome/browser/task_ma...
File chrome/browser/task_manager/task_manager_browsertest_util.h (right):

https://codereview.chromium.org/1922683003/diff/180001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.h:19: // into the
task_management namespace.
Namespace should be based on directory, so this should be task_manager, not
task_management. This comment applies to task_manager_browsertest_util.cc as
well, where you actually use the namespace.

[ncarter (slow), 2016-05-04 21:53:41]

Thanks for the ideas.

https://codereview.chromium.org/1922683003/diff/180001/chrome/browser/task_ma...
File chrome/browser/task_manager/task_manager_browsertest_util.cc (right):

https://codereview.chromium.org/1922683003/diff/180001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.cc:169:
TaskManagerTableModel::GetInstanceForTesting();
On 2016/05/03 13:28:19, sky wrote:
> If you need access to members of the created view why don't you make
> ShowTaskManager() return a cross platform object that you can ask for the
model?

Done. This was a great idea that I hadn't considered. I've made
chrome::ShowTaskManager() return a ui::TableModel, which we can safely downcast
here. I've reverted the other testing hooks in TaskManagerTableModel.

https://codereview.chromium.org/1922683003/diff/180001/chrome/browser/task_ma...
File chrome/browser/task_manager/task_manager_browsertest_util.h (right):

https://codereview.chromium.org/1922683003/diff/180001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager_browsertest_util.h:19: // into the
task_management namespace.
On 2016/05/03 13:28:19, sky wrote:
> Namespace should be based on directory, so this should be task_manager, not
> task_management. This comment applies to task_manager_browsertest_util.cc as
> well, where you actually use the namespace.

Done. Note that in a follow-up CL I am planning on moving most of this file to
the task_management directory and namespace.

[sky, 2016-05-04 23:41:01]

LGTM

[ncarter (slow), 2016-05-05 21:56:15]

ahmed: please take a final look

[afakhry, 2016-05-05 23:40:46]

On 2016/05/05 21:56:15, ncarter wrote:
> ahmed: please take a final look

still lgtm.

[ncarter (slow), 2016-05-06 16:16:18]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2016-05-06 16:16:19]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1922683003/#ps280001
(title: "Fix OSX function call namespace")

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

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

[commit-bot: I haz the power, 2016-05-06 16:20:37]

Description was changed from

==========
Make old task manager tests work against new task manager.

Re-enable some disabled tests.

BUG=606963, 444945, 528282
==========

to

==========
Make old task manager tests work against new task manager.

Re-enable some disabled tests.

BUG=606963, 444945, 528282
==========

[commit-bot: I haz the power, 2016-05-06 16:20:38]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-05-06 16:22:20]

Description was changed from

==========
Make old task manager tests work against new task manager.

Re-enable some disabled tests.

BUG=606963, 444945, 528282
==========

to

==========
Make old task manager tests work against new task manager.

Re-enable some disabled tests.

BUG=606963, 444945, 528282

Committed: https://crrev.com/399f1066416a5987a10a1bc1c3b0b7eaf40f88b5
Cr-Commit-Position: refs/heads/master@{#392071}
==========

[commit-bot: I haz the power, 2016-05-06 16:22:23]

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