Task Manager: overhaul & re-enable task manager browser tests.

Task Manager: overhaul & re-enable task manager browser tests.

Fix some task manager bugs that showed up while working on these tests.
Those bugs, in brief:
  - A couple of the resource managers didn't handle double-add properly,
    although it is possible for this to happen in practice if the task
    manager was opened after a tracked resource had been created, but
    before it was connected (for each of these bugs there is now a test that
    would fail without the fix).
  - On Windows (which has USE_ASH=1), panel resources were being double
    reported; they were tracked by both the Extension and the Panel
    resource provider.

Eliminate a .gypi exclusion that caused the the taskmanager browser
tests to not even be compiled on Aura platforms.

Rewrite almost all tests to use a pattern of waiting for a row with
a particular title to appear in the task manager model, rather than
looking at indices. Substantially, the bread and butter of these tests
are now expectations of the form:

  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(1, MatchAnyTab()));

This pattern allows these tests to be more resilent around the behavior
of the GPU process, and hopefully will keep them from barfing the
next time we introduce a new process type.

For several of the tests, I've add new variants that show the task manager
only AFTER the resources are created. This is done because
the initial enumeration and the listening for new members
are rather distinct codepaths.

BUG=348836, 331947, 31663, 163931, 166322, 66957, 84719, 84850, 93158
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255118

[ncarter (slow), 2014-03-03 22:01:18]

Hi Avi,

Please have a look. I recommend starting with task_manager_browsertest_util.h
which explains the rest of the test changes.

[Avi (use Gerrit), 2014-03-03 22:08:09]

On 2014/03/03 22:01:18, ncarter wrote:
> Hi Avi,
> 
> Please have a look. I recommend starting with task_manager_browsertest_util.h
> which explains the rest of the test changes.

Let me take a look.

For a change of this magnitude (or at least importance), can you file a bug to
attach this to? If there is any follow-up, there should be a place to have the
discussion or have it all linked together.

[ncarter (slow), 2014-03-03 22:10:59]

On 2014/03/03 22:08:09, Avi wrote:
> On 2014/03/03 22:01:18, ncarter wrote:
> > Hi Avi,
> > 
> > Please have a look. I recommend starting with
task_manager_browsertest_util.h
> > which explains the rest of the test changes.
> 
> Let me take a look.
> 
> For a change of this magnitude (or at least importance), can you file a bug to
> attach this to? If there is any follow-up, there should be a place to have the
> discussion or have it all linked together.

Of course; I'm going through a few of the old bugs linked in the code now, but
I'll file a top-level bug too.

[Avi (use Gerrit), 2014-03-03 22:28:33]

On 2014/03/03 22:10:59, ncarter wrote:
> On 2014/03/03 22:08:09, Avi wrote:
> > On 2014/03/03 22:01:18, ncarter wrote:
> > > Hi Avi,
> > > 
> > > Please have a look. I recommend starting with
> task_manager_browsertest_util.h
> > > which explains the rest of the test changes.
> > 
> > Let me take a look.
> > 
> > For a change of this magnitude (or at least importance), can you file a bug
to
> > attach this to? If there is any follow-up, there should be a place to have
the
> > discussion or have it all linked together.
> 
> Of course; I'm going through a few of the old bugs linked in the code now, but
> I'll file a top-level bug too.

Thanks.

You use the idiom "ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(...))" a lot.
Is there a way to roll that into one call, so it would be one function that no
one could screw up?

[ncarter (slow), 2014-03-03 22:42:47]

I could put it in a macro, but not a function. ASSERT_NO_FATAL_FAILURE does
two things, (a) to stop the execution of the test after the first error,
and (b) to give an error message that has an appropriate the source line.
If you move it into the helper function you get neither of these behaviors.


On Mon, Mar 3, 2014 at 2:28 PM, <avi@chromium.org> wrote:

> On 2014/03/03 22:10:59, ncarter wrote:
>
>> On 2014/03/03 22:08:09, Avi wrote:
>> > On 2014/03/03 22:01:18, ncarter wrote:
>> > > Hi Avi,
>> > >
>> > > Please have a look. I recommend starting with
>> task_manager_browsertest_util.h
>> > > which explains the rest of the test changes.
>> >
>> > Let me take a look.
>> >
>> > For a change of this magnitude (or at least importance), can you file a
>> bug
>>
> to
>
>> > attach this to? If there is any follow-up, there should be a place to
>> have
>>
> the
>
>> > discussion or have it all linked together.
>>
>
>  Of course; I'm going through a few of the old bugs linked in the code
>> now, but
>> I'll file a top-level bug too.
>>
>
> Thanks.
>
> You use the idiom "ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(...))"
> a lot.
> Is there a way to roll that into one call, so it would be one function
> that no
> one could screw up?
>
> https://codereview.chromium.org/185873003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ncarter (slow), 2014-03-03 22:44:40]

Oh, hmm .. I could make these ASSERT_TRUE's if I had the
WaitForTaskManagerRows return a bool (and throw in WARN_UNUSED_RESULT). If
you prefer that, I can easily make the mechanical change.


On Mon, Mar 3, 2014 at 2:42 PM, Nick Carter <nick@chromium.org> wrote:

> I could put it in a macro, but not a function. ASSERT_NO_FATAL_FAILURE
> does two things, (a) to stop the execution of the test after the first
> error, and (b) to give an error message that has an appropriate the source
> line. If you move it into the helper function you get neither of these
> behaviors.
>
>
> On Mon, Mar 3, 2014 at 2:28 PM, <avi@chromium.org> wrote:
>
>> On 2014/03/03 22:10:59, ncarter wrote:
>>
>>> On 2014/03/03 22:08:09, Avi wrote:
>>> > On 2014/03/03 22:01:18, ncarter wrote:
>>> > > Hi Avi,
>>> > >
>>> > > Please have a look. I recommend starting with
>>> task_manager_browsertest_util.h
>>> > > which explains the rest of the test changes.
>>> >
>>> > Let me take a look.
>>> >
>>> > For a change of this magnitude (or at least importance), can you file
>>> a bug
>>>
>> to
>>
>>> > attach this to? If there is any follow-up, there should be a place to
>>> have
>>>
>> the
>>
>>> > discussion or have it all linked together.
>>>
>>
>>  Of course; I'm going through a few of the old bugs linked in the code
>>> now, but
>>> I'll file a top-level bug too.
>>>
>>
>> Thanks.
>>
>> You use the idiom "ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(...))"
>> a lot.
>> Is there a way to roll that into one call, so it would be one function
>> that no
>> one could screw up?
>>
>> https://codereview.chromium.org/185873003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Avi (use Gerrit), 2014-03-03 22:52:31]

I like this a lot. The one main issue I have is with the naming of two tests,
which I don't understand.

https://codereview.chromium.org/185873003/diff/40001/chrome/browser/task_mana...
File chrome/browser/task_manager/extension_process_resource_provider.cc (right):

https://codereview.chromium.org/185873003/diff/40001/chrome/browser/task_mana...
chrome/browser/task_manager/extension_process_resource_provider.cc:303: // Don't
add WebContents (those are handled by TabContentsResourceProvider) or
s/WebContents/tab contents/

Everything is a WebContents :) This comment was accidentally broken with a
rename from "TabContents" to "WebContents".

https://codereview.chromium.org/185873003/diff/40001/chrome/browser/task_mana...
File chrome/browser/task_manager/task_manager_browsertest.cc (right):

https://codereview.chromium.org/185873003/diff/40001/chrome/browser/task_mana...
chrome/browser/task_manager/task_manager_browsertest.cc:258:
IN_PROC_BROWSER_TEST_F(TaskManagerBrowserTest, KillPanelExtension) {
The sole difference between this test and KillPanelPanel, below, is the ordering
of the extension load and the showing of the task manager, so the existing names
make little sense. Can you name them appropriately?

We have "Notice[Panel|TabContents]" and "Notice[Panel|TabContents]Changes"
above; perhaps "KillPanel" and "KillChangedPanel"?

Or change everyone: "NoticeExisting*" and "NoticeChanged*" with "KillExisting*"
and "KillChanged*"

https://codereview.chromium.org/185873003/diff/40001/chrome/browser/task_mana...
chrome/browser/task_manager/task_manager_browsertest.cc:307:
IN_PROC_BROWSER_TEST_F(TaskManagerBrowserTest, KillPanelPanel) {
See above comment.

[Avi (use Gerrit), 2014-03-03 22:58:12]

On 2014/03/03 22:44:40, ncarter wrote:
> Oh, hmm .. I could make these ASSERT_TRUE's if I had the
> WaitForTaskManagerRows return a bool (and throw in WARN_UNUSED_RESULT). If
> you prefer that, I can easily make the mechanical change.

The difference between ASSERT_NO_FATAL_FAILURE and ASSERT_TRUE isn't
significant. You raise good points, so leave those alone. I withdraw my
objections there.

[ncarter (slow), 2014-03-03 23:01:43]

https://codereview.chromium.org/185873003/diff/40001/chrome/browser/task_mana...
File chrome/browser/task_manager/task_manager_browsertest.cc (right):

https://codereview.chromium.org/185873003/diff/40001/chrome/browser/task_mana...
chrome/browser/task_manager/task_manager_browsertest.cc:258:
IN_PROC_BROWSER_TEST_F(TaskManagerBrowserTest, KillPanelExtension) {
On 2014/03/03 22:52:31, Avi wrote:
> The sole difference between this test and KillPanelPanel, below, is the
ordering
> of the extension load and the showing of the task manager, so the existing
names
> make little sense. Can you name them appropriately?
> 
> We have "Notice[Panel|TabContents]" and "Notice[Panel|TabContents]Changes"
> above; perhaps "KillPanel" and "KillChangedPanel"?
> 
> Or change everyone: "NoticeExisting*" and "NoticeChanged*" with
"KillExisting*"
> and "KillChanged*"

First, bless you for actually noticing this. But actually there's one other
small difference between the two tests, which is that we call KillProcess on the
panel resource instead of the extension resource. They're expected to kill the
same process.

But I agree, we should stick with the dominant naming scheme here. I like your
suggestion about using "Changed" vs. "Existing" throughout and will adopt it. I
might need to drop the "Notice" from the names to keep things on one line.

[Avi (use Gerrit), 2014-03-03 23:05:10]

On 2014/03/03 23:01:43, ncarter wrote:
> But actually there's one other
> small difference between the two tests, which is that we call KillProcess on
the
> panel resource instead of the extension resource. They're expected to kill the
> same process.

Can you call that out in a comment or something? I'm in Colossal Cave Adventure:

"You are in a maze of twisty little unit tests, all alike"

[ncarter (slow), 2014-03-04 01:23:11]

All done, hopefully.

https://codereview.chromium.org/185873003/diff/40001/chrome/browser/task_mana...
File chrome/browser/task_manager/extension_process_resource_provider.cc (right):

https://codereview.chromium.org/185873003/diff/40001/chrome/browser/task_mana...
chrome/browser/task_manager/extension_process_resource_provider.cc:303: // Don't
add WebContents (those are handled by TabContentsResourceProvider) or
On 2014/03/03 22:52:31, Avi wrote:
> s/WebContents/tab contents/
> 
> Everything is a WebContents :) This comment was accidentally broken with a
> rename from "TabContents" to "WebContents".

Done.

https://codereview.chromium.org/185873003/diff/40001/chrome/browser/task_mana...
File chrome/browser/task_manager/task_manager_browsertest.cc (right):

https://codereview.chromium.org/185873003/diff/40001/chrome/browser/task_mana...
chrome/browser/task_manager/task_manager_browsertest.cc:258:
IN_PROC_BROWSER_TEST_F(TaskManagerBrowserTest, KillPanelExtension) {
On 2014/03/03 23:01:43, ncarter wrote:
> On 2014/03/03 22:52:31, Avi wrote:
> > The sole difference between this test and KillPanelPanel, below, is the
> ordering
> > of the extension load and the showing of the task manager, so the existing
> names
> > make little sense. Can you name them appropriately?
> > 
> > We have "Notice[Panel|TabContents]" and "Notice[Panel|TabContents]Changes"
> > above; perhaps "KillPanel" and "KillChangedPanel"?
> > 
> > Or change everyone: "NoticeExisting*" and "NoticeChanged*" with
> "KillExisting*"
> > and "KillChanged*"
> 
> First, bless you for actually noticing this. But actually there's one other
> small difference between the two tests, which is that we call KillProcess on
the
> panel resource instead of the extension resource. They're expected to kill the
> same process.
> 
> But I agree, we should stick with the dominant naming scheme here. I like your
> suggestion about using "Changed" vs. "Existing" throughout and will adopt it.
I
> might need to drop the "Notice" from the names to keep things on one line.

I added comments in four places, renamed the tests, and also removed a block of
unnecessary copypasta'ed code that might have been causing confusion in the
second test.

https://codereview.chromium.org/185873003/diff/40001/chrome/browser/task_mana...
chrome/browser/task_manager/task_manager_browsertest.cc:307:
IN_PROC_BROWSER_TEST_F(TaskManagerBrowserTest, KillPanelPanel) {
On 2014/03/03 22:52:31, Avi wrote:
> See above comment.

Done.

[ncarter (slow), 2014-03-04 01:30:08]

Adding cbentzel@ and yoshiki@ as OWNERS

cbentzel@chromium.org: Please review changes in
chrome/browser/prerender/prerender_browsertest.cc

yoshiki@chromium.org: Please review changes in chrome/browser/task_manager/*

[Avi (use Gerrit), 2014-03-04 02:24:33]

FWIW this LGTM

[ncarter (slow), 2014-03-05 00:05:05]

-yoshiki, -cbentzel: asking jam@ for OWNERS approval instead.

+jam : approval required.

[jam, 2014-03-05 08:52:22]

rubberstamp lgtm

[ncarter (slow), 2014-03-05 16:14:05]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2014-03-05 16:14:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nick@chromium.org/185873003/70001

[commit-bot: I haz the power, 2014-03-05 17:26:17]

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

[commit-bot: I haz the power, 2014-03-05 17:26:18]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[ncarter (slow), 2014-03-05 17:33:29]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2014-03-05 17:34:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nick@chromium.org/185873003/70001

[commit-bot: I haz the power, 2014-03-05 19:44:47]

Change committed as 255118