[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 #20 from Andrey Kosyakov <caseq at chromium.org>  2013-02-01 09:46:53 PST ---
(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 185286 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=185286&action=review
> > 
> > > Source/WebCore/inspector/front-end/ExtensionServer.js:348
> > > +        var status = this.evaluate(message.expression, true, true, message.evaluateOptions, port._extensionOrigin, callback.bind(this));
> > > +        if (status) {
> > > +            callback.call(this, "Extension server error: " + String.vsprintf(status.description, status.details));
> > > +        }
> > 
> > I still think this should not be necessary: the convention is that if the error condition is immediately detect by a handler, the handler returns the error and then ExtensionServer::_onmessage will dispatch the error to the client callback.
> 
> But without this change the handler never sees the error condition! If evaluate() returns say, E_NOTFOUND, and we ignore the return value (as without this patch) how can _onMessage dispatch an error to the client? It does not even see the error. 
> 

Here's the code that is responsible to dispatching the error to the client:

http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/inspector/front-end/ExtensionServer.js&exact_package=chromium&q=onmessage%20file:extensionserver&l=724

(lines 724, 728 & 729)

You can hit it in existing code if you tweak one of the tests (duh -- we need a test for this as well). If you changle file:/// to something else in this line, you will see that the callback is invoked even upon error:

http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/LayoutTests/inspector/extensions/extensions-eval-content-script.html&exact_package=chromium&q=file:extensions-eval-&type=cs&l=9

That said, the way we report these errors to the client is quite unfortunate. I'm about to fix that, see bug 108640.

> > Can we just resturn frames array from ResourceTreeModel. I think it would require fewer callbacks and make code more readable.
> 
> Because _frames in ResourceTreeModel is not an array, it's a map from frame.id -> frame. We can create an array if you prefer.

I think toy can either return the entire map or perhaps just Object.values()

> I was just trying to follow the practice in that file.

I think forAllResources is implemented this way because the iteration over tree is too complicated to perform on the client side. For iteration over map or array, on the other hand, inversion of control looks like overkill :)


> > > LayoutTests/http/tests/inspector/extensions-eval.html:75
> > > +    var segments = url.split('/'); // http://a.b.c:xxx/path
> > > +    var origin = segments[0] + '//' + segments[2];
> > 
> > I think just hard-coding it would make it more readable.
> 
> What value should I use then?

Just http://127.0.0.1:8000/?

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