Bug 123439 – Fix reported build warnings for GTK

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Tibor Mészáros]

When doing a clean build on GTK there are build warnings (like unused parameter). We should fix these if there is any (there should be)

[Tibor Mészáros]

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"

[Martin Robinson]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

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

[Tibor Mészáros]

Created attachment 216668 [details]
Patch v2

Removed everything that mentioned in previous comments, so it is became a small documentation warning fix.

[Carlos Garcia Campos]

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.

[Tibor Mészáros]

Created attachment 216670 [details]
Patch v3

Fixed the changelog.

[Carlos Garcia Campos]

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?

[Tibor Mészáros]

Created attachment 216678 [details]
Patch v4

Fixed the changelog. Carlos Garcia Campos, thanks for your help and patient.

[Carlos Garcia Campos]

Comment on attachment 216678 [details]
Patch v4

Thanks.

[WebKit Commit Bot]

Comment on attachment 216678 [details]
Patch v4

Clearing flags on attachment: 216678

Committed r159119: <http://trac.webkit.org/changeset/159119>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.