Show extension tabs in task manager and correctly label apps.

Show extension tabs in task manager and correctly label apps.

BUG=31526
BUG=60840
TEST=TaskManagerBrowserTest.NoticeExtensionTabs
TEST=TaskManagerBrowserTest.NoticeAppTabs

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68324

[Erik does not do reviews, 2010-12-02 01:05:17]

LGTM

http://codereview.chromium.org/5526002/diff/1/chrome/browser/tab_contents/tab...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5526002/diff/1/chrome/browser/tab_contents/tab...
chrome/browser/tab_contents/tab_contents.cc:598:
extension_service->GetExtensionById(GetURL().host(), false);
use GetExtensionByURL instead (no need to use .host() then and it also checks
that the scheme is correct)

http://codereview.chromium.org/5526002/diff/1/chrome/browser/task_manager/tas...
File chrome/browser/task_manager/task_manager_browsertest.cc (right):

http://codereview.chromium.org/5526002/diff/1/chrome/browser/task_manager/tas...
chrome/browser/task_manager/task_manager_browsertest.cc:185:
WaitForResourceChange(4);
should we test that this is the right type in the task manager as well?

[Charlie Reis, 2010-12-02 01:45:43]

http://codereview.chromium.org/5526002/diff/1/chrome/browser/tab_contents/tab...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5526002/diff/1/chrome/browser/tab_contents/tab...
chrome/browser/tab_contents/tab_contents.cc:598:
extension_service->GetExtensionById(GetURL().host(), false);
On 2010/12/02 01:05:17, Erik Kay wrote:
> use GetExtensionByURL instead (no need to use .host() then and it also checks
> that the scheme is correct)

Ah, nice.  I should have noticed that one.

http://codereview.chromium.org/5526002/diff/1/chrome/browser/task_manager/tas...
File chrome/browser/task_manager/task_manager_browsertest.cc (right):

http://codereview.chromium.org/5526002/diff/1/chrome/browser/task_manager/tas...
chrome/browser/task_manager/task_manager_browsertest.cc:185:
WaitForResourceChange(4);
On 2010/12/02 01:05:17, Erik Kay wrote:
> should we test that this is the right type in the task manager as well?

Good idea.  The last one is actually a TabContentsResource and not an
ExtensionResource, so it's worth showing that in the test.

I also found a way to test the title prefix.  That means I could test the app
prefix as well-- do you know if there are any packaged apps in the test
directory I could install, or will I need to write a new one?

[Charlie Reis, 2010-12-02 01:52:43]

On 2010/12/02 01:45:43, creis wrote:
> I also found a way to test the title prefix.  That means I could test the app
> prefix as well-- do you know if there are any packaged apps in the test
> directory I could install, or will I need to write a new one?

Ah, I might also have to know the chrome-extension:// URL for it to be able to
open the packaged app's launch page, unless there's easier way to launch a
packaged app from a browser test.

[Charlie Reis, 2010-12-02 02:14:23]

Here's a version with an app tab test, though see the comment inline.

Thanks,
Charlie

http://codereview.chromium.org/5526002/diff/15002/chrome/browser/task_manager...
File chrome/browser/task_manager/task_manager_browsertest.cc (right):

http://codereview.chromium.org/5526002/diff/15002/chrome/browser/task_manager...
chrome/browser/task_manager/task_manager_browsertest.cc:219: GURL
url("chrome-extension://pffmiogcfmficciligcchmiebhahbanm/main.html");
I'm guessing this is fragile and won't have the same ID on other machines.  Is
there an easy way for me to open the app's tab or hard code this ID into the
extension (as in the test above)?

[Charlie Reis, 2010-12-02 21:21:00]

Here's an improved CL.

The previous one was failing the ExtensionApiTest.Processes test because it was
getting a "RENDERER" type for extension tabs.  I realized we should probably
change the type of a TabContentsResource to "EXTENSION" if it is rendering an
extension, and we should make GetResourceExtension return the extension.  The
test now reflects this, and we pass the Processes test.

I also fixed the NoticeAppTabs test to use last_loaded_extension_id_ rather than
hardcoding an arbitrary extension ID.

Finally, I noticed that NoticeExtensionTabs was failing on Windows (similar to
issue 31663), but I think I've found a fix: unload the extension before the test
finishes.  (That explains why the KillExtension test didn't have trouble on
Windows.)  I'll re-enable the NoticeExtensionChanges test in a separate CL,
assuming this goes well on the bots.

Can you take another look?

[Erik does not do reviews, 2010-12-03 17:04:56]

Overall, LGTM, but I'd like to understand the crash you're working aroudn a bit
more.

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:589: bool
TabContents::IsInstalledApp() const {
We have this double check logic scattered around a few places.  Could you just
put this method in ExtensionsService?

ExtensionsService::IsInstalledApp(const GURL& url)

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/task_manager...
File chrome/browser/task_manager/task_manager_browsertest.cc (right):

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/task_manager...
chrome/browser/task_manager/task_manager_browsertest.cc:178: // Show the task
manager. This populates the model, and helps with debugging
Does the model get populated synchronously with this call?  If not, I'd be
concerned of potential test flakiness on the waterfall.

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/task_manager...
chrome/browser/task_manager/task_manager_browsertest.cc:211: // Unload extension
to avoid crash on Windows
what's the crash?  I don't think we do this in other tests, so if it's
important, it seems like we should understand why.

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/task_manager...
File chrome/browser/task_manager/task_manager_resource_providers.cc (right):

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/task_manager...
chrome/browser/task_manager/task_manager_resource_providers.cc:153: if
(tab_contents_->HostsExtension())
in cases like this I usually prefer:
return tab_contents_->HostsExtension() ? EXTENSION : RENDERER;

but I don't care too much :-)

[Charlie Reis, 2010-12-03 23:02:15]

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:589: bool
TabContents::IsInstalledApp() const {
On 2010/12/03 17:04:56, Erik Kay wrote:
> We have this double check logic scattered around a few places.  Could you just
> put this method in ExtensionsService?
> 
> ExtensionsService::IsInstalledApp(const GURL& url)

Done.  No more changes to tab_contents needed-- we can remove the deprecated
code another time.

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/task_manager...
File chrome/browser/task_manager/task_manager_browsertest.cc (right):

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/task_manager...
chrome/browser/task_manager/task_manager_browsertest.cc:178: // Show the task
manager. This populates the model, and helps with debugging
On 2010/12/03 17:04:56, Erik Kay wrote:
> Does the model get populated synchronously with this call?  If not, I'd be
> concerned of potential test flakiness on the waterfall.

Actually, I think that's a good catch.  It looks like the task manager starts
updating asynchronously, and we might have just been lucky in the past with all
the other tests that do this and immediately call EXPECT_EQ.  (Or unlucky, as
the flaky test above might attest.)

I've changed the EXPECT_EQ to WaitForResourceChange so that we allow it to fully
initialize.

If that works, I'll fix the other tests in a follow-up CL (along with the crash
issue in NoticeExtensionChanges).

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/task_manager...
chrome/browser/task_manager/task_manager_browsertest.cc:211: // Unload extension
to avoid crash on Windows
On 2010/12/03 17:04:56, Erik Kay wrote:
> what's the crash?  I don't think we do this in other tests, so if it's
> important, it seems like we should understand why.

I'll look into it once my Windows build finishes.  I got the idea for the
workaround from the fact that the only difference between NoticeExtensionChanges
(disabled on Windows) and KillExtension (not disabled) is that the extension is
gone at the end.

In the mean time, you can see the crash here, under NoticeExtensionTabs:
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/5154/steps...
Looks like a NOTREACHED in GetProcId (in process_util_win.cc).

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/task_manager...
File chrome/browser/task_manager/task_manager_resource_providers.cc (right):

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/task_manager...
chrome/browser/task_manager/task_manager_resource_providers.cc:153: if
(tab_contents_->HostsExtension())
On 2010/12/03 17:04:56, Erik Kay wrote:
> in cases like this I usually prefer:
> return tab_contents_->HostsExtension() ? EXTENSION : RENDERER;
> 
> but I don't care too much :-)

Done.

[Charlie Reis, 2010-12-03 23:07:47]

On 2010/12/03 23:02:15, creis wrote:
> > what's the crash?  I don't think we do this in other tests, so if it's
> > important, it seems like we should understand why.
> 
> I'll look into it once my Windows build finishes.  I got the idea for the
> workaround from the fact that the only difference between
NoticeExtensionChanges
> (disabled on Windows) and KillExtension (not disabled) is that the extension
is
> gone at the end.
> 
> In the mean time, you can see the crash here, under NoticeExtensionTabs:
>
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/5154/steps...
> Looks like a NOTREACHED in GetProcId (in process_util_win.cc).
> 

Drat.  It doesn't crash for me on Windows 7.  I don't suppose you've got a
Windows XP dev box to test it on?  Not sure there's much I can do without seeing
the crash.

[Erik does not do reviews, 2010-12-04 00:28:11]

LGTM

unfortunately I don't have an XP machine handy.  If this only repros on XP, we
may need to test in a VM.

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/task_manager...
File chrome/browser/task_manager/task_manager_browsertest.cc (right):

http://codereview.chromium.org/5526002/diff/20001/chrome/browser/task_manager...
chrome/browser/task_manager/task_manager_browsertest.cc:211: // Unload extension
to avoid crash on Windows
On 2010/12/03 23:02:16, creis wrote:
> On 2010/12/03 17:04:56, Erik Kay wrote:
> > what's the crash?  I don't think we do this in other tests, so if it's
> > important, it seems like we should understand why.
> 
> I'll look into it once my Windows build finishes.  I got the idea for the
> workaround from the fact that the only difference between
NoticeExtensionChanges
> (disabled on Windows) and KillExtension (not disabled) is that the extension
is
> gone at the end.
> 
> In the mean time, you can see the crash here, under NoticeExtensionTabs:
>
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/5154/steps...
> Looks like a NOTREACHED in GetProcId (in process_util_win.cc).

OK.  Could you file a bug for this and reference it here?

[Charlie Reis, 2010-12-04 02:13:08]

On 2010/12/04 00:28:11, Erik Kay wrote:
> OK.  Could you file a bug for this and reference it here?

Done.  I've added the things we discovered here to http://crbug.com/31663, which
was already filed for that crash.