Task Manager: Make double-clicking on an extension do something useful.

Task Manager: Make double-clicking on an extension do something useful.

Extension background pages, etc, don't implement anything for
WebContentsDelegate::ActivateContents; as a result, double-clicking
on them does nothing. This change makes it so that we open
the chrome-extensions:// page instead, highlighting the details
for the particular extension.

Make double-clicking on a subframe row have the same behavior as
double-clicking on the main row for the tab.

BUG=614529
Committed: https://crrev.com/e578802016df8e28e20aab78561446f2697fab68
Cr-Commit-Position: refs/heads/master@{#395990}

[ncarter (slow), 2016-05-25 00:32:56]

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

[ncarter (slow), 2016-05-25 00:32:56]

Ahmed, Bruce: PTAL

[ncarter (slow), 2016-05-25 00:33:12]

Description was changed from

==========
Task Manager: Make double-clicking on an extension do something useful.

Extension background pages, etc, don't have implement anything for
WebContentsDelegate::ActivateContents; as a result, double-clicking
on them does nothing. This change makes it so that we open
the chrome-extensions:// page instead, highlighting the details
for the particular extension.

Make double-clicking on a subframe row have the same behavior as
double-clicking on the main row for the tab.

BUG=614529
==========

to

==========
Task Manager: Make double-clicking on an extension do something useful.

Extension background pages, etc, don't mplement anything for
WebContentsDelegate::ActivateContents; as a result, double-clicking
on them does nothing. This change makes it so that we open
the chrome-extensions:// page instead, highlighting the details
for the particular extension.

Make double-clicking on a subframe row have the same behavior as
double-clicking on the main row for the tab.

BUG=614529
==========

[brucedawson, 2016-05-25 00:50:23]

Description was changed from

==========
Task Manager: Make double-clicking on an extension do something useful.

Extension background pages, etc, don't mplement anything for
WebContentsDelegate::ActivateContents; as a result, double-clicking
on them does nothing. This change makes it so that we open
the chrome-extensions:// page instead, highlighting the details
for the particular extension.

Make double-clicking on a subframe row have the same behavior as
double-clicking on the main row for the tab.

BUG=614529
==========

to

==========
Task Manager: Make double-clicking on an extension do something useful.

Extension background pages, etc, don't implement anything for
WebContentsDelegate::ActivateContents; as a result, double-clicking
on them does nothing. This change makes it so that we open
the chrome-extensions:// page instead, highlighting the details
for the particular extension.

Make double-clicking on a subframe row have the same behavior as
double-clicking on the main row for the tab.

BUG=614529
==========

[brucedawson, 2016-05-25 00:59:12]

On 2016/05/25 00:32:56, ncarter wrote:
> Ahmed, Bruce: PTAL

For maximum consistency it seems that clicking on an extension's *main* process
in the task manager should do the same thing. Does it make sense to make such a
change as part of this, or as a separate discussion/CL?

[afakhry, 2016-05-25 18:50:59]

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

https://codereview.chromium.org/2006423002/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/extension_task.cc:49:
extension_id_(extension->id()) {
I applied this locally and flashed it on a chromeos device. This causes a crash
on startup. |extension| is nullptr.

That's why in GetExtensionTitle() and LoadExtensionIcon() we always check for
|extension| not being null. You need to account for this.

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

On 2016/05/25 00:59:12, brucedawson wrote:
> On 2016/05/25 00:32:56, ncarter wrote:
> > Ahmed, Bruce: PTAL
> 
> For maximum consistency it seems that clicking on an extension's *main*
process
> in the task manager should do the same thing. Does it make sense to make such
a
> change as part of this, or as a separate discussion/CL?

Good news, that's actually what's implemented here. Double-clicking a subframe
row does the same thing as double-clicking its main frame. A breakdown of some
of the cases:

1. If you double click on the row for an extension background page (which is
what we usually think of as the 'main' extension process): this opens the
chrome://extensions page.

2. If a normal http/https webpage embeds <iframe
src="chrome-extension://fhwdgads">, and oopifs are enabled, then double-clicking
on the "Subframe: " row will focus the tab containing the iframe, and the
chrome://extensions is not loaded. If this extension also happens to have a
background or event page (which is almost guaranteed in this case, given how
extensions work), that will be a second row in the task manager in the same
process as the oopif; the two rows will be grouped together, and double-clicking
the background page will have behavior #1.

3. If you load an extension URL in a normal tag (e.g., open the extension's
options.html page), it renders in the extension process, but double-clicking it
will activate the options.html tab.

4. If an extension background page contains an http oopif, double-clicking the
oopif row will open chrome://extensions, just as if you'd double-clicked the row
for the background page.

Still not perfect are the behaviors for e.g. app windows or dialogs that are
free-floating and which could be focused.

[ncarter (slow), 2016-05-25 19:59:09]

please have another look

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

https://codereview.chromium.org/2006423002/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/providers/web_contents/extension_task.cc:49:
extension_id_(extension->id()) {
On 2016/05/25 18:50:59, afakhry wrote:
> I applied this locally and flashed it on a chromeos device. This causes a
crash
> on startup. |extension| is nullptr.
> 
> That's why in GetExtensionTitle() and LoadExtensionIcon() we always check for
> |extension| not being null. You need to account for this.

Yuck. Thanks for figuring that out.

[afakhry, 2016-05-25 20:07:09]

lgtm with one question:

https://codereview.chromium.org/2006423002/diff/20001/chrome/browser/task_man...
File
chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc
(right):

https://codereview.chromium.org/2006423002/diff/20001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:235:
GetTaskForFrame(web_contents()->GetMainFrame()));
I hope the task of the main frame gets created before its subframe tasks. Is
that guaranteed?

[ncarter (slow), 2016-05-25 20:34:23]

https://codereview.chromium.org/2006423002/diff/20001/chrome/browser/task_man...
File
chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc
(right):

https://codereview.chromium.org/2006423002/diff/20001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:235:
GetTaskForFrame(web_contents()->GetMainFrame()));
On 2016/05/25 20:07:09, afakhry wrote:
> I hope the task of the main frame gets created before its subframe tasks. Is
> that guaranteed?

Yes; the way we use WebContentsObserver guarantees this (in general, creation
and ForEachFrame guarantee top-down, and deletion is guaranteed to be
bottom-up.) WebContentsObserverSanityChecker enforces these assumptions.

[ncarter (slow), 2016-05-25 20:34:45]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2016-05-25 20:35:35]

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

[commit-bot: I haz the power, 2016-05-25 21:29:53]

Description was changed from

==========
Task Manager: Make double-clicking on an extension do something useful.

Extension background pages, etc, don't implement anything for
WebContentsDelegate::ActivateContents; as a result, double-clicking
on them does nothing. This change makes it so that we open
the chrome-extensions:// page instead, highlighting the details
for the particular extension.

Make double-clicking on a subframe row have the same behavior as
double-clicking on the main row for the tab.

BUG=614529
==========

to

==========
Task Manager: Make double-clicking on an extension do something useful.

Extension background pages, etc, don't implement anything for
WebContentsDelegate::ActivateContents; as a result, double-clicking
on them does nothing. This change makes it so that we open
the chrome-extensions:// page instead, highlighting the details
for the particular extension.

Make double-clicking on a subframe row have the same behavior as
double-clicking on the main row for the tab.

BUG=614529
==========

[commit-bot: I haz the power, 2016-05-25 21:29:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-25 21:31:38]

Description was changed from

==========
Task Manager: Make double-clicking on an extension do something useful.

Extension background pages, etc, don't implement anything for
WebContentsDelegate::ActivateContents; as a result, double-clicking
on them does nothing. This change makes it so that we open
the chrome-extensions:// page instead, highlighting the details
for the particular extension.

Make double-clicking on a subframe row have the same behavior as
double-clicking on the main row for the tab.

BUG=614529
==========

to

==========
Task Manager: Make double-clicking on an extension do something useful.

Extension background pages, etc, don't implement anything for
WebContentsDelegate::ActivateContents; as a result, double-clicking
on them does nothing. This change makes it so that we open
the chrome-extensions:// page instead, highlighting the details
for the particular extension.

Make double-clicking on a subframe row have the same behavior as
double-clicking on the main row for the tab.

BUG=614529

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

[commit-bot: I haz the power, 2016-05-25 21:31:39]

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