Bug 84130 - [SOUP] Add a way to register custom uri schemes in WebKit2
Summary: [SOUP] Add a way to register custom uri schemes in WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, Soup
Depends on:
Blocks: 84133
  Show dependency treegraph
 
Reported: 2012-04-17 01:19 PDT by Carlos Garcia Campos
Modified: 2012-04-27 00:13 PDT (History)
10 users (show)

See Also:


Attachments
Patch (59.68 KB, patch)
2012-04-17 01:49 PDT, Carlos Garcia Campos
pnormand: commit-queue-
Details | Formatted Diff | Diff
Updated patch to fix the build (64.03 KB, patch)
2012-04-17 02:27 PDT, Carlos Garcia Campos
gustavo: commit-queue-
Details | Formatted Diff | Diff
Another build fix (65.88 KB, patch)
2012-04-17 02:40 PDT, Carlos Garcia Campos
mrobinson: review-
Details | Formatted Diff | Diff
Updated patch according to review comments (65.94 KB, patch)
2012-04-20 01:17 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-04-17 01:19:15 PDT
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.
Comment 1 Carlos Garcia Campos 2012-04-17 01:49:23 PDT
Created attachment 137488 [details]
Patch

This patch introduces new C API for ports using soup. It will be tested by GTK+ unit tests.
Comment 2 WebKit Review Bot 2012-04-17 01:53:21 PDT
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.
Comment 3 Carlos Garcia Campos 2012-04-17 01:54:44 PDT
(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.
Comment 4 Philippe Normand 2012-04-17 01:56:14 PDT
Comment on attachment 137488 [details]
Patch

Attachment 137488 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12418519
Comment 5 Carlos Garcia Campos 2012-04-17 02:27:03 PDT
Created attachment 137501 [details]
Updated patch to fix the build

I forgot to git add the .messages.in files.
Comment 6 WebKit Review Bot 2012-04-17 02:31:23 PDT
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.
Comment 7 Gustavo Noronha (kov) 2012-04-17 02:35:33 PDT
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
Comment 8 Carlos Garcia Campos 2012-04-17 02:40:31 PDT
Created attachment 137504 [details]
Another build fix

There was another file missing in the patch, sorry.
Comment 9 WebKit Review Bot 2012-04-17 02:44:10 PDT
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.
Comment 10 Martin Robinson 2012-04-17 05:59:02 PDT
(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.
Comment 11 Martin Robinson 2012-04-19 15:30:17 PDT
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.
Comment 12 Carlos Garcia Campos 2012-04-19 23:20:22 PDT
(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.
Comment 13 Martin Robinson 2012-04-19 23:25:51 PDT
(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).
Comment 14 Carlos Garcia Campos 2012-04-19 23:32:29 PDT
(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
Comment 15 Carlos Garcia Campos 2012-04-20 01:02:37 PDT
(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
Comment 16 Carlos Garcia Campos 2012-04-20 01:17:41 PDT
Created attachment 138059 [details]
Updated patch according to review comments
Comment 17 Carlos Garcia Campos 2012-04-27 00:13:04 PDT
Committed r115411: <http://trac.webkit.org/changeset/115411>