Bug 88473

Created attachment 146167 [details] Patch

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 on attachment 146167 [details] Patch r- per Andrey's comments.

(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).

Created attachment 146355 [details] Patch

(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.

Created attachment 146421 [details] Patch

Should be good now.

(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 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 on attachment 146421 [details] Patch r- per Andrey's comments.

(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.

(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.

Created attachment 146646 [details] Patch

(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 on attachment 146646 [details] Patch Looks much better now! Does the test still work?

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.

Created attachment 146651 [details] Patch

(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.

(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.

Created attachment 147001 [details] Patch

(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 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

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

Created attachment 147199 [details] Patch

(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 on attachment 147199 [details] Patch Clearing flags on attachment: 147199 Committed r120311: <http://trac.webkit.org/changeset/120311>

All reviewed patches have been landed. Closing bug.

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.

Skipped test with http://trac.webkit.org/changeset/121067 and filed https://bugs.webkit.org/show_bug.cgi?id=89790