Bug 130252 – Fix WebCore unused parameter warnings for WebKitGTK+ CMake build

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

These unused parameter warnings are making it difficult to spot important warnings. We've put off fixing them for years, so we should fix them now.

[Martin Robinson]

Created attachment 226738 [details]
Patch

[WebKit Commit Bot]

Attachment 226738 [details] did not pass style-queue:


ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:134:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:135:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:148:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:168:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:169:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:170:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:700:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:837:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

(In reply to comment #2)
> Attachment 226738 [details] did not pass style-queue:

gtk2xtbin.c has never followed WebKit coding style, so these are false positives.

[Carlos Garcia Campos]

Comment on attachment 226738 [details]
Patch

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

Thanks!

> Source/WebCore/platform/gtk/GtkVersioning.c:91
> +    UNUSED_PARAM(srcX);
> +

Have you checked this isn't a bug? sounds weird we are not using srcX, but I haven't looked at the code.

> Source/WebCore/platform/soup/SharedBufferSoup.cpp:44
> -void SharedBuffer::tryReplaceContentsWithPlatformBuffer(SharedBuffer* newContents)
> +void SharedBuffer::tryReplaceContentsWithPlatformBuffer(SharedBuffer* /* newContents */)

Maybe we can simply omit the parameter in this case.

> Source/WebCore/plugins/gtk/PluginViewGtk.cpp:500
> -void PluginView::plugAddedCallback(GtkSocket* socket, PluginView* view)
> +void PluginView::plugAddedCallback(GtkSocket*, PluginView* view)
>  {
>      ASSERT(socket);

This is going to fail in debug builds. I think we can remove the assert, the callback will never be emitted with a NULL widget.

> Source/WebCore/plugins/gtk/gtk2xtbin.c:135
> +  UNUSED_PARAM(source_data);
> +  UNUSED_PARAM(timeout);

We can't omit the names here because this file is C?

> Source/WebCore/rendering/InlineTextBox.cpp:991
> +#if !ENABLE(CSS3_TEXT_DECORATION_SKIP_INK)
> +    UNUSED_PARAM(textPainter);
> +#endif

Why not adding an #else to the existing ifdef? because there's more than one in the function body?

[Martin Robinson]

(In reply to comment #4)
> (From update of attachment 226738 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226738&action=review
> 
> Thanks!
> 
> > Source/WebCore/platform/gtk/GtkVersioning.c:91
> > +    UNUSED_PARAM(srcX);
> > +
> 
> Have you checked this isn't a bug? sounds weird we are not using srcX, but I haven't looked at the code.

It was pretty suspicious to me too, but I haven't had a chance to look into it. I've uncovered at least two serious problems already fixing these issues!

> 
> > Source/WebCore/platform/soup/SharedBufferSoup.cpp:44
> > -void SharedBuffer::tryReplaceContentsWithPlatformBuffer(SharedBuffer* newContents)
> > +void SharedBuffer::tryReplaceContentsWithPlatformBuffer(SharedBuffer* /* newContents */)
> 
> Maybe we can simply omit the parameter in this case.

Sure.
 
> > Source/WebCore/plugins/gtk/PluginViewGtk.cpp:500
> > -void PluginView::plugAddedCallback(GtkSocket* socket, PluginView* view)
> > +void PluginView::plugAddedCallback(GtkSocket*, PluginView* view)
> >  {
> >      ASSERT(socket);
> 
> This is going to fail in debug builds. I think we can remove the assert, the callback will never be emitted with a NULL widget.

Nice catch! We should probably replace it with ASSERT_UNUSED.

> > Source/WebCore/plugins/gtk/gtk2xtbin.c:135
> > +  UNUSED_PARAM(source_data);
> > +  UNUSED_PARAM(timeout);
> 
> We can't omit the names here because this file is C?

That's right.

> > Source/WebCore/rendering/InlineTextBox.cpp:991
> > +#if !ENABLE(CSS3_TEXT_DECORATION_SKIP_INK)
> > +    UNUSED_PARAM(textPainter);
> > +#endif
> 
> Why not adding an #else to the existing ifdef? because there's more than one in the function body?

Yes. I think it makes sense to have the UNUSED_PARAM at the start of the function too. Having it in a predictable location will likely be better for visibility.

[Martin Robinson]

Committed r165688: <http://trac.webkit.org/changeset/165688>