[Part 4 of 6] Split LegacyTaskManagerTester into its own file.

Split LegacyTaskManagerTester into its own file.

[splitting apart task_manager_browsertest_util, part 4 of 6]

BUG=606963
Committed: https://crrev.com/df5aeb4ed8d6475bfd13a8b158c30cb6afc6f595
Cr-Commit-Position: refs/heads/master@{#392659}

[ncarter (slow), 2016-05-06 21:10:49]

Description was changed from

==========
Split LegacyTaskManagerTester into its own file.

[splitting apart task_manager_browsertest_util, part 3]

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

to

==========
Split LegacyTaskManagerTester into its own file.

[splitting apart task_manager_browsertest_util, part 4]

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

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

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

[ncarter (slow), 2016-05-06 21:50:57]

Ahmed, please review (this is part 4)

https://codereview.chromium.org/1955123002/diff/20001/chrome/browser/task_man...
File chrome/browser/task_manager/legacy_task_manager_tester.h (left):

https://codereview.chromium.org/1955123002/diff/20001/chrome/browser/task_man...
chrome/browser/task_manager/legacy_task_manager_tester.h:6: #define
MEDIA_MOJO_SERVICES_MOJO_MEDIA_APPLICATION_FACTORY_H_
Ignore this LHS.

[ncarter (slow), 2016-05-06 22:24:31]

Description was changed from

==========
Split LegacyTaskManagerTester into its own file.

[splitting apart task_manager_browsertest_util, part 4]

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

to

==========
Split LegacyTaskManagerTester into its own file.

[splitting apart task_manager_browsertest_util, part 4 of 6]

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

[afakhry, 2016-05-07 00:16:09]

https://codereview.chromium.org/1955123002/diff/20001/chrome/browser/task_man...
File chrome/browser/task_manager/legacy_task_manager_tester.h (right):

https://codereview.chromium.org/1955123002/diff/20001/chrome/browser/task_man...
chrome/browser/task_manager/legacy_task_manager_tester.h:16: class
LegacyTaskManagerTester : public browsertest_util::TaskManagerTester {
mmm, what's the benefit of an extra level here? i.e. why did you split it into
LegacyTaskManagerTester and LegacyTaskManagerTesterImpl?

Is it a matter of just hiding the Impl?

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

https://codereview.chromium.org/1955123002/diff/20001/chrome/browser/task_man...
chrome/browser/task_manager/task_manager_browsertest_util.cc:327: return
LegacyTaskManagerTester::Create(base::Closure());
I just noticed this. There's a code duplication between here and
ResourceChangeObserver() ctor.

Can we dedup and make GetTaskManagerTester() take a closure, and use in the
ResourceChangeObserver() ctor to set the |task_manager_tester_|?

Who else uses GetTaskManagerTester()?

[ncarter (slow), 2016-05-07 22:27:10]

https://codereview.chromium.org/1955123002/diff/20001/chrome/browser/task_man...
File chrome/browser/task_manager/legacy_task_manager_tester.h (right):

https://codereview.chromium.org/1955123002/diff/20001/chrome/browser/task_man...
chrome/browser/task_manager/legacy_task_manager_tester.h:16: class
LegacyTaskManagerTester : public browsertest_util::TaskManagerTester {
On 2016/05/07 00:16:09, afakhry wrote:
> mmm, what's the benefit of an extra level here? i.e. why did you split it into
> LegacyTaskManagerTester and LegacyTaskManagerTesterImpl?
> 
> Is it a matter of just hiding the Impl?

It's about hiding the impl and preventing anybody from using the class directly,
while still having a logical home for the factory function. It also results in
fewer lines of code than declaring all the methods in the header.

[ncarter (slow), 2016-05-07 22:29:08]

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

https://codereview.chromium.org/1955123002/diff/20001/chrome/browser/task_man...
chrome/browser/task_manager/task_manager_browsertest_util.cc:327: return
LegacyTaskManagerTester::Create(base::Closure());
On 2016/05/07 00:16:09, afakhry wrote:
> I just noticed this. There's a code duplication between here and
> ResourceChangeObserver() ctor.
> 
> Can we dedup and make GetTaskManagerTester() take a closure, and use in the
> ResourceChangeObserver() ctor to set the |task_manager_tester_|?
> 
> Who else uses GetTaskManagerTester()?

Your suggestion is implemented happens in PS5. And yes, originally it seemed
like it made sense to hide the callback, but there's only one caller, so it's no
big deal in practice.

[afakhry, 2016-05-09 17:46:45]

https://codereview.chromium.org/1955123002/diff/20001/chrome/browser/task_man...
File chrome/browser/task_manager/legacy_task_manager_tester.h (right):

https://codereview.chromium.org/1955123002/diff/20001/chrome/browser/task_man...
chrome/browser/task_manager/legacy_task_manager_tester.h:16: class
LegacyTaskManagerTester : public browsertest_util::TaskManagerTester {
On 2016/05/07 22:27:10, ncarter wrote:
> On 2016/05/07 00:16:09, afakhry wrote:
> > mmm, what's the benefit of an extra level here? i.e. why did you split it
into
> > LegacyTaskManagerTester and LegacyTaskManagerTesterImpl?
> > 
> > Is it a matter of just hiding the Impl?
> 
> It's about hiding the impl and preventing anybody from using the class
directly,
> while still having a logical home for the factory function. It also results in
> fewer lines of code than declaring all the methods in the header.

I honestly think it's an overkill. The object can still be used through
(TaskManagerTester*), and created through the factory method.

[ncarter (slow), 2016-05-09 20:10:08]

On 2016/05/09 17:46:45, afakhry wrote:
>
https://codereview.chromium.org/1955123002/diff/20001/chrome/browser/task_man...
> File chrome/browser/task_manager/legacy_task_manager_tester.h (right):
> 
>
https://codereview.chromium.org/1955123002/diff/20001/chrome/browser/task_man...
> chrome/browser/task_manager/legacy_task_manager_tester.h:16: class
> LegacyTaskManagerTester : public browsertest_util::TaskManagerTester {
> On 2016/05/07 22:27:10, ncarter wrote:
> > On 2016/05/07 00:16:09, afakhry wrote:
> > > mmm, what's the benefit of an extra level here? i.e. why did you split it
> into
> > > LegacyTaskManagerTester and LegacyTaskManagerTesterImpl?
> > > 
> > > Is it a matter of just hiding the Impl?
> > 
> > It's about hiding the impl and preventing anybody from using the class
> directly,
> > while still having a logical home for the factory function. It also results
in
> > fewer lines of code than declaring all the methods in the header.
> 
> I honestly think it's an overkill. The object can still be used through
> (TaskManagerTester*), and created through the factory method.

Done.

[afakhry, 2016-05-09 20:17:53]

lgtm

[ncarter (slow), 2016-05-10 18:26:38]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2016-05-10 18:27:03]

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

[commit-bot: I haz the power, 2016-05-10 18:32:50]

Description was changed from

==========
Split LegacyTaskManagerTester into its own file.

[splitting apart task_manager_browsertest_util, part 4 of 6]

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

to

==========
Split LegacyTaskManagerTester into its own file.

[splitting apart task_manager_browsertest_util, part 4 of 6]

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

[commit-bot: I haz the power, 2016-05-10 18:32:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-10 18:35:41]

Description was changed from

==========
Split LegacyTaskManagerTester into its own file.

[splitting apart task_manager_browsertest_util, part 4 of 6]

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

to

==========
Split LegacyTaskManagerTester into its own file.

[splitting apart task_manager_browsertest_util, part 4 of 6]

BUG=606963

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

[commit-bot: I haz the power, 2016-05-10 18:35:43]

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