WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
45395
[GTK] DOM bindings do not have gir annotations
https://bugs.webkit.org/show_bug.cgi?id=45395
Summary
[GTK] DOM bindings do not have gir annotations
Gabriel Jacobo
Reported
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.
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Xan Lopez
Comment 1
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.
Gabriel Jacobo
Comment 2
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.
Xan Lopez
Comment 3
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.
Xan Lopez
Comment 4
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.
Xan Lopez
Comment 5
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
Martin Robinson
Comment 6
2011-07-11 13:07:44 PDT
***
Bug 63772
has been marked as a duplicate of this bug. ***
arno.
Comment 7
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 ?
Xan Lopez
Comment 8
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.
arno.
Comment 9
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.
Gabriel Jacobo
Comment 10
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.
Xan Lopez
Comment 11
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.
Xan Lopez
Comment 12
2011-07-12 05:24:21 PDT
Comment on
attachment 100448
[details]
patch for issue 3 Looks good.
Gabriel Jacobo
Comment 13
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)
Xan Lopez
Comment 14
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.
WebKit Review Bot
Comment 15
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
Xan Lopez
Comment 16
2011-07-12 06:46:19 PDT
Comment on
attachment 100448
[details]
patch for issue 3 We also need to update the test results.
arno.
Comment 17
2011-07-12 06:55:43 PDT
Created
attachment 100479
[details]
patch for issue 3
Xan Lopez
Comment 18
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?
Xan Lopez
Comment 19
2011-07-12 12:41:59 PDT
Comment on
attachment 100479
[details]
patch for issue 3 Just pushed this with some changes as
r90841
.
Xan Lopez
Comment 20
2011-07-12 12:42:10 PDT
Closing.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug