Fix bugs in the new task manager Impl so that ported old browsertests pass

Fix bugs in the new task manager Impl so that ported old browsertests pass

There were a couple of minor bugs related to the classification of tasks in
their titles.

BUG=606963
TEST=browser_tests --gtest_filter=TaskManagerBrowserTest.Notice*

Committed: https://crrev.com/fab47c7a6d0d53da697567699923a4701ef45d00
Cr-Commit-Position: refs/heads/master@{#394596}

[afakhry, 2016-05-18 21:53:28]

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

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

Hey Nick, could you please review this CL?

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/providers/web_contents/devtools_task.h
(right):

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/devtools_task.h:14: class
DevToolsTask : public TabContentsTask {
In the old TM, there wasn't a separate DevToolsResource. It was handled the same
way as TabContentsResources.

I left a separate DevToolsTask here just in case we want to do anything specific
with it in the future.

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/providers/web_contents/tab_contents_task.cc
(right):

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/tab_contents_task.cc:62:
bool is_app = process_map->Contains(GetChildProcessUniqueID()) &&
This is where I thank you for porting those old tests! :D There was no way of
finding this subtle bug without them!

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/task_manager_browsertest.cc (right):

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/task_manager_browsertest.cc:691:
ui_test_utils::NavigateToURL(browser(), url);
The new task manager doesn't recalculate the titles repeatedly like the old one,
so the task classification in the title won't change unless we force a reload.
Is that OK?

[ncarter (slow), 2016-05-18 22:21:02]

lgtm+++++++++

would lgtm again

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/providers/web_contents/devtools_task.h
(right):

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/devtools_task.h:14: class
DevToolsTask : public TabContentsTask {
On 2016/05/18 21:53:29, afakhry wrote:
> In the old TM, there wasn't a separate DevToolsResource. It was handled the
same
> way as TabContentsResources.
> 
> I left a separate DevToolsTask here just in case we want to do anything
specific
> with it in the future.

Makes sense.

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/providers/web_contents/tab_contents_task.cc
(right):

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/tab_contents_task.cc:62:
bool is_app = process_map->Contains(GetChildProcessUniqueID()) &&
On 2016/05/18 21:53:29, afakhry wrote:
> This is where I thank you for porting those old tests! :D There was no way of
> finding this subtle bug without them!

Oh, wow, awesome! Glad you figured this out.

ALSO this is a great example of why we ought to have strongly typed integer ids
in base (or at least, in the content api, for child process unique ids)!

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/task_manager_browsertest.cc (right):

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/task_manager_browsertest.cc:689: // Disable
extension and relaod.
reload is misspelled.

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/task_manager_browsertest.cc:691:
ui_test_utils::NavigateToURL(browser(), url);
On 2016/05/18 21:53:29, afakhry wrote:
> The new task manager doesn't recalculate the titles repeatedly like the old
one,
> so the task classification in the title won't change unless we force a reload.
> Is that OK?

I think that's okay. I imagine the page will still actually think it's an app
until reload, anyhow.

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

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/task_manager_browsertest.cc (right):

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/task_manager_browsertest.cc:708:
ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(0, MatchAnyApp()));
Actually, I think we can remove this last section -- it looks redundant now?

[afakhry, 2016-05-18 22:32:49]

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/task_manager_browsertest.cc (right):

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/task_manager_browsertest.cc:689: // Disable
extension and relaod.
On 2016/05/18 22:21:01, ncarter wrote:
> reload is misspelled.

Done.

https://codereview.chromium.org/1989393003/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/task_manager_browsertest.cc:708:
ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(0, MatchAnyApp()));
On 2016/05/18 22:22:33, ncarter wrote:
> Actually, I think we can remove this last section -- it looks redundant now?

Yes, it it. Done.

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

The CQ bit was checked by afakhry@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1989393003/#ps20001
(title: "Nicks comments")

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

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

[commit-bot: I haz the power, 2016-05-18 23:28:16]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-18 23:30:53]

Description was changed from

==========
Fix bugs in the new task manager Impl so that ported old browsertests pass

There were a couple of minor bugs related to the classification of tasks in
their titles.

BUG=606963
TEST=browser_tests --gtest_filter=TaskManagerBrowserTest.Notice*
==========

to

==========
Fix bugs in the new task manager Impl so that ported old browsertests pass

There were a couple of minor bugs related to the classification of tasks in
their titles.

BUG=606963
TEST=browser_tests --gtest_filter=TaskManagerBrowserTest.Notice*

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

[commit-bot: I haz the power, 2016-05-18 23:30:54]

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