Bug 88473 - Web Inspector: Implement ExtensionPanel.show() method
Summary: Web Inspector: Implement ExtensionPanel.show() method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 89333
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-06 17:18 PDT by Jan Keromnes
Modified: 2012-06-22 16:10 PDT (History)
15 users (show)

See Also:


Attachments
Patch (6.26 KB, patch)
2012-06-06 18:04 PDT, Jan Keromnes
no flags Details | Formatted Diff | Diff
Patch (6.22 KB, patch)
2012-06-07 12:07 PDT, Jan Keromnes
no flags Details | Formatted Diff | Diff
Patch (6.57 KB, patch)
2012-06-07 17:04 PDT, Jan Keromnes
no flags Details | Formatted Diff | Diff
Patch (7.00 KB, patch)
2012-06-08 14:09 PDT, Jan Keromnes
no flags Details | Formatted Diff | Diff
Patch (7.01 KB, patch)
2012-06-08 14:39 PDT, Jan Keromnes
no flags Details | Formatted Diff | Diff
Patch (8.82 KB, patch)
2012-06-11 20:42 PDT, Jan Keromnes
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (786.40 KB, application/zip)
2012-06-12 01:02 PDT, WebKit Review Bot
no flags Details
Patch (8.94 KB, patch)
2012-06-12 17:19 PDT, Jan Keromnes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Keromnes 2012-06-06 17:18:26 PDT
PROBLEM:
- Create an ExtensionPanel with `panels.create()`
- Register an openResourceHandler with `panels.setOpenResourceHandler()`
- Change WebInspector settings to "Open links in [your panel]"
- When you click on a link in the Elements panel, your Panel doesn't show
- There is no way to programatically show the panel when needed

SOLUTION:
- Implement an `ExtensionPanel.show()` method
- Call it in your openResourceHandler so that, when a resource link is clicked, your panel shows
Comment 1 Jan Keromnes 2012-06-06 18:04:11 PDT
Created attachment 146167 [details]
Patch
Comment 2 Andrey Kosyakov 2012-06-07 07:22:52 PDT
Comment on attachment 146167 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146167&action=review

We're somewhat wary of the extensions switching focus unexpectedly for the user. So it would be good idea to add a check that only allows panel.show() to be called in response to a user action (I can only think of one so far -- open resource handler). So the idea is to set a flag upon invoking the handler, clearing it upon exit from the handle and checking it in show(). Unfortunately, with server-to-client callback being async, this would have to be done on the client side -- but it's still better than nothing :)

> Source/WebCore/inspector/front-end/ExtensionAPI.js:399
> +    },
> +    show: function()

please add empty line before show.

> Source/WebCore/inspector/front-end/ExtensionServer.js:210
> +        WebInspector.showPanel(message.id);

Please add a check for id to be the extension panel (i.e. id in this._clientObjects). It wouldn't be possible to pass incorrect panel id through an API call, but we're trying to also protect from direct messages sent to the extension server. Also very unlikely, but just in case :)

> LayoutTests/inspector/extensions/extensions-panel.html:74
> +        panel.onSearch.addListener(function(action, query) {
> +            output("Panel searched");
> +        });

This does not seem to have any effect on the test output, so please remove it for now.
Comment 3 Vsevolod Vlasov 2012-06-07 07:35:45 PDT
Comment on attachment 146167 [details]
Patch

r- per Andrey's comments.
Comment 4 Jan Keromnes 2012-06-07 11:52:27 PDT
(In reply to comment #2)

Thanks for your input on this Andrey!

> We're somewhat wary of the extensions switching focus unexpectedly for the user. So it would be good idea to add a check that only allows panel.show() to be called in response to a user action (I can only think of one so far -- open resource handler). So the idea is to set a flag upon invoking the handler, clearing it upon exit from the handle and checking it in show(). Unfortunately, with server-to-client callback being async, this would have to be done on the client side -- but it's still better than nothing :)

I believe unexpected focus-grabbing extensions would only harm themselves, because users would remove them and dissuade other users from installing it. But if you think it's important to prevent that, I will gladly implement a check for it.

What if the extension panel is a debugger and the runtime hits a breakpoint?

Another solution would be to make handlers (like the open resource handler) return a panel ID to bring up, but I'm not sure if it's a good idea.

> 
> > Source/WebCore/inspector/front-end/ExtensionAPI.js:399
> > +    },
> > +    show: function()
> 
> please add empty line before show.

Will do.

> > Source/WebCore/inspector/front-end/ExtensionServer.js:210
> > +        WebInspector.showPanel(message.id);
> 
> Please add a check for id to be the extension panel (i.e. id in this._clientObjects). It wouldn't be possible to pass incorrect panel id through an API call, but we're trying to also protect from direct messages sent to the extension server. Also very unlikely, but just in case :)

The method `WebInspector.showPanel()` in `inspector.js:835` is already protected against wild input, so I think it is unnecessary to duplicate that protection.

> 
> > LayoutTests/inspector/extensions/extensions-panel.html:74
> > +        panel.onSearch.addListener(function(action, query) {
> > +            output("Panel searched");
> > +        });
> 
> This does not seem to have any effect on the test output, so please remove it for now.

Will remove it for now. I am planning on adding a failing test in another patch for the `onSearch()` callback not being called in extension panels (see issue https://code.google.com/p/chromium/issues/detail?id=124800).
Comment 5 Jan Keromnes 2012-06-07 12:07:57 PDT
Created attachment 146355 [details]
Patch
Comment 6 johnjbarton 2012-06-07 12:48:59 PDT
(In reply to comment #4)
> What if the extension panel is a debugger and the runtime hits a breakpoint?
> 
> Another solution would be to make handlers (like the open resource handler) return a panel ID to bring up, but I'm not sure if it's a good idea.

In discussing this with Jan we came up with another idea.  

In general, given one event -- link clicked or breakpoint hit -- we want only one UI result. We don't want panels to fight over 'show' or 'breakpoint'. 

The 'link-clicked' example already solves this problem: the Settings panel has "Open Link In" which selects the link handler to be called. Thus even if two panels both have showPanel() calls in their handler, only one handler and thus one panel.show() will fire. 

We can reuse this idea for the breakpoint use case: a Settings panel entry for "Open Breakpoints In". 

We still have (at least ;-) two problems:

  1) do we allow show() to be called outside of a handler?  I think this is ok, the extension does not get control except through events (either fired at it by WebInspector or when the panel is visible).  

  2) will extension handlers want to be called but not want to do UI operations? For example I can imagine that a network analysis panel wants all events so it can store data, but it need not do UI unless the panel is selected by the user.  This means that we would like the Settings entry to control the UI operations not the handler calls. For example, two extensions might both get "breakpoint hit", both would call panel.show(), but only the one selected in Settings will succeed.

I think we should look through the existing events to see if we can divide them into "will need to do UI" and "no UI needed".  If they divide cleanly then we can use the one-handler-only solution.
Comment 7 Jan Keromnes 2012-06-07 17:04:12 PDT
Created attachment 146421 [details]
Patch
Comment 8 Jan Keromnes 2012-06-07 17:04:41 PDT
Should be good now.
Comment 9 Andrey Kosyakov 2012-06-08 08:11:54 PDT
(In reply to comment #6)
> In general, given one event -- link clicked or breakpoint hit -- we want only one UI result. We don't want panels to fight over 'show' or 'breakpoint'. 

Absolutely! This is why "open resource" is not a usual event and we came up with a notion of handler, where only one of the installed handlers will fire.

> 
> We can reuse this idea for the breakpoint use case: a Settings panel entry for "Open Breakpoints In". 

One of the reasons we don't have debugger API exposed yet is that giving extensions access to breakpoints requires careful consideration of how multiple extension will co-exist.

> We still have (at least ;-) two problems:
> 
>   1) do we allow show() to be called outside of a handler?  I think this is ok, the extension does not get control except through events (either fired at it by WebInspector or when the panel is visible).  

Those of the events that were not produced by the user gesture (e.g. network ones, or evaluation callbacks) should probably not cause focus change. I guess breakpoints/JS exception would be an exception from that rule.

>   2) will extension handlers want to be called but not want to do UI operations? For example I can imagine that a network analysis panel wants all events so it can store data, but it need not do UI unless the panel is selected by the user.  

Totally agree.

>This means that we would like the Settings entry to control the UI operations not the handler calls. For example, two extensions might both get "breakpoint hit", both would call panel.show(), but only the one selected in Settings will succeed.

Yup, that's definitely a good options. May be just "Active debugger".

> I think we should look through the existing events to see if we can divide them into "will need to do UI" and "no UI needed".  If they divide cleanly then we can use the one-handler-only solution.

+1.
Comment 10 Andrey Kosyakov 2012-06-08 08:17:24 PDT
Comment on attachment 146421 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146421&action=review

> Source/WebCore/inspector/front-end/ExtensionAPI.js:312
> +                Panels._canShow = true;
>                  callback.call(null, new Resource(message.resource), message.lineNumber);
> +                delete Panels._canShow;

There are two problems with this approach:
- _canShow is accessible to user code, so I would be able to do webInspector.panels.constructor._canShow = true; myPanel.show()
- if user callback throws, _canShow sticks.
Note that most of stuff defined within ExtensionAPI.js is actually within a closure, so just make var userGesture = false; within and use it.

> Source/WebCore/inspector/front-end/ExtensionAPI.js:405
> +        if (Panels._canShow) {

That would be more readable as a bail-out, e.g.
if (!Panels._canShow)
    return;
Comment 11 Vsevolod Vlasov 2012-06-08 09:38:50 PDT
Comment on attachment 146421 [details]
Patch

r- per Andrey's comments.
Comment 12 johnjbarton 2012-06-08 11:19:43 PDT
(In reply to comment #10).
> Note that most of stuff defined within ExtensionAPI.js is actually within a closure,

Just FYI, The source for ExtensionAPI.js does not show the closure. To see the closure you have to read and enjoy devtools_extension_api.js.
Comment 13 Andrey Kosyakov 2012-06-08 14:03:42 PDT
(In reply to comment #12)
> (In reply to comment #10).
> > Note that most of stuff defined within ExtensionAPI.js is actually within a closure,
> 
> Just FYI, The source for ExtensionAPI.js does not show the closure. To see the closure you have to read and enjoy devtools_extension_api.js.

There are actually several levels of closure :) The inner one is in ExtensionAPI.js (see injectedExtensionAPI() function here: http://trac.webkit.org/browser/trunk/Source/WebCore/inspector/front-end/ExtensionAPI.js?rev=118492#L94) -- that's the core of the API. The one that John refers is rather to take care of a few platform-specific fields, namespaces etc., and combine the core API with some definitions shared between the client and the server.
Comment 14 Jan Keromnes 2012-06-08 14:09:49 PDT
Created attachment 146646 [details]
Patch
Comment 15 Jan Keromnes 2012-06-08 14:19:57 PDT
(In reply to comment #13)
> There are actually several levels of closure :) The inner one is in ExtensionAPI.js (see injectedExtensionAPI() function here: http://trac.webkit.org/browser/trunk/Source/WebCore/inspector/front-end/ExtensionAPI.js?rev=118492#L94) -- that's the core of the API. The one that John refers is rather to take care of a few platform-specific fields, namespaces etc., and combine the core API with some definitions shared between the client and the server.

That's where I declared the flag. Is the new patch better?
Comment 16 Andrey Kosyakov 2012-06-08 14:26:20 PDT
Comment on attachment 146646 [details]
Patch

Looks much better now! Does the test still work?
Comment 17 Andrey Kosyakov 2012-06-08 14:32:50 PDT
Comment on attachment 146646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146646&action=review

> Source/WebCore/inspector/front-end/ExtensionAPI.js:409
> +        if (!userAction) return;

style: return needs to be on a line of its own.
Comment 18 Jan Keromnes 2012-06-08 14:39:51 PDT
Created attachment 146651 [details]
Patch
Comment 19 Jan Keromnes 2012-06-08 14:40:44 PDT
(In reply to comment #16)
> (From update of attachment 146646 [details])
> Looks much better now! Does the test still work?

Thanks! And yes, the test still works because it uses `extension_showPanel("extension")` to force the panel to show.

It would be nice to have a new test that clicks on a resource link and triggers the handler to show the panel, but that mixes up `extensions-resources.html` and `extensions-panel.html` and I didn't know how to make that work.

(In reply to comment #17)
> style: return needs to be on a line of its own.

Fixed.
Comment 20 Andrey Kosyakov 2012-06-11 11:55:16 PDT
(In reply to comment #19)
> (In reply to comment #16)
> > (From update of attachment 146646 [details] [details])
> > Looks much better now! Does the test still work?
> 
> Thanks! And yes, the test still works because it uses `extension_showPanel("extension")` to force the panel to show.
> 
> It would be nice to have a new test that clicks on a resource link and triggers the handler to show the panel, but that mixes up `extensions-resources.html` and `extensions-panel.html` and I didn't know how to make that work.

Yep -- we definitely need a test for the new method. Looks like it wold be mostly performed in the extension world, so I think it should be reasonably straightforward. You can move InspectorTest.clickOnURL to LayoutTests/http/tests/inspector/extensions-test.js and re-use it in your test.
Comment 21 Jan Keromnes 2012-06-11 20:42:31 PDT
Created attachment 147001 [details]
Patch
Comment 22 Jan Keromnes 2012-06-11 20:56:05 PDT
(In reply to comment #20)
> (In reply to comment #19)
> > It would be nice to have a new test that clicks on a resource link and triggers the handler to show the panel, but that mixes up `extensions-resources.html` and `extensions-panel.html` and I didn't know how to make that work.
> 
> Yep -- we definitely need a test for the new method. Looks like it wold be mostly performed in the extension world, so I think it should be reasonably straightforward. You can move InspectorTest.clickOnURL to LayoutTests/http/tests/inspector/extensions-test.js and re-use it in your test.

I wrote the test but I have 2 issues:

1) InspectorTest.clickOnURL cannot be moved to LayoutTests/http/tests/inspector/extensions-test.js, because it needs to log a message in the inspected page and evaluate an xpath query to get a valid resource link. This is why I copied and adapted the logMessage and clickOnURL functions from extensions-resources.html to extensions-panel.html.

2) In my test I created an open resource handler in which I call `panel.show()` to show the panel while authorized to do so, but in order for my handler to be called I need to modify the WebInspector "Open links in" Settings. I tried to do so with `webInspector.openAnchorLocationRegistry._activeHandler = panel._id;`, but `webInspector.openAnchorLocationRegistry` is undefined in extensions-panel.html -- why? How else can I modify the Settings so that the test calls my handler when a resource link is clicked?
Comment 23 WebKit Review Bot 2012-06-12 01:02:30 PDT
Comment on attachment 147001 [details]
Patch

Attachment 147001 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12941566

New failing tests:
inspector/extensions/extensions-panel.html
Comment 24 WebKit Review Bot 2012-06-12 01:02:35 PDT
Created attachment 147029 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 25 Jan Keromnes 2012-06-12 17:19:26 PDT
Created attachment 147199 [details]
Patch
Comment 26 Jan Keromnes 2012-06-12 17:20:35 PDT
(In reply to comment #20)
> Yep -- we definitely need a test for the new method. Looks like it wold be mostly performed in the extension world, so I think it should be reasonably straightforward. You can move InspectorTest.clickOnURL to LayoutTests/http/tests/inspector/extensions-test.js and re-use it in your test.

Test now works. Should I ask for commit-queue?
Comment 27 WebKit Review Bot 2012-06-14 03:34:43 PDT
Comment on attachment 147199 [details]
Patch

Clearing flags on attachment: 147199

Committed r120311: <http://trac.webkit.org/changeset/120311>
Comment 28 WebKit Review Bot 2012-06-14 03:34:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Beth Dakin 2012-06-22 15:56:49 PDT
The modifications to the layout test are causing it to fail most (but not all) of the time on the Mac bots. When it fails, it fails because "Panel shown" sometimes happens before "Extension panel size correct" instead of after. I'm going to skip it for now.
Comment 30 Beth Dakin 2012-06-22 16:10:33 PDT
Skipped test with http://trac.webkit.org/changeset/121067 and filed https://bugs.webkit.org/show_bug.cgi?id=89790