[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 #15 from Andrey Kosyakov <caseq at chromium.org>  2013-01-29 09:40:43 PST ---
(From update of attachment 185025)
View in context: https://bugs.webkit.org/attachment.cgi?id=185025&action=review

> Source/WebCore/ChangeLog:91
>          (WebCore::RenderBox::exclusionShapeOutsideInfo): Ditto.
>  
> +        * inspector/front-end/ExtensionServer.js:

Looks like a merge error.

> 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));
> +        }

Why is this necessary? I don't think this will pass type validation by closure compiler.

> Source/WebCore/inspector/front-end/ExtensionServer.js:794
> +            } else if (options.scriptExecutionContext) {

Please drop redundant braces.

> Source/WebCore/inspector/front-end/ExtensionServer.js:797
> +            var url;

Redundant?

> Source/WebCore/inspector/front-end/ExtensionServer.js:803
> +                    if (contextLists[i].url === url) {

I would expect options.frameURL to be used to resolve frame (perhaps with the help of ResourceTreeModel), then use contextByFrameAndSecurityOrigin()

> Source/WebCore/inspector/front-end/ExtensionServer.js:804
> +                        if (!contextSecurityOrigin || contextSecurityOrigin && url.indexOf(contextSecurityOrigin) === 0) { 

if you choose to iterate over context list, the frame URL should be compared against the URL of the frame the context belongs to, not against the security origin.

> Source/WebCore/inspector/front-end/ExtensionServer.js:816
> +            } else if (contextSecurityOrigin) {

This starts to look a bit complicated. I would suggest something like:
var frame = options.frameURL ? resolveURLToFrame(options.frameURL) : WebInspector.resourceTreeModel.mainFrame;
if (!frame)
    return this._status.E_NOTFOUND(options.frameURL || "<top>")
if (contextSecurityOrigin) {
    context = WebInspector.runtimeModel.contextByFrameAndSecurityOrigin(frame, contextSecurityOrigin);
    if (!context)
         return this._status.E_NOTFOUND(contextSecurityOrigin)
}

> LayoutTests/ChangeLog:108
> +        * http/tests/inspector/extensions-eval-expected.txt: Renamed from LayoutTests/inspector/extensions/extensions-eval-expected.txt.
> +        * http/tests/inspector/extensions-eval.html: Renamed from LayoutTests/inspector/extensions/extensions-eval.html.
> +        * http/tests/inspector/resources/extensions-frame-eval.html: Added.

Merge error?

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