Move the GTK+ port to the new inspector UI landed by Apple.
Does the new inspector provide remote capabilities, like the old one did?
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.
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.
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.
Created attachment 210455 [details] Patch
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
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
(In reply to comment #7) > > Source/WebInspectorUI/GNUmakefile.am:4 > > + $(WebInspectorUI)/Localizations/en.lproj//localizedStrings.js \ > > Remove '/' from '//' Bad copy/paste =( fixed locally.
Created attachment 210460 [details] Patch
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.
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.
Created attachment 210505 [details] Patch
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
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?
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
(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?
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
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.
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...
Comment on attachment 210505 [details] Patch Attachment 210505 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1695609
(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;
Created attachment 210535 [details] Patch
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 ?
(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.
(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.
(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?
Created attachment 210607 [details] Patch
(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
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.
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
Created attachment 210612 [details] Patch
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
Comment on attachment 210612 [details] Patch Attachment 210612 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1701409
Created attachment 210615 [details] Patch
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.
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.
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.
(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
(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.
(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.
Created attachment 211056 [details] Patch
Just need wk2 OWNERS approval now, I guess! This one has been tested by distcheck =)
(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.
(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?
(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.
(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)
Created attachment 211070 [details] Patch
(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.
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.
(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?
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.
(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.
(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.
Created attachment 211473 [details] Patch
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?
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
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
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.
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.
(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.
Created attachment 211546 [details] Patch
Comment on attachment 211546 [details] Patch This looks good to me, but please make sure Anders has no objections before landing it. Thanks!
(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!
Comment on attachment 211546 [details] Patch Clearing flags on attachment: 211546 Committed r155714: <http://trac.webkit.org/changeset/155714>
All reviewed patches have been landed. Closing bug.