Bug 96768 - [GTK] Don't use the C API internally in WebKitWebContext
Summary: [GTK] Don't use the C API internally in WebKitWebContext
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
Depends on: 96767
Blocks: 96766 96769
  Show dependency treegraph
 
Reported: 2012-09-14 06:38 PDT by Carlos Garcia Campos
Modified: 2012-10-08 01:05 PDT (History)
4 users (show)

See Also:


Attachments
Patch (19.01 KB, patch)
2012-09-14 06:40 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch to apply on current git master (21.44 KB, patch)
2012-10-01 08:39 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-09-14 06:38:27 PDT
Use the C++ classes instead.
Comment 1 Carlos Garcia Campos 2012-09-14 06:40:30 PDT
Created attachment 164128 [details]
Patch
Comment 2 Carlos Garcia Campos 2012-10-01 08:39:39 PDT
Created attachment 166480 [details]
Updated patch to apply on current git master

Some new patches broke this one.
Comment 3 WebKit Review Bot 2012-10-01 08:41:07 PDT
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 4 WebKit Review Bot 2012-10-01 08:41:25 PDT
Attachment 166480 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:23:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Mario Sanchez Prada 2012-10-03 01:26:34 PDT
Comment on attachment 166480 [details]
Updated patch to apply on current git master

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

This patch looks good to me, in the context of the meta bug 9766.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:23
>> +#include "WebResourceCacheManagerProxy.h"
> 
> Alphabetical sorting problem.  [build/include_order] [4]

This doesn't look like a false positive.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:167
> +    priv->geolocationProvider = WebKitGeolocationProvider::create(toAPI(priv->context->geolocationManagerProxy()));

I was about to comment on this, but I see you addressed the geolocation specific changes in bug 96778.
Comment 6 Carlos Garcia Campos 2012-10-03 01:32:56 PDT
(In reply to comment #5)
> (From update of attachment 166480 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166480&action=review
> 
> This patch looks good to me, in the context of the meta bug 9766.
> 
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:23
> >> +#include "WebResourceCacheManagerProxy.h"
> > 
> > Alphabetical sorting problem.  [build/include_order] [4]
> 
> This doesn't look like a false positive.

Yes, I will fix this before landing.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:167
> > +    priv->geolocationProvider = WebKitGeolocationProvider::create(toAPI(priv->context->geolocationManagerProxy()));
> 
> I was about to comment on this, but I see you addressed the geolocation specific changes in bug 96778.

Yes, this bug is only about WebKitWebContext
Comment 7 Martin Robinson 2012-10-05 11:01:57 PDT
Comment on attachment 166480 [details]
Updated patch to apply on current git master

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

Looks good, but seems to violate one of the rules of the style guide. I don't see this as a huge issue, but it would be nice to follow the rule if it isn't too hard.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContextPrivate.h:36
> +using namespace WebKit;

The style guide doesn't seem to like namespace declarations in the global scope in headers, since it leaks into implementation files: http://www.webkit.org/coding/coding-style.html (number 1 for "using" Statements).

There are a couple things you can do to make your life easier here though:

1. Put the private methods in the WebKit namespace.
2. Use WebKit:: in the header and put using WebKit at the top of the files that include this file.
Comment 8 Carlos Garcia Campos 2012-10-06 00:36:01 PDT
(In reply to comment #7)
> (From update of attachment 166480 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166480&action=review
> 
> Looks good, but seems to violate one of the rules of the style guide. I don't see this as a huge issue, but it would be nice to follow the rule if it isn't too hard.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContextPrivate.h:36
> > +using namespace WebKit;
> 
> The style guide doesn't seem to like namespace declarations in the global scope in headers, since it leaks into implementation files: http://www.webkit.org/coding/coding-style.html (number 1 for "using" Statements).

I think this is a special case because it's not a C++ header, but a private header to add prototypes of some internal functions. 

> There are a couple things you can do to make your life easier here though:
> 
> 1. Put the private methods in the WebKit namespace.
> 2. Use WebKit:: in the header and put using WebKit at the top of the files that include this file.

Since we are already doing it, and all C API migration patches do it too, do you mind if I fix it all together afterwards?
Comment 9 Martin Robinson 2012-10-06 23:42:58 PDT
(In reply to comment #8)

> Since we are already doing it, and all C API migration patches do it too, do you mind if I fix it all together afterwards?

Okay. I think it's probably okay in this case, especially since the style bot didn't complain. We should probably fix it later though.
Comment 10 Martin Robinson 2012-10-06 23:43:28 PDT
Comment on attachment 166480 [details]
Updated patch to apply on current git master

Please fix the remaining style error before landing. :)
Comment 11 Carlos Garcia Campos 2012-10-08 01:05:27 PDT
Committed r130621: <http://trac.webkit.org/changeset/130621>