Bug 45395 - [GTK] DOM bindings do not have gir annotations
Summary: [GTK] DOM bindings do not have gir annotations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
: 63772 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-09-08 09:25 PDT by Gabriel Jacobo
Modified: 2011-07-12 12:42 PDT (History)
5 users (show)

See Also:


Attachments
gobject introspection enhancements (40.00 KB, patch)
2010-09-08 09:25 PDT, Gabriel Jacobo
xan.lopez: review-
Details | Formatted Diff | Diff
extracted patch for issue 3 (1.29 KB, patch)
2011-07-11 14:08 PDT, arno.
xan.lopez: review-
Details | Formatted Diff | Diff
patch for issue 3 (2.17 KB, patch)
2011-07-12 01:21 PDT, arno.
xan.lopez: review-
Details | Formatted Diff | Diff
patch for issue 3 (26.62 KB, patch)
2011-07-12 06:55 PDT, arno.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabriel Jacobo 2010-09-08 09:25:04 PDT
Created attachment 66904 [details]
gobject introspection enhancements

This is a patch for the GObject bindings script so it generates gobject introspection annotations the g-ir-scanner tool properly understand. There also a bunch of fixes for the existing annotations to make them comply with the recent changes in gobject-introspection trunk.
There's a workaround added for some functions names in the CodeGeneratorGObject.pm script, that's in my opinion the simplest of several possible fixes to a naming convention problem between what this script generates and what the scanner expects.

The problem arises in functions belonging to classes like WebKitDOMTHMLElement, where the scanner logic that matches methods to classes expects the method to be called webkit_domhtml_element_something while we output them as webkit_dom_html_element_something. The workaround I implemented is to output the function as webkit_domhtml_element_something where appropriate and set up a #define for each of these functions as to preserve compatibility with the rest of the codebase.

With this patch installed, after the build is done I rerun the following command under a jhbuild shell to check what's up with the scanner...

g-ir-scanner --warn-all --symbol-prefix=webkit -v --namespace WebKit --nsversion=3.0 \
             --include=GObject-2.0 \
             --include=Gtk-3.0 \
             --include=JSCore-3.0 \
             --include=Soup-2.4 \
             --library=webkitgtk-3.0 \
             --libtool="./doltlibtool" \
             --pkg gobject-2.0 \
             --pkg gtk+-3.0 \
             --pkg libsoup-2.4 \
             --output WebKit-3.0.gir \
             --add-include-path ./WebKit/gtk \
             --add-include-path . \
             -I./WebKit/gtk \
             -I./WebKit/gtk \
             -I./DerivedSources \
             -I./DerivedSources/webkit \
             -I./JavaScriptCore/ForwardingHeaders \
             -I./JavaScriptCore/API \
             -I. \
             WebKit/gtk/webkit/webkitversion.h DerivedSources/webkit/WebKitDOMCSSRule.h DerivedSources/webkit/WebKitDOMCSSRuleList.h DerivedSources/webkit/WebKitDOMCSSStyleDeclaration.h DerivedSources/webkit/WebKitDOMCSSStyleSheet.h DerivedSources/webkit/WebKitDOMCSSValue.h DerivedSources/webkit/WebKitDOMMediaList.h DerivedSources/webkit/WebKitDOMStyleMedia.h DerivedSources/webkit/WebKitDOMStyleSheet.h DerivedSources/webkit/WebKitDOMStyleSheetList.h DerivedSources/webkit/WebKitDOMAttr.h DerivedSources/webkit/WebKitDOMCDATASection.h DerivedSources/webkit/WebKitDOMCharacterData.h DerivedSources/webkit/WebKitDOMComment.h DerivedSources/webkit/WebKitDOMDocument.h DerivedSources/webkit/WebKitDOMDocumentFragment.h DerivedSources/webkit/WebKitDOMDocumentType.h DerivedSources/webkit/WebKitDOMDOMImplementation.h DerivedSources/webkit/WebKitDOMDOMStringList.h DerivedSources/webkit/WebKitDOMDOMStringMap.h DerivedSources/webkit/WebKitDOMElement.h DerivedSources/webkit/WebKitDOMEntityReference.h DerivedSources/webkit/WebKitDOMEvent.h DerivedSources/webkit/WebKitDOMMessagePort.h DerivedSources/webkit/WebKitDOMMouseEvent.h DerivedSources/webkit/WebKitDOMNamedNodeMap.h DerivedSources/webkit/WebKitDOMNode.h DerivedSources/webkit/WebKitDOMNodeFilter.h DerivedSources/webkit/WebKitDOMNodeIterator.h DerivedSources/webkit/WebKitDOMNodeList.h DerivedSources/webkit/WebKitDOMProcessingInstruction.h DerivedSources/webkit/WebKitDOMRange.h DerivedSources/webkit/WebKitDOMText.h DerivedSources/webkit/WebKitDOMTreeWalker.h DerivedSources/webkit/WebKitDOMUIEvent.h DerivedSources/webkit/WebKitDOMBlob.h DerivedSources/webkit/WebKitDOMFile.h DerivedSources/webkit/WebKitDOMFileList.h DerivedSources/webkit/WebKitDOMHTMLAnchorElement.h DerivedSources/webkit/WebKitDOMHTMLAppletElement.h DerivedSources/webkit/WebKitDOMHTMLAreaElement.h DerivedSources/webkit/WebKitDOMHTMLBaseElement.h DerivedSources/webkit/WebKitDOMHTMLBaseFontElement.h DerivedSources/webkit/WebKitDOMHTMLBlockquoteElement.h DerivedSources/webkit/WebKitDOMHTMLBodyElement.h DerivedSources/webkit/WebKitDOMHTMLBRElement.h DerivedSources/webkit/WebKitDOMHTMLButtonElement.h DerivedSources/webkit/WebKitDOMHTMLCanvasElement.h DerivedSources/webkit/WebKitDOMHTMLCollection.h DerivedSources/webkit/WebKitDOMHTMLDirectoryElement.h DerivedSources/webkit/WebKitDOMHTMLDivElement.h DerivedSources/webkit/WebKitDOMHTMLDListElement.h DerivedSources/webkit/WebKitDOMHTMLDocument.h DerivedSources/webkit/WebKitDOMHTMLElement.h DerivedSources/webkit/WebKitDOMHTMLEmbedElement.h DerivedSources/webkit/WebKitDOMHTMLFieldSetElement.h DerivedSources/webkit/WebKitDOMHTMLFontElement.h DerivedSources/webkit/WebKitDOMHTMLFormElement.h DerivedSources/webkit/WebKitDOMHTMLFrameElement.h DerivedSources/webkit/WebKitDOMHTMLFrameSetElement.h DerivedSources/webkit/WebKitDOMHTMLHeadElement.h DerivedSources/webkit/WebKitDOMHTMLHeadingElement.h DerivedSources/webkit/WebKitDOMHTMLHRElement.h DerivedSources/webkit/WebKitDOMHTMLHtmlElement.h DerivedSources/webkit/WebKitDOMHTMLIFrameElement.h DerivedSources/webkit/WebKitDOMHTMLImageElement.h DerivedSources/webkit/WebKitDOMHTMLInputElement.h DerivedSources/webkit/WebKitDOMHTMLIsIndexElement.h DerivedSources/webkit/WebKitDOMHTMLLabelElement.h DerivedSources/webkit/WebKitDOMHTMLLegendElement.h DerivedSources/webkit/WebKitDOMHTMLLIElement.h DerivedSources/webkit/WebKitDOMHTMLLinkElement.h DerivedSources/webkit/WebKitDOMHTMLMapElement.h DerivedSources/webkit/WebKitDOMHTMLMarqueeElement.h DerivedSources/webkit/WebKitDOMHTMLMediaElement.h DerivedSources/webkit/WebKitDOMHTMLMenuElement.h DerivedSources/webkit/WebKitDOMHTMLMetaElement.h DerivedSources/webkit/WebKitDOMHTMLModElement.h DerivedSources/webkit/WebKitDOMHTMLObjectElement.h DerivedSources/webkit/WebKitDOMHTMLOListElement.h DerivedSources/webkit/WebKitDOMHTMLOptGroupElement.h DerivedSources/webkit/WebKitDOMHTMLOptionElement.h DerivedSources/webkit/WebKitDOMHTMLOptionsCollection.h DerivedSources/webkit/WebKitDOMHTMLParagraphElement.h DerivedSources/webkit/WebKitDOMHTMLParamElement.h DerivedSources/webkit/WebKitDOMHTMLPreElement.h DerivedSources/webkit/WebKitDOMHTMLQuoteElement.h DerivedSources/webkit/WebKitDOMHTMLScriptElement.h DerivedSources/webkit/WebKitDOMHTMLSelectElement.h DerivedSources/webkit/WebKitDOMHTMLStyleElement.h DerivedSources/webkit/WebKitDOMHTMLTableElement.h DerivedSources/webkit/WebKitDOMHTMLTableCaptionElement.h DerivedSources/webkit/WebKitDOMHTMLTableColElement.h DerivedSources/webkit/WebKitDOMHTMLTableSectionElement.h DerivedSources/webkit/WebKitDOMHTMLTableCellElement.h DerivedSources/webkit/WebKitDOMHTMLTextAreaElement.h DerivedSources/webkit/WebKitDOMHTMLTitleElement.h DerivedSources/webkit/WebKitDOMHTMLTableRowElement.h DerivedSources/webkit/WebKitDOMHTMLUListElement.h DerivedSources/webkit/WebKitDOMMediaError.h DerivedSources/webkit/WebKitDOMTimeRanges.h DerivedSources/webkit/WebKitDOMValidityState.h DerivedSources/webkit/WebKitDOMDOMApplicationCache.h DerivedSources/webkit/WebKitDOMBarInfo.h DerivedSources/webkit/WebKitDOMConsole.h DerivedSources/webkit/WebKitDOMDOMWindow.h DerivedSources/webkit/WebKitDOMDOMSelection.h DerivedSources/webkit/WebKitDOMEventTarget.h DerivedSources/webkit/WebKitDOMHistory.h DerivedSources/webkit/WebKitDOMLocation.h DerivedSources/webkit/WebKitDOMMemoryInfo.h DerivedSources/webkit/WebKitDOMObject.h DerivedSources/webkit/WebKitDOMNavigator.h DerivedSources/webkit/WebKitDOMScreen.h DerivedSources/webkit/WebKitDOMWebKitPoint.h DerivedSources/webkit/WebKitDOMDOMMimeType.h DerivedSources/webkit/WebKitDOMDOMMimeTypeArray.h DerivedSources/webkit/WebKitDOMDOMPlugin.h DerivedSources/webkit/WebKitDOMDOMPluginArray.h DerivedSources/webkit/WebKitDOMDatabase.h DerivedSources/webkit/WebKitDOMStorage.h DerivedSources/webkit/WebKitDOMXPathExpression.h DerivedSources/webkit/WebKitDOMXPathNSResolver.h DerivedSources/webkit/WebKitDOMXPathResult.h DerivedSources/webkit/webkitdom.h DerivedSources/webkit/webkitdomdefines.h  ./DerivedSources/webkit/WebKitDOMHTMLAudioElement.h ./WebKit/gtk/webkit/webkit.h ./WebKit/gtk/webkit/webkitdefines.h ./WebKit/gtk/webkit/webkitdownload.h ./WebKit/gtk/webkit/webkiterror.h ./WebKit/gtk/webkit/webkithittestresult.h ./WebKit/gtk/webkit/webkitnetworkrequest.h ./WebKit/gtk/webkit/webkitnetworkresponse.h ./WebKit/gtk/webkit/webkitsoupauthdialog.h ./WebKit/gtk/webkit/webkitwebbackforwardlist.h ./WebKit/gtk/webkit/webkitwebdatasource.h ./WebKit/gtk/webkit/webkitwebframe.h ./WebKit/gtk/webkit/webkitwebhistoryitem.h ./WebKit/gtk/webkit/webkitwebinspector.h ./WebKit/gtk/webkit/webkitwebnavigationaction.h ./WebKit/gtk/webkit/webkitwebpolicydecision.h ./WebKit/gtk/webkit/webkitgeolocationpolicydecision.h ./WebKit/gtk/webkit/webkitwebresource.h ./WebKit/gtk/webkit/webkitwebsettings.h ./WebKit/gtk/webkit/webkitwebwindowfeatures.h ./WebKit/gtk/webkit/webkitwebview.h ./WebKit/gtk/webkit/webkitwebdatabase.h ./WebKit/gtk/webkit/webkitsecurityorigin.h \
             ./WebKit/gtk/webkit/*.cpp


The output I get is:

WebKit/gtk/webkit/webkitwebview.h:402: Warning: WebKit: Unknown container Type(target_giname=WebKit.DOMDocument, ctype=WebKitDOMDocument*) for element-type annotation
WebKit/gtk/webkit/webkiterror.h:61: Warning: WebKit: webkit_network_error_quark: Couldn't find corresponding enumeration
WebKit/gtk/webkit/webkiterror.h:64: Warning: WebKit: webkit_policy_error_quark: Couldn't find corresponding enumeration
WebKit/gtk/webkit/webkiterror.h:67: Warning: WebKit: webkit_plugin_error_quark: Couldn't find corresponding enumeration
<unknown>:: Warning: WebKit: (GLibSignal)current-toplevel: return value: Missing (transfer) annotation
WebKit/gtk/webkit/webkitwebframe.h:146: Warning: WebKit: webkit_web_frame_get_global_context: return value: Unresolved type: 'JSGlobalContextRef'
<unknown>:: Warning: WebKit: (GLibSignal)inspect-web-view: return value: Missing (transfer) annotation
<unknown>:: Warning: WebKit: (GLibSignal)navigation-requested: return value: Unresolved type: 'WebKitNavigationResponse'
<unknown>:: Warning: WebKit: (Callback)current_toplevel: return value: Missing (transfer) annotation
<unknown>:: Warning: WebKit: (Callback)create_web_view: return value: Missing (transfer) annotation
<unknown>:: Warning: WebKit: (Callback)window_object_cleared: argument context: Unresolved type: 'JSGlobalContextRef'
<unknown>:: Warning: WebKit: (Callback)window_object_cleared: argument window_object: Unresolved type: 'JSObjectRef'


Also, verifying the WebKit gir file also helps to check everything is in working order. 
So, the most important pending item is fixing the JS unresolved types, I tried playing with JSCore.gir but didnt get any positive results. Also, i'm not sure if the default transfer mode should be none, it seemed the safest bet.
I tested this with a Python test app that uses pygobject, that you can check in my blog http://mdqinc.com/blog, and everything seems to be working fine.
Comment 1 Xan Lopez 2010-09-11 08:55:51 PDT
So, let's see, these are actually three patches, don't merge everything in one.

1- Fixes to make stuff work with a recent gobject-introspection.
2- Annotation fixes.
3- Changes to the DOM bindings to make them work with introspection.

For 1) I've opened a bug with a patch that has the stuff needed to make things compile in here: https://bugs.webkit.org/show_bug.cgi?id=45590. It's missing the include of the glib URL in the JSCore gir file (JSCore does not use glib, why do you need that?) and the --symbol-prefix stuff, which is not needed in trunk.

I'll comment on the other parts of the patch.
Comment 2 Gabriel Jacobo 2010-09-11 09:10:28 PDT
The --symbol-prefix=webkit is required, just take a look at the generated WebKit-3.0.gir file that you get without it, all the "by hand" functions of the interface generate a "Unknown namespace for symbol". 
Also, to check for other warnings, add --warn-all to the g-ir-scanner command to verify before removing stuff from the patch.
Comment 3 Xan Lopez 2010-09-11 09:12:27 PDT
Comment on attachment 66904 [details]
gobject introspection enhancements

> Index: GNUmakefile.am
> ===================================================================
> --- GNUmakefile.am	(revisión: 66973)
> +++ GNUmakefile.am	(copia de trabajo)
> @@ -397,7 +397,7 @@
>  WEBKIT_GIRSOURCES += WebKit-@WEBKITGTK_API_VERSION@.gir
>  
>  $(WEBKIT_GIRSOURCES): $(G_IR_SCANNER) $(JSCORE_GIRSOURCES) libwebkitgtk-@WEBKITGTK_API_MAJOR_VERSION@.@WEBKITGTK_API_MINOR_VERSION@.la
> -	$(AM_V_GEN)$(G_IR_SCANNER) -v --namespace WebKit --nsversion=@WEBKITGTK_API_VERSION@ \
> +	$(AM_V_GEN)$(G_IR_SCANNER) -v --symbol-prefix=webkit --namespace WebKit --nsversion=@WEBKITGTK_API_VERSION@ \

Is this needed on top of the --prefix parameter?

>  	     --include=GObject-2.0 \
>  	     --include=Gtk-@GTK_API_VERSION@ \
>  	     --include=JSCore-@WEBKITGTK_API_VERSION@ \



>   */
>  gchar* webkit_web_frame_get_inner_text(WebKitWebFrame* frame)
>  {
> @@ -790,7 +790,7 @@
>   * webkit_web_frame_dump_render_tree:
>   * @frame: a #WebKitWebFrame
>   *
> - * Return value: Non-recursive render tree dump of @frame
> + * Return value: (transfer none): Non-recursive render tree dump of @frame
>   */

This is not public API, and besides the annotation is wrong (?).

>  gchar* webkit_web_frame_dump_render_tree(WebKitWebFrame* frame)
>  {
> @@ -814,7 +814,7 @@
>   * @frame: a #WebKitWebFrame
>   * @id: an element ID string
>   *
> - * Return value: The counter value of element @id in @frame
> + * Return value: (transfer none): The counter value of element @id in @frame
>   */

Same thing...

>   */
>  int webkit_web_frame_number_of_pages(WebKitWebFrame* frame, float pageWidth, float pageHeight)
>  {
> @@ -877,7 +877,7 @@
>   * webkit_web_frame_get_pending_unload_event_count:
>   * @frame: a #WebKitWebFrame
>   *
> - * Return value: number of pending unload events
> + * Return value: (transfer none): number of pending unload events
>   */

Transfer annotation for int? This is not needed, same for booleans and any non pointer type.

>  }
>  
>  /**
> - * webkit_network_response_get_soup_message:
> + * webkit_network_response_get_message:

You are most definitely not allowed to change the function name. Not sure why are you doing that but you can't.

Please go again over all annotations, a lot of them are wrong.


> Index: WebCore/bindings/scripts/CodeGeneratorGObject.pm
> ===================================================================
> --- WebCore/bindings/scripts/CodeGeneratorGObject.pm	(revisión: 66983)
> +++ WebCore/bindings/scripts/CodeGeneratorGObject.pm	(copia de trabajo)

Wow, sorry but no, this is way too ugly :). If the scanner can't cope with our names as they are we'll have to teach it to, I won't accept this patch.

Besides, I thought we had decided to write the introspection data in a different file and include it at compile time? I don't think you can possibly get right all the stuff automatically?

r- for all the comments above.
Comment 4 Xan Lopez 2010-09-11 09:14:43 PDT
(In reply to comment #2)
> The --symbol-prefix=webkit is required, just take a look at the generated WebKit-3.0.gir file that you get without it, all the "by hand" functions of the interface generate a "Unknown namespace for symbol". 

I don't get this in trunk.
Comment 5 Xan Lopez 2010-09-11 09:21:32 PDT
Oh, another thing, WebKit has some rules about how to send patches and stuff (in particular, you need to add ChangeLogs, follow the style guides, ...). Read http://webkit.org/coding/contributing.html
Comment 6 Martin Robinson 2011-07-11 13:07:44 PDT
*** Bug 63772 has been marked as a duplicate of this bug. ***
Comment 7 arno. 2011-07-11 14:08:26 PDT
Created attachment 100360 [details]
extracted patch for issue 3

With attached patch, extracted and slighty modified from Gabriel Jacobo patch, I'm able to have introspection working, ie, fix issue 3 as described in comment 1.
I'm unsure about the transfer none though: Shouldn't that be transfer full instead ?
Comment 8 Xan Lopez 2011-07-11 15:08:59 PDT
Comment on attachment 100360 [details]
extracted patch for issue 3

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

This looks OK, but it needs a ChangeLog to get r+. Please read http://www.webkit.org/coding/contributing.html, especially the bit about generating a proper ChangeLog. Thanks for the patch!

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:791
> +    $selfClass =~ s/\*$//;

I think $className does not end in *, so this seems redundant.

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:795
> +        my $paramIDLType = $param->type;

Minor nit, probably you can do without this variable.

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:797
> +        $paramType =~ s/\*$//;        

Same than first comment.

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:806
> +        push(@hBody, " * Returns: (transfer none):\n");    

Answering to your question, this is OK. GDOM objects are GC by WebKit automatically, so there's no need to manually unref them. transfer: full would also be OK, since it's possibly to unref them manually too. So AFAIK this is mostly up to us. transfer none is probably safer, so I think we can start with that.
Comment 9 arno. 2011-07-12 01:21:23 PDT
Created attachment 100448 [details]
patch for issue 3

(In reply to comment #8)
> (From update of attachment 100360 [details])

> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:791
> > +    $selfClass =~ s/\*$//;
> 
> I think $className does not end in *, so this seems redundant.

fixed
 
> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:795
> > +        my $paramIDLType = $param->type;
> 
> Minor nit, probably you can do without this variable.

fixed

> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:797
> > +        $paramType =~ s/\*$//;        
> 
> Same than first comment.

fixed

> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:806
> > +        push(@hBody, " * Returns: (transfer none):\n");    
> 
> Answering to your question, this is OK. GDOM objects are GC by WebKit automatically, so there's no need to manually unref them. transfer: full would also be OK, since it's possibly to unref them manually too. So AFAIK this is mostly up to us. transfer none is probably safer, so I think we can start with that.

ok, thanks for your answer.
Here is a new patch with a changelog. I fell like I'm "hijacking" Gabriel's patch but hopefully, that will help getting annotations for dom methods.
Also, I 
push(@hBody, " * Returns:\n");
if return type is not dom class type.
Comment 10 Gabriel Jacobo 2011-07-12 05:01:07 PDT
Don't sweat it Arno, feel free to do with the patch as you wish as I lost interest in working on it further due to the poor attitude towards contributions from Xan in particular and the fact that someone deleted one of my comments where I suggested that perhaps taking 5 minutes to skim over a patch was no proper way of dealing with new contributors (as proven by the fact that he criticized the patch for fixing some annotations, and mistook that for doing changes in the API ¿?) . Anyway, hope you have better luck as there certainly is interest in the community for proper working bindings and introspection.
Comment 11 Xan Lopez 2011-07-12 05:14:58 PDT
(In reply to comment #10)
> Don't sweat it Arno, feel free to do with the patch as you wish as I lost interest in working on it further due to the poor attitude towards contributions from Xan in particular and the fact that someone deleted one of my comments where I suggested that perhaps taking 5 minutes to skim over a patch was no proper way of dealing with new contributors (as proven by the fact that he criticized the patch for fixing some annotations, and mistook that for doing changes in the API ¿?) . Anyway, hope you have better luck as there certainly is interest in the community for proper working bindings and introspection.

I don't think it's possible to delete comments here, so you are probably just confusing some error in sending your comment with a conspiracy against you. I most certainly didn't delete anything.

Anyone can make mistakes, but I think all my comments about your patch still stand. The vast majority of the annotations were wrong (it makes no sense to annotate ints or booleans), so I just stopped looking after finding a bunch of those and asked you to go over them again. Changing the scanner to emit wrong names in the gtk-doc is a hack, and I said so, I didn't confuse that with any change in the API.

In any case, as Arno has proved parts of your patch were useful, and I didn't say the contrary, so certainly your contributions were and still are welcome. It is my duty as reviewer to try to land the best patches possible, and you shouldn't confuse criticisms of code with personal attacks against you.
Comment 12 Xan Lopez 2011-07-12 05:24:21 PDT
Comment on attachment 100448 [details]
patch for issue 3

Looks good.
Comment 13 Gabriel Jacobo 2011-07-12 05:28:47 PDT
>>  }
>>  
>>  /**
>> - * webkit_network_response_get_soup_message:
>> + * webkit_network_response_get_message:

>You are most definitely not allowed to change the function name. Not sure why >are you doing that but you can't.


I was referring to that specifically as proof that you didn't do a proper review in my opinion (and I mentioned that in the lost comment). The fix quoted is a fix for the annotation that didn't match the corresponding function name (and not the other way around, I was not changing any API). 
So if you say that my comment wasn't deleted I believe you and I apologize for saying so, it certainly seemed strange to delete a comment over some minor bashing...but with the power trips some people go on over the internet, you never know.
Anyway, good luck Arno with the patch, feel free to contact me if you have questions (I've noted down part of the thinking behind this in some older posts in my blog at mdqinc.com)
Comment 14 Xan Lopez 2011-07-12 05:48:58 PDT
(In reply to comment #13)
> >>  }
> >>  
> >>  /**
> >> - * webkit_network_response_get_soup_message:
> >> + * webkit_network_response_get_message:
> 
> >You are most definitely not allowed to change the function name. Not sure why >are you doing that but you can't.
> 
> 
> I was referring to that specifically as proof that you didn't do a proper review in my opinion (and I mentioned that in the lost comment). The fix quoted is a fix for the annotation that didn't match the corresponding function name (and not the other way around, I was not changing any API). 

I guess since in other parts of the patch you were modifying the script to generate incorrect documentation I assumed this was the case too. It wasn't, so my mistake. Anyway this one mistake does not change the fact that most of the patch was not ready suitable to be landed, so I don't really agree that my first review was not proper. I made a mistake? Ok, but I also pointed out lots of incorrect things in your patch. The normal thing to do would be to tell me about the mistake and change the rest of the stuff, not to drop the ball completely IMHO.

Also this shows why it's a good idea to send one patch per separate issue instead of doing a giant patch with all your changes. Makes things harder to review.
Comment 15 WebKit Review Bot 2011-07-12 06:37:38 PDT
Comment on attachment 100448 [details]
patch for issue 3

Rejecting attachment 100448 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2

Last 500 characters of output:
ace.cpp...
No changes found.
Testing the CPP generator on TestMediaQueryListListener.idl
Detecting changes in WebDOMTestMediaQueryListListener.h...
No changes found.
Detecting changes in WebDOMTestMediaQueryListListener.cpp...
No changes found.
Testing the CPP generator on TestInterface.idl
Detecting changes in WebDOMTestInterface.cpp...
No changes found.
Detecting changes in WebDOMTestInterface.h...
No changes found.
(To update the reference files, execute "run-bindings-tests --reset-results")

Full output: http://queues.webkit.org/results/9021166
Comment 16 Xan Lopez 2011-07-12 06:46:19 PDT
Comment on attachment 100448 [details]
patch for issue 3

We also need to update the test results.
Comment 17 arno. 2011-07-12 06:55:43 PDT
Created attachment 100479 [details]
patch for issue 3
Comment 18 Xan Lopez 2011-07-12 09:55:47 PDT
Comment on attachment 100479 [details]
patch for issue 3

Well, now we can see that the class type indeed does not have an * at the end, but the param type does have it in some cases (basically when the type is not recognized in the GetGlibTypeName method), so I guess for the type name it does make sense to get rid of any trailing * in case they exist. What a mess :). Do you mind updating the patch with this?
Comment 19 Xan Lopez 2011-07-12 12:41:59 PDT
Comment on attachment 100479 [details]
patch for issue 3

Just pushed this with some changes as r90841.
Comment 20 Xan Lopez 2011-07-12 12:42:10 PDT
Closing.