Bug 155497 – [GTK] WebInspector broken after r197620

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Alejandro G. Castro]

I get these errors when I try to open it:

resource:///org/webkitgtk/inspector/UserInterface/Base/Setting.js:61:44: CONSOLE ERROR SecurityError: DOM Exception 18: An attempt was made to break through the security policy of the user agent.
resource:///org/webkitgtk/inspector/UserInterface/Main.html:1:21: CONSOLE ERROR ReferenceError: Can't find variable: InspectorFrontendAPI
resource:///org/webkitgtk/inspector/UserInterface/Main.html:1:21: CONSOLE ERROR ReferenceError: Can't find variable: InspectorFrontendAPI
resource:///org/webkitgtk/inspector/UserInterface/Main.html:1:21: CONSOLE ERROR ReferenceError: Can't find variable: InspectorFrontendAPI
resource:///org/webkitgtk/inspector/UserInterface/Main.html:1:21: CONSOLE ERROR ReferenceError: Can't find variable: InspectorFrontendAPI
resource:///org/webkitgtk/inspector/UserInterface/Protocol/InspectorBackendCommands.js:32:17: CONSOLE ERROR ReferenceError: Can't find variable: InspectorBackend

[Radar WebKit Bug Importer]

<rdar://problem/25171910>

[Timothy Hatcher]

You likely need the same change that landed for Mac to fix this. See bug 155265.

http://trac.webkit.org/changeset/197920/trunk/Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm

[Alejandro G. Castro]

Created attachment 274289 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 274289 [details]
Patch

This patch is correct, because we register resource scheme as local, but I think that was a mistake in the first place. A GResources is not like a local file, you can't replace by another file, for example, they are inmutable, always available and never fail to load. So, I think we should stop doing considering resource:// as local, which should also fix this problem.

[Carlos Garcia Campos]

Created attachment 274291 [details]
Alternative patch

This is an alternative approach to stop considering resource:// as local.

[Philippe Normand]

Comment on attachment 274291 [details]
Alternative patch

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

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:833
> +bool WebInspectorProxy::isValidInspectorURL(const URL& url)
> +{
> +    return url.isLocalFile();
> +}

Shouldn't this rather use WebCore::SchemeRegistry::shouldTreatURLSchemeAsLocal() ?

[Carlos Garcia Campos]

(In reply to comment #6)
> Comment on attachment 274291 [details]
> Alternative patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274291&action=review
> 
> > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:833
> > +bool WebInspectorProxy::isValidInspectorURL(const URL& url)
> > +{
> > +    return url.isLocalFile();
> > +}
> 
> Shouldn't this rather use
> WebCore::SchemeRegistry::shouldTreatURLSchemeAsLocal() ?

No, we changed that for GTK+ when we started to use GResources for the inspector, so now that we no longer consider GResources as local, we can bring back the previous code.

[Alejandro G. Castro]

(In reply to comment #4)
> Comment on attachment 274289 [details]
> Patch
> 
> This patch is correct, because we register resource scheme as local, but I
> think that was a mistake in the first place. A GResources is not like a
> local file, you can't replace by another file, for example, they are
> inmutable, always available and never fail to load. So, I think we should
> stop doing considering resource:// as local, which should also fix this
> problem.

I sounds like a better approach to me, thanks.

[Philippe Normand]

Comment on attachment 274291 [details]
Alternative patch

Alright then, looks good to me!

[Carlos Garcia Campos]

(In reply to comment #9)
> Comment on attachment 274291 [details]
> Alternative patch
> 
> Alright then, looks good to me!

Thanks! This requires WebKit2 owner approval, since I'm changing WebKit2 cross-platform code

[Darin Adler]

Comment on attachment 274291 [details]
Alternative patch

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

Why add these functions just so we can make assertions? Lets just remove the assertions and not add the functions.

> Source/WebKit2/UIProcess/WebInspectorProxy.h:61
> +};

Semicolon here is incorrect.

[Carlos Garcia Campos]

(In reply to comment #11)
> Comment on attachment 274291 [details]
> Alternative patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274291&action=review
> 
> Why add these functions just so we can make assertions? Lets just remove the
> assertions and not add the functions.

There's also one actual check, and then the two assertions. But I think we can indeed get rid of it entirely, since then we are comparing the given URL with the platform provided main URL, so the initial check looks like an optimization that is never actually happening.

> > Source/WebKit2/UIProcess/WebInspectorProxy.h:61
> > +};
> 
> Semicolon here is incorrect.

[Carlos Garcia Campos]

Created attachment 274646 [details]
Patch for landing.

Darin, please set cq+ if you are ok with this.

[WebKit Commit Bot]

Comment on attachment 274646 [details]
Patch for landing.

Clearing flags on attachment: 274646

Committed r198532: <http://trac.webkit.org/changeset/198532>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.