Bug 84130

Summary: [SOUP] Add a way to register custom uri schemes in WebKit2
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia@igalia.com>
Component: WebKit2Assignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth@webkit.org, dpranke@chromium.org, gns@gnome.org, levin@chromium.org, mrobinson@webkit.org, ojan@chromium.org, pnormand@igalia.com, svillar@igalia.com, webkit.review.bot@gmail.com, xan.lopez@gmail.com
Priority: P2 Keywords: Gtk, Soup
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 84133    
Attachments:
Description Flags
Patch
pnormand: commit‑queue-
Updated patch to fix the build
gns: commit‑queue-
Another build fix
mrobinson: review-
Updated patch according to review comments mrobinson: review+

Description From 2012-04-17 01:19:15 PST
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 From 2012-04-17 01:49:23 PST -------
Created an attachment (id=137488) [details]
Patch

This patch introduces new C API for ports using soup. It will be tested by GTK+ unit tests.
------- Comment #2 From 2012-04-17 01:53:21 PST -------
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 From 2012-04-17 01:54:44 PST -------
(In reply to comment #2)
> Attachment 137488 [details] [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 From 2012-04-17 01:56:14 PST -------
(From update of attachment 137488 [details])
Attachment 137488 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12418519
------- Comment #5 From 2012-04-17 02:27:03 PST -------
Created an attachment (id=137501) [details]
Updated patch to fix the build

I forgot to git add the .messages.in files.
------- Comment #6 From 2012-04-17 02:31:23 PST -------
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 From 2012-04-17 02:35:33 PST -------
(From update of attachment 137501 [details])
Attachment 137501 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12423174
------- Comment #8 From 2012-04-17 02:40:31 PST -------
Created an attachment (id=137504) [details]
Another build fix

There was another file missing in the patch, sorry.
------- Comment #9 From 2012-04-17 02:44:10 PST -------
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 From 2012-04-17 05:59:02 PST -------
(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 From 2012-04-19 15:30:17 PST -------
(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.

>> 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 From 2012-04-19 23:20:22 PST -------
(In reply to comment #11)
> (From update of attachment 137504 [details] [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 From 2012-04-19 23:25:51 PST -------
(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 From 2012-04-19 23:32:29 PST -------
(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 From 2012-04-20 01:02:37 PST -------
(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 From 2012-04-20 01:17:41 PST -------
Created an attachment (id=138059) [details]
Updated patch according to review comments
------- Comment #17 From 2012-04-27 00:13:04 PST -------
Committed r115411: <http://trac.webkit.org/changeset/115411>