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

See Also:


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

New patch in trunk broke this one
Comment 3 WebKit Review Bot 2012-10-01 08:46:35 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 Mario Sanchez Prada 2012-10-03 01:31:27 PDT
Comment on attachment 166481 [details]
Updated patch to apply on current git master

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

This patch is pretty straightforward and I have not much to comment here. LGTM

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:58
> +    WebBackForwardList* webList;

I would probably name it backForwardList instead of webList. But I'm not genius choosing names either, so take it as a subjective opinion.

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:140
> +WebKitBackForwardList* webkitBackForwardListCreate(WebBackForwardList* webList)

Ditto

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:32
> +    RefPtr<WebBackForwardListItem> webListItem;

And here too. Or maybe just listItem here, not to make it too long
Comment 5 Carlos Garcia Campos 2012-10-03 01:35:44 PDT
(In reply to comment #4)
> (From update of attachment 166481 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166481&action=review
> 
> This patch is pretty straightforward and I have not much to comment here. LGTM
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:58
> > +    WebBackForwardList* webList;
> 
> I would probably name it backForwardList instead of webList. But I'm not genius choosing names either, so take it as a subjective opinion.

This was wkList before, I think, I renamed to webList to avoid confusion with C API variables.
Comment 6 Gustavo Noronha (kov) 2012-10-17 05:33:21 PDT
Comment on attachment 166481 [details]
Updated patch to apply on current git master

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

>>> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:58
>>> +    WebBackForwardList* webList;
>> 
>> I would probably name it backForwardList instead of webList. But I'm not genius choosing names either, so take it as a subjective opinion.
> 
> This was wkList before, I think, I renamed to webList to avoid confusion with C API variables.

How about naming it items or backForwardItems? backForwardList is a bad name I think because it's the name of the object that holds this private structure in most cases. This is the list of backForwardItems.
Comment 7 Carlos Garcia Campos 2012-10-17 08:46:25 PDT
(In reply to comment #6)
> (From update of attachment 166481 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166481&action=review
> 
> >>> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:58
> >>> +    WebBackForwardList* webList;
> >> 
> >> I would probably name it backForwardList instead of webList. But I'm not genius choosing names either, so take it as a subjective opinion.
> > 
> > This was wkList before, I think, I renamed to webList to avoid confusion with C API variables.
> 
> How about naming it items or backForwardItems? backForwardList is a bad name I think because it's the name of the object that holds this private structure in most cases. This is the list of backForwardItems.

Sure!
Comment 8 Carlos Garcia Campos 2012-10-17 08:48:00 PDT
Committed r131605: <http://trac.webkit.org/changeset/131605>