[Webkit-unassigned] [Bug 106811] Web Inspector: Add iframe option to inspectedWindow.eval() extension API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=106811





--- Comment #7 from johnjbarton <johnjbarton at chromium.org>  2013-01-16 10:10:43 PST ---
(In reply to comment #6)
> (From update of attachment 182649 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182649&action=review
> 
> > Source/WebCore/inspector/front-end/ExtensionServer.js:801
> > +            } else if (options.frame && typeof options.frame === "object" && options.frame.url && options.frame.securityOrigin) {
> 
> can we support options.frame and options.useContentScriptContext at the same time? I can imagine this may be useful.

Yes, good idea.

> 
> > Source/WebCore/inspector/front-end/ExtensionServer.js:810
> > +                            context = contextLists[i].contextBySecurityOrigin(options.frame.securityOrigin);    
> 
> So this will allow an extension to evaluate in content script of other extensions. This may be a bit more than we would like to allow now. How about just using frame.url and handling useContentScriptContext.

It allows a devtools extension to evaluate in the context of another browser extension. Two browser extensions are 'peers': they have equivalent possible permissions and powers. Allowing browser extensions to operate on other browser extensions could lead to confusing interference. 

Devtools extensions are not peers of browser extensions: they are explicitly -- by the specific user opening devtools -- granted extra power -- eg the ability to eval() directly into the context of web page. Shouldn't devtools extensions should be able to eval() into browser extensions just like devtools can?

> 
> > Source/WebCore/inspector/front-end/ExtensionServer.js:816
> > +                    callback(errorMessage);
> 
> this should not be necessary, if you return an error status, the caller will invoke callback.
> 
> > Source/WebCore/inspector/front-end/ExtensionServer.js:817
> > +                    return this._status.E_FAILED(errorMessage);
> 
> I think E_NOTFOUND is more appropriate here, E_FAILED is too generic.

Actually the opposite was my experience: I started with E_NOTFOUND but I could not figure out what to do with an error message like 'Not found "http://localhost:9696"'. I realize that E_NOTFOUND makes better documentation in the source code but the resulting error message is not as good for the extension author.

> 
> > LayoutTests/inspector/extensions/extensions-eval-expected.txt:19
> > +log: Extension server error: Operation failed: No context with URL: "file:///work/chromium/src/third_party/WebKit/LayoutTests/inspector/extensions/resources/extensions-frame-eval.html" and origin "bogus"
> 
> Well.. That's not going to ever pass on the bots :)

It would if everyone switched to my file system layout ;-)  Too much cut and paste I can see.

> 
> > LayoutTests/inspector/extensions/extensions-eval.html:49
> > +function extension_testZEvalInIFrame(nextTest) {
> 
> Z?

For some reason the test issues tests.sort() and I wanted this test to run last.

> 
> > LayoutTests/inspector/extensions/extensions-eval.html:50
> > +    var iframeSrc = 'resources/extensions-frame-eval.html';
> 
> I think a good test would be to try something cross-origin -- i.e. move it under http tests and use http://127.0.0.1:8000/ as on origin.

Sure.

Let me know about the other issues here and I will resubmit the patch.

Thanks!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.