Bug 120647 – [GTK] Move to the new web inspector

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Gustavo Noronha (kov)]

Move the GTK+ port to the new inspector UI landed by Apple.

[Zan Dobersek]

Does the new inspector provide remote capabilities, like the old one did?

[Gustavo Noronha (kov)]

I'm currently trying to figure that out. The backend seems to still be the same, just the UI has changed, so I guess it should be possible with a not too huge amount of work if it does not yet.

[Gustavo Noronha (kov)]

According to Benjamin Poulain it does support the remote inspector, so I just need to figure out how it ties to the web socket in wk2 and what needs to be sent to the browser. I have a WIP patch with the regular inspector UI working, shall finish it tomorrow, hopefully.

[Gustavo Noronha (kov)]

Turns out I just needed to change the paths in the inspector server - I thought it wasn't working because the resources panel is erroring out and not showing the resources, but that should probably be fixed in a separate bug/patch, and the other bits work.

[Gustavo Noronha (kov)]

Created attachment 210455 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Seokju Kwon]

Comment on attachment 210455 [details]
Patch

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

> Source/WebInspectorUI/GNUmakefile.am:4
> +	$(WebInspectorUI)/Localizations/en.lproj//localizedStrings.js \

Remove '/' from '//'

> Source/WebInspectorUI/GNUmakefile.am:28
> +	$(WebInspectorUI)/Localizations/en.lproj//localizedStrings.js

ditto

[Gustavo Noronha (kov)]

(In reply to comment #7)
> > Source/WebInspectorUI/GNUmakefile.am:4
> > +	$(WebInspectorUI)/Localizations/en.lproj//localizedStrings.js \
> 
> Remove '/' from '//'

Bad copy/paste =( fixed locally.

[Gustavo Noronha (kov)]

Created attachment 210460 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 210460 [details]
Patch

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

> Source/WebInspectorUI/GNUmakefile.am:12
> +# Installing web inspector files
> +webinspectordir = ${datadir}/webkitgtk-@WEBKITGTK_API_VERSION@/InspectorUI
> +dist_webinspector_DATA = \
> +	$(WebInspectorUI)/Localizations/en.lproj/localizedStrings.js \
> +	$(shell ls $(WebInspectorUI)/UserInterface/*.html) \
> +	$(shell ls $(WebInspectorUI)/UserInterface/*.js) \
> +	$(shell ls $(WebInspectorUI)/UserInterface/*.css)
> +
> +webinspectorimagesdir = ${datadir}/webkitgtk-@WEBKITGTK_API_VERSION@/InspectorUI/Images
> +dist_webinspectorimages_DATA = \
> +	$(shell ls $(WebInspectorUI)/UserInterface/Images/*.png) \
> +	$(shell ls $(WebInspectorUI)/UserInterface/Images/*.svg)

I wonder if we can take advantage of the move to compile all inspector files as gresources. That would fix the problem of inspector not working because resources are not installed and loading the resources will be faster too.

[Martin Robinson]

Comment on attachment 210460 [details]
Patch

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

This patch is a lot simpler than I feared!

>> Source/WebInspectorUI/GNUmakefile.am:12
>> +	$(shell ls $(WebInspectorUI)/UserInterface/Images/*.svg)
> 
> I wonder if we can take advantage of the move to compile all inspector files as gresources. That would fix the problem of inspector not working because resources are not installed and loading the resources will be faster too.

It seems reasonable to split that out since this essentially just changes the set of resources the inspector uses.

[Gustavo Noronha (kov)]

Created attachment 210505 [details]
Patch

[EFL EWS Bot]

Comment on attachment 210505 [details]
Patch

Attachment 210505 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1693767

[Martin Robinson]

Comment on attachment 210505 [details]
Patch

Can you do a quick check to see how much this impacts the loading time of the shared object?

[Early Warning System Bot]

Comment on attachment 210505 [details]
Patch

Attachment 210505 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1701229

[Gustavo Noronha (kov)]

(In reply to comment #11)
> This patch is a lot simpler than I feared!

Indeed! I thought it would be a monster, turns out it's just a different set of HTML/js/css/images for the same backend code.

> > I wonder if we can take advantage of the move to compile all inspector files as gresources. That would fix the problem of inspector not working because resources are not installed and loading the resources will be faster too.
> 
> It seems reasonable to split that out since this essentially just changes the set of resources the inspector uses.

I went ahead and did it! It works well except for one bit: the WebKit2 inspector proxy will load anything that is not the inspector's main resource in the inspected view instead of in the inspector view. So I had to add a custom check - and added stuff to all ports to not have an #if/#else combo… not sure it's worth it. Maybe we should just have the #if/#else combo.

(In reply to comment #14)
> (From update of attachment 210505 [details])
> Can you do a quick check to see how much this impacts the loading time of the shared object?

I can try, do you have any methods for this in mind? I guess runtime of the testwebinspector unit test would be good enough?

[Build Bot]

Comment on attachment 210505 [details]
Patch

Attachment 210505 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1697422

[Gustavo Noronha (kov)]

I guess the more interesting would be to know the impact in the no-inspector case, so I took testloading for a ride. The results appear to back my theory that there's no preloading of the resources at startup time:

Without gresource:

0.20user 0.05system 0:00.34elapsed 74%CPU (0avgtext+0avgdata 45556maxresident)k
0inputs+112outputs (0major+22196minor)pagefaults 0swaps

With:

0.19user 0.06system 0:00.35elapsed 74%CPU (0avgtext+0avgdata 45592maxresident)k
0inputs+176outputs (0major+22206minor)pagefaults 0swaps

Both were variating between 0.18 and 0.20 user time, 0.05-0.06 system time, cpu usage was 73/74 for both, maxresident was also swinging between ~45550 and ~47000 on both, I think the impact is fairly negligible. The .o that was generated for the resources has around 3.3M, fwiw.

[Martin Robinson]

Comment on attachment 210505 [details]
Patch

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

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:321
> +    // Use KURL so we can compare just the fileSystemPaths.

I guess this comment is inaccurate now.

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:323
> +    KURL requestURL(KURL(), toImpl(requestRef)->url());

Does toImpl(requestRef)->url() already return a KURL? If so we probably don't need to construct it with an empty base URL.

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:325
> +    ASSERT(inspectorURL.isLocalFile());

Is this still true?

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:327
> +    return requestURL.isLocalFile() && requestURL.fileSystemPath() == inspectorURL.fileSystemPath();

Not sure what the result of calling isLocalFile and fileSystemPath on GResource URLs is...

[Build Bot]

Comment on attachment 210505 [details]
Patch

Attachment 210505 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1695609

[Gustavo Noronha (kov)]

(In reply to comment #19)
> > Source/WebKit2/UIProcess/WebInspectorProxy.cpp:321
> > +    // Use KURL so we can compare just the fileSystemPaths.
> 
> I guess this comment is inaccurate now.

This is actually the original code that is still used by the other ports.

> > Source/WebKit2/UIProcess/WebInspectorProxy.cpp:323
> > +    KURL requestURL(KURL(), toImpl(requestRef)->url());
> 
> Does toImpl(requestRef)->url() already return a KURL? If so we probably don't need to construct it with an empty base URL.

That returns a String;

[Gustavo Noronha (kov)]

Created attachment 210535 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 210535 [details]
Patch

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

I don't think we should consider resource:// as a custom scheme, but simply as a local scheme, adding it to the local schemes in the SchemeRegistry, the same way qt does for qrc or apple for applewebdata. fileSystemRepresentation() should just work with resource:// uris.

> Source/WebInspectorUI/GNUmakefile.am:13
> +	$(AM_V_at)echo '  <gresource prefix="/org/webkitgtk/InspectorUI">' >> ${GENSOURCES_WEBINSPECTOR_UI}/GResourceBundle.xml

What about /org/webkitgtk/ui/inspector ?

[Carlos Garcia Campos]

(In reply to comment #19)
> (From update of attachment 210505 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210505&action=review
> 
> > Source/WebKit2/UIProcess/WebInspectorProxy.cpp:321
> > +    // Use KURL so we can compare just the fileSystemPaths.
> 
> I guess this comment is inaccurate now.
> 
> > Source/WebKit2/UIProcess/WebInspectorProxy.cpp:323
> > +    KURL requestURL(KURL(), toImpl(requestRef)->url());
> 
> Does toImpl(requestRef)->url() already return a KURL? If so we probably don't need to construct it with an empty base URL.
> 
> > Source/WebKit2/UIProcess/WebInspectorProxy.cpp:325
> > +    ASSERT(inspectorURL.isLocalFile());
> 
> Is this still true?

It should, but we need to upate the SchemeRegistry.

> > Source/WebKit2/UIProcess/WebInspectorProxy.cpp:327
> > +    return requestURL.isLocalFile() && requestURL.fileSystemPath() == inspectorURL.fileSystemPath();
> 
> Not sure what the result of calling isLocalFile and fileSystemPath on GResource URLs is...

We need to add a custom implementation of KURL::fileSystemPath() like qt does for qrc.

[Carlos Garcia Campos]

(In reply to comment #18)
 
> Both were variating between 0.18 and 0.20 user time, 0.05-0.06 system time, cpu usage was 73/74 for both, maxresident was also swinging between ~45550 and ~47000 on both, I think the impact is fairly negligible. The .o that was generated for the resources has around 3.3M, fwiw.

We can reduce the binary size by compressing the text based resources. It complicates the makefile rules, I guess, but it's just a matter of adding compressed="true" for the resources we want to be compressed and it just works automatically. We might want to check how that impacts the performance, though.

[Gustavo Noronha (kov)]

(In reply to comment #23)
> (From update of attachment 210535 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210535&action=review
> 
> I don't think we should consider resource:// as a custom scheme, but simply as a local scheme, adding it to the local schemes in the SchemeRegistry, the same way qt does for qrc or apple for applewebdata. fileSystemRepresentation() should just work with resource:// uris.

I guess that would work, need to remove (or override as well) the url.isLocalFile() calls/assert.
 
> > Source/WebInspectorUI/GNUmakefile.am:13
> > +	$(AM_V_at)echo '  <gresource prefix="/org/webkitgtk/InspectorUI">' >> ${GENSOURCES_WEBINSPECTOR_UI}/GResourceBundle.xml
> 
> What about /org/webkitgtk/ui/inspector ?

Works for me =)

(In reply to comment #25)
> We can reduce the binary size by compressing the text based resources. It complicates the makefile rules, I guess, but it's just a matter of adding compressed="true" for the resources we want to be compressed and it just works automatically. We might want to check how that impacts the performance, though.

Should be simple to do, would it be ok if we optimize in a separate patch, though?

[Gustavo Noronha (kov)]

Created attachment 210607 [details]
Patch

[Carlos Garcia Campos]

(In reply to comment #26)
> (In reply to comment #25)
> > We can reduce the binary size by compressing the text based resources. It complicates the makefile rules, I guess, but it's just a matter of adding compressed="true" for the resources we want to be compressed and it just works automatically. We might want to check how that impacts the performance, though.
> 
> Should be simple to do, would it be ok if we optimize in a separate patch, though?

Sure

[Carlos Garcia Campos]

Comment on attachment 210607 [details]
Patch

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

> Source/WebCore/platform/gtk/KURLGtk.cpp:30
> +    if (protocolIs("resource"))

I guess this should be if (protocolIs("resource") || isLocalFile()), otherwise you return a null string for local files.

[Build Bot]

Comment on attachment 210607 [details]
Patch

Attachment 210607 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1694897

[Gustavo Noronha (kov)]

Created attachment 210612 [details]
Patch

[Build Bot]

Comment on attachment 210612 [details]
Patch

Attachment 210612 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1696703

[Build Bot]

Comment on attachment 210612 [details]
Patch

Attachment 210612 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1701409

[Gustavo Noronha (kov)]

Created attachment 210615 [details]
Patch

[Anton Obzhirov]

WEBKIT_INSPECTOR_SERVER_PATH was added originally to allow reading inspector resources files for the server not only from debug path or installation path but from any custom folder. This still might be handy for some environments so may be we should keep it.

[Gustavo Noronha (kov)]

Can you provide an example of a use-case that would use that strategy? Is that something you're using today in some fashion? I like the fact that we drop a lot of complexity by using the GResource approach, so I would prefer to stick to not having the environment variables unless there's a compelling use case.

[Carlos Garcia Campos]

Comment on attachment 210615 [details]
Patch

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

Looks great to me!

> Source/WebInspectorUI/GNUmakefile.am:29
> +webcore_built_sources += \
> +	DerivedSources/WebInspectorUI/GResourceBundle.c

I think you should add DerivedSources/WebInspectorUI/GResourceBundle.xml here too or in DISTCLEANFILES, so that it's deleted in distclean

> Source/WebKit2/GNUmakefile.am:39
> +webkit2gtk_built_sources += \
> +	DerivedSources/WebInspectorUI/WebKit2GResourceBundle.c

Ditto, DerivedSources/WebInspectorUI/WebKit2GResourceBundle.xml should added here or in DISTCLEANFILES

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:335
> -    ASSERT(inspectorURL.isLocalFile());
> +    ASSERT(WebCore::SchemeRegistry::shouldTreatURLSchemeAsLocal(inspectorURL.protocol()));
>  
>      // Allow loading of the main inspector file.
> -    if (requestURL.isLocalFile() && requestURL.fileSystemPath() == inspectorURL.fileSystemPath()) {
> +    if (WebCore::SchemeRegistry::shouldTreatURLSchemeAsLocal(requestURL.protocol()) && requestURL.fileSystemPath() == inspectorURL.fileSystemPath()) {

This change needs approval from a WebKit2 owner.

[Carlos Garcia Campos]

(In reply to comment #35)
> WEBKIT_INSPECTOR_SERVER_PATH was added originally to allow reading inspector resources files for the server not only from debug path or installation path but from any custom folder. This still might be handy for some environments so may be we should keep it.

We don't need it at all, resource *will* always be available, since they are included in the library at build time

[Anton Obzhirov]

(In reply to comment #36)
> Can you provide an example of a use-case that would use that strategy? Is that something you're using today in some fashion? I like the fact that we drop a lot of complexity by using the GResource approach, so I would prefer to stick to not having the environment variables unless there's a compelling use case.

Hm, I know that Simon was having problems using remote inspector in one of our environments, we should check what he thinks about it. He should be back from
his holiday next week.
For the use case - in some application frameworks based on WebKit engine there is no way to get web inspector page using usual means. You have to use remote inspector for debugging and sometime custom installation scripts are used
to install the framework so webkit resources might end up in different folder.

[Carlos Garcia Campos]

(In reply to comment #39)
> (In reply to comment #36)
> > Can you provide an example of a use-case that would use that strategy? Is that something you're using today in some fashion? I like the fact that we drop a lot of complexity by using the GResource approach, so I would prefer to stick to not having the environment variables unless there's a compelling use case.
> 
> Hm, I know that Simon was having problems using remote inspector in one of our environments, we should check what he thinks about it. He should be back from
> his holiday next week.
> For the use case - in some application frameworks based on WebKit engine there is no way to get web inspector page using usual means. You have to use remote inspector for debugging and sometime custom installation scripts are used
> to install the framework so webkit resources might end up in different folder.

After this patch, inspector resources will not end up in any folder, they will be compiled into the webkit shared library, so that they will *always* be available, not matter what prefix you install or even if you don't run make install at all.

[Gustavo Noronha (kov)]

Created attachment 211056 [details]
Patch

[Gustavo Noronha (kov)]

Just need wk2 OWNERS approval now, I guess! This one has been tested by distcheck =)

[Martin Robinson]

(In reply to comment #40)
aving the environment variables unless there's a compelling use case.
> > 
> > Hm, I know that Simon was having problems using remote inspector in one of our environments, we should check what he thinks about it. He should be back from
> > his holiday next week.
> > For the use case - in some application frameworks based on WebKit engine there is no way to get web inspector page using usual means. You have to use remote inspector for debugging and sometime custom installation scripts are used
> > to install the framework so webkit resources might end up in different folder.
> 
> After this patch, inspector resources will not end up in any folder, they will be compiled into the webkit shared library, so that they will *always* be available, not matter what prefix you install or even if you don't run make install at all.

It's pretty important that remote inspector support keeps working, if possible. Remote inspectors can run on other machines entirely. A typical usecase is to run the inspector on your laptop against a page running on a mobile device.

[Carlos Garcia Campos]

(In reply to comment #43)
> (In reply to comment #40)
> aving the environment variables unless there's a compelling use case.
> > > 
> > > Hm, I know that Simon was having problems using remote inspector in one of our environments, we should check what he thinks about it. He should be back from
> > > his holiday next week.
> > > For the use case - in some application frameworks based on WebKit engine there is no way to get web inspector page using usual means. You have to use remote inspector for debugging and sometime custom installation scripts are used
> > > to install the framework so webkit resources might end up in different folder.
> > 
> > After this patch, inspector resources will not end up in any folder, they will be compiled into the webkit shared library, so that they will *always* be available, not matter what prefix you install or even if you don't run make install at all.
> 
> It's pretty important that remote inspector support keeps working, if possible. Remote inspectors can run on other machines entirely. A typical usecase is to run the inspector on your laptop against a page running on a mobile device.

Do we need any local resource for a remote inspector running on another machine?

[Martin Robinson]

(In reply to comment #44)

> > It's pretty important that remote inspector support keeps working, if possible. Remote inspectors can run on other machines entirely. A typical usecase is to run the inspector on your laptop against a page running on a mobile device.
> 
> Do we need any local resource for a remote inspector running on another machine?

My understanding is that the inspector resources need to be on both machines, but perhaps I'm mistaken.

[Gustavo Noronha (kov)]

(In reply to comment #45)
> My understanding is that the inspector resources need to be on both machines, but perhaps I'm mistaken.

You are =). The remote inspector keeps working with this patch - the way the remote inspector works is the machine you access the inspector from connects on the port opened by WebKit on the target (remote machine) and requests the inspector resources from it.

If you look at the changes done to Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp you will see the code that serves the inspector resources and how it's using the resource:// URIs to serve them. I admitedly only tested on the same machine, but my system's Epiphany was able to look at the pages in the remote instance with the patch (full disclaimer: I actually broke it when I changed the GResource path following Carlos' suggestion, so good thing you asked, patch incoming! =D)

[Gustavo Noronha (kov)]

Created attachment 211070 [details]
Patch

[Anton Obzhirov]

(In reply to comment #46)
> (In reply to comment #45)
> > My understanding is that the inspector resources need to be on both machines, but perhaps I'm mistaken.
> 
> You are =). The remote inspector keeps working with this patch - the way the remote inspector works is the machine you access the inspector from connects on the port opened by WebKit on the target (remote machine) and requests the inspector resources from it.
> 
> If you look at the changes done to Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp you will see the code that serves the inspector resources and how it's using the resource:// URIs to serve them. I admitedly only tested on the same machine, but my system's Epiphany was able to look at the pages in the remote instance with the patch (full disclaimer: I actually broke it when I changed the GResource path following Carlos' suggestion, so good thing you asked, patch incoming! =D)

Yes, you are right :). Inspector server on remote machine fetches resources to local machine inspector. The idea is to use locally the version of resource files available for the inspector on remote machine.

[Anders Carlsson]

Comment on attachment 211070 [details]
Patch

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

> Source/WebCore/platform/gtk/KURLGtk.cpp:33
> +String KURL::fileSystemPath() const
> +{
> +    if (!isValid())
> +        return String();
> +
> +    if (isLocalFile() || protocolIs("resource"))
> +        return decodeURLEscapeSequences(path());
> +
> +    return String();

I don't think we should fragment the meaning of fileSystemPath like this. Pretty sure that new files should use the BSD license too.

[Carlos Garcia Campos]

(In reply to comment #49)
> (From update of attachment 211070 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211070&action=review
> 
> > Source/WebCore/platform/gtk/KURLGtk.cpp:33
> > +String KURL::fileSystemPath() const
> > +{
> > +    if (!isValid())
> > +        return String();
> > +
> > +    if (isLocalFile() || protocolIs("resource"))
> > +        return decodeURLEscapeSequences(path());
> > +
> > +    return String();
> 
> I don't think we should fragment the meaning of fileSystemPath like this. Pretty sure that new files should use the BSD license too.

Instead of adding our own fileSystemPath implementation, we can probably use decodeURLEscapeSequences directly in WebInspectorProxy since we are already checking it should be considered a local patch before using it. I wonder if there could be other places where fileSystemPath might be used with a resource:// URI, though. I guess adding an #ifdef for GTK platform in KURL::fileSystemPath is not an option either. Any other proposal?

[Carlos Garcia Campos]

Comment on attachment 211070 [details]
Patch

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

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:335
>      KURL inspectorURL(KURL(), webInspectorProxy->inspectorPageURL());
>      KURL requestURL(KURL(), toImpl(requestRef)->url());
>  
> -    ASSERT(inspectorURL.isLocalFile());
> +    ASSERT(WebCore::SchemeRegistry::shouldTreatURLSchemeAsLocal(inspectorURL.protocol()));
>  
>      // Allow loading of the main inspector file.
> -    if (requestURL.isLocalFile() && requestURL.fileSystemPath() == inspectorURL.fileSystemPath()) {
> +    if (WebCore::SchemeRegistry::shouldTreatURLSchemeAsLocal(requestURL.protocol()) && requestURL.fileSystemPath() == inspectorURL.fileSystemPath()) {

We could move all this to a helper function, something like WebInspectorProxy::isMainPageURL(toImpl(requestRef)->url()) and you create the KURLs there to compare their file system paths or the decodeURLEscapeSequences.

[Gustavo Noronha (kov)]

(In reply to comment #49)
> (From update of attachment 211070 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211070&action=review
> I don't think we should fragment the meaning of fileSystemPath like this. Pretty sure that new files should use the BSD license too.

Thanks for the review, are the change to use SchemeRegistry OK for you, then? I think the idea of using decodeURLEscapeSequences(path()) directly makes sense, would that be fine?

Regarding the license, I thought that was at the contributor's discretion to choose between BSD and LGPL. No strong opinion for this patch though, I can change it.

[Anders Carlsson]

(In reply to comment #52)
> (In reply to comment #49)
> > (From update of attachment 211070 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=211070&action=review
> > I don't think we should fragment the meaning of fileSystemPath like this. Pretty sure that new files should use the BSD license too.
> 
> Thanks for the review, are the change to use SchemeRegistry OK for you, then? I think the idea of using decodeURLEscapeSequences(path()) directly makes sense, would that be fine?

Hmm, unless I’m mistaken, isn’t there a way to register a scheme as being local, could we do that from the inspector code instead of changing SchemeRegistry.

Yeah, I think using decodeURLEscapeSequences would be fine.

> 
> Regarding the license, I thought that was at the contributor's discretion to choose between BSD and LGPL. No strong opinion for this patch though, I can change it.

No worries, I trust you.

[Gustavo Noronha (kov)]

Created attachment 211473 [details]
Patch

[Gustavo Noronha (kov)]

Thanks again! I moved registering the scheme as local to webkitgtk initialization - for wk2gtk I used the SecurityManager, which is the object responsible for this in our API, because it already has code that deals with the fact that the SchemeRegistry needs to be updated on both processes, I also think it's the proper place for it to go because it's the bit of code we'll be revisiting when we support multiple contexts, so we won't forget about it. How does this one look?

[Build Bot]

Comment on attachment 211473 [details]
Patch

Attachment 211473 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1874063

New failing tests:
fast/regions/firstletter-inside-flowthread.html
fast/regions/autoheight-singleregion-breakaftermargin.html
fast/regions/autoheight-singleregion-multiplebreaks.html
fast/regions/autowidth-normalflow-minmaxwidth.html
fast/regions/autoheight-singleregion-breakafteralways-maxheight.html
http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html
fast/regions/autoheight-vertical-lr.html
fast/regions/autoheight-horizontal-bt.html
fast/regions/autoheight-maxheight-region.html
fast/regions/autoheight-allregions-nobreaks.html
fast/regions/autoheight-singleregion-breakafteralways.html
fast/regions/autoheight-breakbeforealways.html
fast/regions/autoheight-firstregion-breakalways.html
fast/regions/autoheight-singleregion-breakbeforealways.html
fast/regions/autoheight-middleregion.html
fast/regions/autoheight-lastregion-overflowauto-breaksignored.html
fast/regions/hover-in-second-region.html
fast/regions/autoheight-lastregion-overflowauto.html
fast/regions/autowidth-normalflow-vertrl.html
fast/regions/autoheight-allregions.html
fast/regions/autoheight-secondregion.html
fast/regions/autoheight-definedheight-changenotdetected.html
fast/regions/listmarker-inside-flowthread.html
fast/regions/autoheight-vertical-rl.html
fast/regions/autoheight-singleregion-overflowauto-breaksignored.html
fast/regions/autoheight-secondregion-breakoutside.html
fast/regions/autoheight-singleregion-overflowauto.html
fast/regions/autoheight-dynamic-update.html
fast/regions/autowidth-normalflow-minwidth.html
fast/regions/autoheight-breakafteralways-maxheight.html

[Build Bot]

Created attachment 211488 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Carlos Garcia Campos]

Comment on attachment 211473 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:204
> +    priv->securityManager = adoptGRef(webkitSecurityManagerCreate(webContext.get()));
> +    webkit_security_manager_register_uri_scheme_as_local(priv->securityManager.get(), "resource");
> +

I think it would be better to initialize this in WebContext::platformInitializeWebProcess (in WebContextGtk.cpp). You can append "resource" to parameters.urlSchemesRegisteredAsLocal if not present. Benefits:

 - It will work also for users of the C API (WTR, unit tests)
 - You avoid an IPC message right after creating the web context.
 - You don't create the WebKitSecurityManager object only for this, which is created on demand on purpose.

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:335
>      KURL inspectorURL(KURL(), webInspectorProxy->inspectorPageURL());
>      KURL requestURL(KURL(), toImpl(requestRef)->url());
>  
> -    ASSERT(inspectorURL.isLocalFile());
> +    ASSERT(WebCore::SchemeRegistry::shouldTreatURLSchemeAsLocal(inspectorURL.protocol()));
>  
>      // Allow loading of the main inspector file.
> -    if (requestURL.isLocalFile() && requestURL.fileSystemPath() == inspectorURL.fileSystemPath()) {
> +    if (WebCore::SchemeRegistry::shouldTreatURLSchemeAsLocal(requestURL.protocol()) && decodeURLEscapeSequences(requestURL.path()) == decodeURLEscapeSequences(inspectorURL.path())) {

I still think all this would look better in a helper function.

[Gustavo Noronha (kov)]

Comment on attachment 211473 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:204
>> +
> 
> I think it would be better to initialize this in WebContext::platformInitializeWebProcess (in WebContextGtk.cpp). You can append "resource" to parameters.urlSchemesRegisteredAsLocal if not present. Benefits:
> 
>  - It will work also for users of the C API (WTR, unit tests)
>  - You avoid an IPC message right after creating the web context.
>  - You don't create the WebKitSecurityManager object only for this, which is created on demand on purpose.

That was my first take on it, the cons of doing it this way is I'll have to also register it in the ui process' SchemeRegistry manually. No big deal, though.

[Carlos Garcia Campos]

(In reply to comment #59)
> (From update of attachment 211473 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211473&action=review
> 
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:204
> >> +
> > 
> > I think it would be better to initialize this in WebContext::platformInitializeWebProcess (in WebContextGtk.cpp). You can append "resource" to parameters.urlSchemesRegisteredAsLocal if not present. Benefits:
> > 
> >  - It will work also for users of the C API (WTR, unit tests)
> >  - You avoid an IPC message right after creating the web context.
> >  - You don't create the WebKitSecurityManager object only for this, which is created on demand on purpose.
> 
> That was my first take on it, the cons of doing it this way is I'll have to also register it in the ui process' SchemeRegistry manually. No big deal, though.

Yes, we need to keep make sure SchemeRegistry in UI and WebProcess are in sync, that's what the security manager does too.

[Gustavo Noronha (kov)]

Created attachment 211546 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 211546 [details]
Patch

This looks good to me, but please make sure Anders has no objections before landing it. Thanks!

[Anders Carlsson]

(In reply to comment #62)
> (From update of attachment 211546 [details])
> This looks good to me, but please make sure Anders has no objections before landing it. Thanks!

Go for it!

[Gustavo Noronha (kov)]

Comment on attachment 211546 [details]
Patch

Clearing flags on attachment: 211546

Committed r155714: <http://trac.webkit.org/changeset/155714>

[Gustavo Noronha (kov)]

All reviewed patches have been landed.  Closing bug.