Summary: | Fix reported build warnings for GTK | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tibor Mészáros <mtiborinf> | ||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cgarcia, commit-queue, dbates, gustavo, mrobinson | ||||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Tibor Mészáros
2013-10-29 03:18:39 PDT
Created attachment 215916 [details]
patch
There were some build warnings, when doing a clean build.
- removed unused/deprecated methods or variables
- fixed documentation for methods in WebKitDomCustom.h
- fixed the CodeGeneratorGObject.pm for correct documentation generation for the methods, where the type is "void"
Comment on attachment 215916 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=215916&action=review > Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp:-70 > -gchar* webkit_dom_html_element_get_id(WebKitDOMHTMLElement* element) > -{ > - g_warning("The get_id method on WebKitDOMHTMLElement is deprecated. Use the one in WebKitDOMElement instead."); > - return webkit_dom_element_get_id(WEBKIT_DOM_ELEMENT(element)); I think removing these methods will cause an ABI break, which is why they were added in the first place. > Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:77 > +/*gboolean webkit_dom_event_target_add_event_listener_with_closure(WebKitDOMEventTarget* target, const char* eventName, GClosure* handler, gboolean useCapture) Why is this commented out? > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1618 > - gdk_threads_leave(); > g_main_loop_run(webView->priv->modalLoop.get()); > - gdk_threads_enter(); > } I'm not sure if it's right to remove this here. I think we need to keep it to avoid breaking applications (even though the functions are deprecated). > Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:-103 > - gdk_threads_leave(); > g_main_loop_run(m_runLoop.get()); > - gdk_threads_enter(); > - Ditto. (In reply to comment #2) > (From update of attachment 215916 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215916&action=review > > > Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp:-70 > > -gchar* webkit_dom_html_element_get_id(WebKitDOMHTMLElement* element) > > -{ > > - g_warning("The get_id method on WebKitDOMHTMLElement is deprecated. Use the one in WebKitDOMElement instead."); > > - return webkit_dom_element_get_id(WEBKIT_DOM_ELEMENT(element)); > > I think removing these methods will cause an ABI break, which is why they were added in the first place. Right. > > Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:77 > > +/*gboolean webkit_dom_event_target_add_event_listener_with_closure(WebKitDOMEventTarget* target, const char* eventName, GClosure* handler, gboolean useCapture) > > Why is this commented out? > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1618 > > - gdk_threads_leave(); > > g_main_loop_run(webView->priv->modalLoop.get()); > > - gdk_threads_enter(); > > } > > I'm not sure if it's right to remove this here. I think we need to keep it to avoid breaking applications (even though the functions are deprecated). Right too, see https://bugs.webkit.org/show_bug.cgi?id=120070#c11 > > Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:-103 > > - gdk_threads_leave(); > > g_main_loop_run(m_runLoop.get()); > > - gdk_threads_enter(); > > - > > Ditto. Comment on attachment 215916 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=215916&action=review Thanks for the patch. Please make sure the patch applies to current trunk. Since the warnings fixed a quite different, I think it would be better to split the patch and open a bug report for every patch. One for compile warnings, since the leveldb is not specific to GTK, another one for documentation warnings and another one for deprecated api used (if appropriate, if the only deprecated api used is gdk_threads_enter/leave see bug #120070). > Source/ThirdParty/leveldb/table/table.cc:-231 > - Slice handle = iiter->value(); leveldb code is not specific to GTK+. Since this is in third_party maybe we should update leveldb to a newer version if warnings are fixed or fix the warnings upstream. > Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp:-77 > -void webkit_dom_html_element_set_id(WebKitDOMHTMLElement* element, const gchar* value) > -{ > - g_warning("The set_id method on WebKitDOMHTMLElement is deprecated. Use the one in WebKitDOMElement instead."); > - webkit_dom_element_set_id(WEBKIT_DOM_ELEMENT(element), value); > -} What compile warnings are causing these methods? > Source/WebCore/bindings/gobject/WebKitDOMCustom.h:32 > - * Returns: > + * Returns: (transfer none): This is a workaround, gtk-doc doesn't complain because there's something after the Returns:, but the annotation doesn't make sense for a method returning a gboolean. This could be fixed adding something like Returns: a #gboolean I think this was already fixed in r158200 in any case. > Source/WebCore/bindings/gobject/WebKitDOMCustom.h:40 > - * Returns: > + * Returns: (transfer none): Ditto. > Source/WebCore/bindings/gobject/WebKitDOMCustom.h:59 > - * Returns: > + * Returns: (transfer none): Ditto. > Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:-115 > -WEBKIT_API gboolean webkit_dom_event_target_add_event_listener_with_closure(WebKitDOMEventTarget *target, > - const char *event_name, > - GClosure *handler, > - gboolean use_capture); This is valid API, why are you removing it? > Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:-134 > -WEBKIT_API gboolean webkit_dom_event_target_remove_event_listener_with_closure(WebKitDOMEventTarget *target, > - const char *event_name, > - GClosure *handler, > - gboolean use_capture); Ditto. > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:960 > - if (IsGDOMClassType($function->signature->type)) { > + if (IsGDOMClassType($function->signature->type) && $returnType ne "void") { I'm pretty sure this has already been fixed too in r158200 Created attachment 216668 [details]
Patch v2
Removed everything that mentioned in previous comments, so it is became a small documentation warning fix.
Comment on attachment 216668 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=216668&action=review Thanks for the patch, there are a couple of problems with the ChangeLog. > Source/WebCore/ChangeLog:3 > + There was a void method, that has return value in it's documentation, so I removed it. This line should contain the bug title, add your explanation below, after the Reviewed by line. > Source/WebCore/ChangeLog:8 > + No new tests (OOPS!). You should remove this line about the new tests. Created attachment 216670 [details]
Patch v3
Fixed the changelog.
Comment on attachment 216670 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=216670&action=review You can use Tools/Scripts/prepare-ChangeLog script instead of manually creating the changelog entry to make sure it's correct. > Source/WebCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=123439#c4 Remove the #c4. > Source/WebCore/ChangeLog:6 > + There was a void method, that has return value in it's documentation, so I removed it. I said *after* the Reviewed by line, not before. Sorry to be so nitpicking but I'm not sure commit-queue will be able to land the patch if the changelog is not correctly formatted. > Source/WebCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). I already r+'ed the patch, you can fill this with my name and upload a new version of the patch asking only commit-queue? Created attachment 216678 [details]
Patch v4
Fixed the changelog. Carlos Garcia Campos, thanks for your help and patient.
Comment on attachment 216678 [details]
Patch v4
Thanks.
Comment on attachment 216678 [details] Patch v4 Clearing flags on attachment: 216678 Committed r159119: <http://trac.webkit.org/changeset/159119> All reviewed patches have been landed. Closing bug. |