Bug 84130 – [SOUP] Add a way to register custom uri schemes in WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

In WebKit2 the soup session and all its features are in the WebProcess, so we can let apps add their own features to the session to implement things like we do in WebKit1. In order to allow applications to register their own uri schemes, we need to implement SoupRequest feature ni the web process and provide IPC mesassages to register schemes and handle uri scheme requests.

[Carlos Garcia Campos]

Created attachment 137488 [details]
Patch

This patch introduces new C API for ports using soup. It will be tested by GTK+ unit tests.

[WebKit Review Bot]

Attachment 137488 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/WT..." exit_code: 1
Source/WTF/wtf/gobject/GRefPtr.h:211:  The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WTF/wtf/gobject/GRefPtr.h:212:  The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

(In reply to comment #2)
> Attachment 137488 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/WT..." exit_code: 1
> Source/WTF/wtf/gobject/GRefPtr.h:211:  The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Source/WTF/wtf/gobject/GRefPtr.h:212:  The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Total errors found: 2 in 32 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

All other methods in that file use the same pattern, so we should either fix the style in that file or add an exception to the style checker.

[Philippe Normand]

Comment on attachment 137488 [details]
Patch

Attachment 137488 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12418519

[Carlos Garcia Campos]

Created attachment 137501 [details]
Updated patch to fix the build

I forgot to git add the .messages.in files.

[WebKit Review Bot]

Attachment 137501 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/WT..." exit_code: 1
Source/WTF/wtf/gobject/GRefPtr.h:211:  The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WTF/wtf/gobject/GRefPtr.h:212:  The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 137501 [details]
Updated patch to fix the build

Attachment 137501 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12423174

[Carlos Garcia Campos]

Created attachment 137504 [details]
Another build fix

There was another file missing in the patch, sorry.

[WebKit Review Bot]

Attachment 137504 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/WT..." exit_code: 1
Source/WTF/wtf/gobject/GRefPtr.h:211:  The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WTF/wtf/gobject/GRefPtr.h:212:  The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

(In reply to comment #3)

> All other methods in that file use the same pattern, so we should either fix the style in that file or add an exception to the style checker.

It's probably best to write the code in a way that makes the style bot happy now and worry about fixing the old code later.

[Martin Robinson]

Comment on attachment 137504 [details]
Another build fix

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

This looks pretty good, but I think renaming the IPC message and avoiding __BUILDING_SOUP warrant a new version of the patch.

>> Source/WTF/wtf/gobject/GRefPtr.h:211
>> +template <> GPtrArray* refGPtr(GPtrArray* ptr);
> 
> The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]

As I said above, we write new code so that it doesn't cause style errors. Likely the style bot was updated to catch these errors, so now we must avoid them.

> Source/WebKit2/UIProcess/API/C/WKAPICast.h:366
>  #if defined(BUILDING_GTK__)
>  #include "WKAPICastGtk.h"
>  #endif
> +
> +#if defined(BUILDING_SOUP__)
> +#include "WKAPICastSoup.h"
> +#endif
> +

In these cases we should be probably be using PLATFORM(SOUP/GTK) or WTF_PLATFORM_SOUP/GTK instead of duplicating the meaning of that macro with BUILDING_SOUP__.

> Source/WebKit2/WebProcess/soup/WebSoupRequestManager.cpp:74
> +    genericRequestClass->schemes = const_cast<const char**>(reinterpret_cast<char**>(m_schemes->pdata));

The interior cast can be a static_cast here instead of a reinterpret_cast, I believe.

> Source/WebKit2/WebProcess/soup/WebSoupRequestManager.cpp:78
> +void WebSoupRequestManager::sendURIRequest(const CoreIPC::DataReference& requestData, const String& mimeType, uint64_t requestID)

You might consider renaming this message. I was confused a while, because I wondered why the UIProcess would be sending a URI request. Then I realized it was just the result of naming the message "sendURIRequest." Perhaps something like handleCustomURIRequest would make sense on both sides of the IPC channel.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:3112
> +                and not modified_identifier.startswith('webkit_soup')

Nice.

[Carlos Garcia Campos]

(In reply to comment #11)
> (From update of attachment 137504 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137504&action=review
> 
> This looks pretty good, but I think renaming the IPC message and avoiding __BUILDING_SOUP warrant a new version of the patch.

Thanks for reviewing!

> >> Source/WTF/wtf/gobject/GRefPtr.h:211
> >> +template <> GPtrArray* refGPtr(GPtrArray* ptr);
> > 
> > The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> As I said above, we write new code so that it doesn't cause style errors. Likely the style bot was updated to catch these errors, so now we must avoid them.

Fair enough

> > Source/WebKit2/UIProcess/API/C/WKAPICast.h:366
> >  #if defined(BUILDING_GTK__)
> >  #include "WKAPICastGtk.h"
> >  #endif
> > +
> > +#if defined(BUILDING_SOUP__)
> > +#include "WKAPICastSoup.h"
> > +#endif
> > +
> 
> In these cases we should be probably be using PLATFORM(SOUP/GTK) or WTF_PLATFORM_SOUP/GTK instead of duplicating the meaning of that macro with BUILDING_SOUP__.

This is a public header, that's why it uses BUILDING_GTK__ also instead of PLATFORM(GTK)

> > Source/WebKit2/WebProcess/soup/WebSoupRequestManager.cpp:74
> > +    genericRequestClass->schemes = const_cast<const char**>(reinterpret_cast<char**>(m_schemes->pdata));
> 
> The interior cast can be a static_cast here instead of a reinterpret_cast, I believe.

I think I tried that, but I'll try again to be sure.

> > Source/WebKit2/WebProcess/soup/WebSoupRequestManager.cpp:78
> > +void WebSoupRequestManager::sendURIRequest(const CoreIPC::DataReference& requestData, const String& mimeType, uint64_t requestID)
> 
> You might consider renaming this message. I was confused a while, because I wondered why the UIProcess would be sending a URI request. Then I realized it was just the result of naming the message "sendURIRequest." Perhaps something like handleCustomURIRequest would make sense on both sides of the IPC channel.

Ok, it was motivated by the SoupRequest interface, but I agree it can be confusing.

[Martin Robinson]

(In reply to comment #12)

> > In these cases we should be probably be using PLATFORM(SOUP/GTK) or WTF_PLATFORM_SOUP/GTK instead of duplicating the meaning of that macro with BUILDING_SOUP__.
> 
> This is a public header, that's why it uses BUILDING_GTK__ also instead of PLATFORM(GTK)

Is this truly the case? I notice that the header is C++ (it uses the class keyword) and also that it uses other WTF macros such as ENABLE(INSPECTOR).

[Carlos Garcia Campos]

(In reply to comment #13)
> (In reply to comment #12)
> 
> > > In these cases we should be probably be using PLATFORM(SOUP/GTK) or WTF_PLATFORM_SOUP/GTK instead of duplicating the meaning of that macro with BUILDING_SOUP__.
> > 
> > This is a public header, that's why it uses BUILDING_GTK__ also instead of PLATFORM(GTK)
> 
> Is this truly the case? I notice that the header is C++ (it uses the class keyword) and also that it uses other WTF macros such as ENABLE(INSPECTOR).

Oh, I'll check, in that case it's much better to simply use USE(SOUP) and avoid adding new macros

[Carlos Garcia Campos]

(In reply to comment #12)
> > > Source/WebKit2/UIProcess/API/C/WKAPICast.h:366
> > >  #if defined(BUILDING_GTK__)
> > >  #include "WKAPICastGtk.h"
> > >  #endif
> > > +
> > > +#if defined(BUILDING_SOUP__)
> > > +#include "WKAPICastSoup.h"
> > > +#endif
> > > +
> > 
> > In these cases we should be probably be using PLATFORM(SOUP/GTK) or WTF_PLATFORM_SOUP/GTK instead of duplicating the meaning of that macro with BUILDING_SOUP__.
> 
> This is a public header, that's why it uses BUILDING_GTK__ also instead of PLATFORM(GTK)

Ok, switched to use USE(SOUP) in WkAPICast.h, but BUILDING_SOUP is still needed for WKBase.h

> > > Source/WebKit2/WebProcess/soup/WebSoupRequestManager.cpp:74
> > > +    genericRequestClass->schemes = const_cast<const char**>(reinterpret_cast<char**>(m_schemes->pdata));
> > 
> > The interior cast can be a static_cast here instead of a reinterpret_cast, I believe.
> 
> I think I tried that, but I'll try again to be sure.

error: invalid static_cast from type 'void**' to type 'char**'

Only Chuck Norris knows which C++ style cast to use without trying them all until one works :-D

[Carlos Garcia Campos]

Created attachment 138059 [details]
Updated patch according to review comments

[Carlos Garcia Campos]

Committed r115411: <http://trac.webkit.org/changeset/115411>