Bug 96742 – Fix unused parameter compile warnings in WebKit/WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Jinwoo Song]

Fix remained unused parameter warnings during EFL build. Patch is coming.

[Jinwoo Song]

Created attachment 164274 [details]
patch

[Jinwoo Song]

Created attachment 164278 [details]
patch

[Chris Dumez]

I think it would be better to use the UNUSED_PARAM() macro instead of removing the argument names. We loose useful information with this patch.

[Gyuyoung Kim]

(In reply to comment #3)
> I think it would be better to use the UNUSED_PARAM() macro instead of removing the argument names. We loose useful information with this patch.

I also agree to use UNUSED_PARAM(), but, I think we don't even need to use it for unimplemented functions.

[Jinwoo Song]

Created attachment 164370 [details]
patch.

I used the UNUSED_PARAM() macro for not used arguments 
and removed the arguments which are meaningless as the parameter type name contains the parameter name.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit2/WebProcess/WebPage/DrawingArea.h:113:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 1 in 64 files


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

[Gyuyoung Kim]

Comment on attachment 164370 [details]
patch.

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

> Source/WebKit/efl/ChangeLog:3
> +        [EFL] Fix unused parameter compile warnings in WebKit/WebKit2

You have to remove [EFL] prefix because this patch touches common files as well as efl files.

[Jinwoo Song]

Created attachment 164374 [details]
patch.

Changed the title and fixed the style error.

[Gyuyoung Kim]

Comment on attachment 164374 [details]
patch.

Looks fine on EFL port side.

[Gyuyoung Kim]

Comment on attachment 164374 [details]
patch.

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

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:159
> +    UNUSED_PARAM(frameLoadRequest);

I missed something stuff. I think it is better to only use UNUSED_PARAM in functions inside which has #if ENABLE(). Because, excessive UNUSED_PARAM usage makes noise in code, IMO. r128570 also did same.

> Source/WebKit/efl/WebCoreSupport/FullscreenVideoControllerEfl.cpp:63
> +    UNUSED_PARAM(autoHide);

Just remove parameter in unimplemented function.

[Jinwoo Song]

(In reply to comment #10)
> (From update of attachment 164374 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164374&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:159
> > +    UNUSED_PARAM(frameLoadRequest);
> 
> I missed something stuff. I think it is better to only use UNUSED_PARAM in functions inside which has #if ENABLE(). Because, excessive UNUSED_PARAM usage makes noise in code, IMO. r128570 also did same.
Actually, my second patch is exactly the one you explain. Do you want to revert the patch to second one?
> 
> > Source/WebKit/efl/WebCoreSupport/FullscreenVideoControllerEfl.cpp:63
> > +    UNUSED_PARAM(autoHide);
> 
> Just remove parameter in unimplemented function.

[Gyuyoung Kim]

(In reply to comment #11)
> Actually, my second patch is exactly the one you explain. Do you want to revert the patch to second one?

Yes, I prefer to use second patch.

[Jinwoo Song]

Created attachment 164489 [details]
patch

[Jinwoo Song]

(In reply to comment #12)
> (In reply to comment #11)
> > Actually, my second patch is exactly the one you explain. Do you want to revert the patch to second one?
> 
> Yes, I prefer to use second patch.
Done. :)

[Mikhail Pozdnyakov]

Comment on attachment 164489 [details]
patch

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

> Source/WebKit2/WebProcess/WebPage/DrawingArea.h:73
> +    virtual bool forceRepaintAsync(uint64_t) { return false; }

Is it now clear that forceRepaintAsync accepts callback?

> Source/WebKit2/WebProcess/WebPage/DrawingArea.h:113
> +    virtual void updateBackingStoreState(uint64_t, bool, float, const WebCore::IntSize&, const WebCore::IntSize&) { }

The method became very mysterious :) Now it's pretty hard to guess what its arguments mean.

> Source/WebKit2/WebProcess/WebPage/LayerTreeHost.h:76
> +    virtual bool forceRepaintAsync(uint64_t) { return false; }

Same here.. I think we should not sacrifice with code readability just in order to avoid compiler warnings

[Mikhail Pozdnyakov]

*** Bug 96901 has been marked as a duplicate of this bug. ***

[Gyuyoung Kim]

(In reply to comment #15)
> (From update of attachment 164489 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164489&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/DrawingArea.h:73
> > +    virtual bool forceRepaintAsync(uint64_t) { return false; }
> 
> Is it now clear that forceRepaintAsync accepts callback?
> 
> > Source/WebKit2/WebProcess/WebPage/DrawingArea.h:113
> > +    virtual void updateBackingStoreState(uint64_t, bool, float, const WebCore::IntSize&, const WebCore::IntSize&) { }
> 
> The method became very mysterious :) Now it's pretty hard to guess what its arguments mean.
> 
> > Source/WebKit2/WebProcess/WebPage/LayerTreeHost.h:76
> > +    virtual bool forceRepaintAsync(uint64_t) { return false; }
> 
> Same here.. I think we should not sacrifice with code readability just in order to avoid compiler warnings

The r128570 removed all unused parameters. However, I also worry about disturbing code readability again. Excessive UNUSED_PARAM disturbs code readability. On the other hands, to remove all parameters disturbs it as well. Why don't we use UNUSED_PARAM only for both #if ENABLE inside and primitive parameter ?

[Mikhail Pozdnyakov]

> The r128570 removed all unused parameters. However, I also worry about disturbing code readability again. Excessive UNUSED_PARAM disturbs code readability. On the other hands, to remove all parameters disturbs it as well. Why don't we use UNUSED_PARAM only for both #if ENABLE inside and primitive parameter ?

Think, we could keep variable name in /* */ (i.e. void doSomething(uint64_t /*trickyCallback*/ ); ) and also please take a look at bug96985.

[Gyuyoung Kim]

(In reply to comment #18)
> > The r128570 removed all unused parameters. However, I also worry about disturbing code readability again. Excessive UNUSED_PARAM disturbs code readability. On the other hands, to remove all parameters disturbs it as well. Why don't we use UNUSED_PARAM only for both #if ENABLE inside and primitive parameter ?
> 
> Think, we could keep variable name in /* */ (i.e. void doSomething(uint64_t /*trickyCallback*/ ); ) and also please take a look at bug96985.

Good suggestion. BTW, It looks /* */ is better for me.

[Jinwoo Song]

(In reply to comment #19)
> (In reply to comment #18)
> > > The r128570 removed all unused parameters. However, I also worry about disturbing code readability again. Excessive UNUSED_PARAM disturbs code readability. On the other hands, to remove all parameters disturbs it as well. Why don't we use UNUSED_PARAM only for both #if ENABLE inside and primitive parameter ?
> > 
> > Think, we could keep variable name in /* */ (i.e. void doSomething(uint64_t /*trickyCallback*/ ); ) and also please take a look at bug96985.
> 
> Good suggestion. BTW, It looks /* */ is better for me.
Thanks for good suggestion, Mikhail and Gyuyoung. I'll apply the /* */ for the primitive parameters.

[Jinwoo Song]

Created attachment 164539 [details]
patch

Keep the primitive variable names in /* */.

[Gyuyoung Kim]

Comment on attachment 164539 [details]
patch

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

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:396
> +void ChromeClientEfl::reachedMaxAppCacheSize(int64_t)

/* spaceNeeded */ ?

> Source/WebKit/efl/ewk/ewk_frame.cpp:1828
> +WTF::PassRefPtr<WebCore::Widget> ewk_frame_plugin_create(Evas_Object* ewkFrame, const WebCore::IntSize& pluginSize, WebCore::HTMLPlugInElement* element, const WebCore::KURL& url, const WTF::Vector<WTF::String>& paramName, const WTF::Vector<WTF::String>& paramValues, const WTF::String& mimeType, bool loadManually)

I think paramNames is proper for Vector.

> Source/WebKit/efl/ewk/ewk_view.cpp:620
> +static void _ewk_view_smart_add_console_message(Ewk_View_Smart_Data*, const char* message, unsigned lineNumber, const char* sourceID)

You should not change *unsigned int* to *unsigned* in this patch.

> Source/WebKit/efl/ewk/ewk_view.cpp:1040
> +static void _ewk_view_smart_move(Evas_Object* ewkView, Evas_Coord, Evas_Coord)

Need to use /* */ for x, y.

> Source/WebKit/efl/ewk/ewk_view.cpp:4481
> +bool ewk_view_accelerated_compositing_object_create(Evas_Object* /*ewkView*/, Evas_Native_Surface* /*nativeSurface*/, const WebCore::IntRect& /*rect*/)

Unneeded to wrap /* */ to ewkView and nativeSurface.

> Source/WebKit/efl/ewk/ewk_view.cpp:4487
> +WebCore::GraphicsContext3D* ewk_view_accelerated_compositing_context_get(Evas_Object* /*ewkView*/)

Unneeded /* */ because ewkView is clear in this file.

> Source/WebKit/efl/ewk/ewk_view_tiled.cpp:49
> +static void* _ewk_view_tiled_updates_process_pre(void* data, Evas_Object* /*ewkView*/)

ditto.

> Source/WebKit/efl/ewk/ewk_view_tiled.cpp:66
> +_ewk_view_tiled_contents_size_changed_cb(void* data, Evas_Object* /*ewkView*/, void* eventInfo)

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:292
> +static void _ewk_view_on_focus_out(void* data, Evas*, Evas_Object* /*ewkView*/, void* /*eventInfo*/)

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:300
> +static void _ewk_view_on_mouse_wheel(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:309
> +static void _ewk_view_on_mouse_down(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:318
> +static void _ewk_view_on_mouse_up(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:327
> +static void _ewk_view_on_mouse_move(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:336
> +static void _ewk_view_on_key_down(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:345
> +static void _ewk_view_on_key_up(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:455
> +static void _ewk_view_smart_move(Evas_Object* ewkView, Evas_Coord, Evas_Coord)

ditto.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:266
> +void WebChromeClient::addMessageToConsole(MessageSource, MessageType, MessageLevel, const String& message, unsigned lineNumber, const String& /*sourceID*/)

ditto.

[Jinwoo Song]

Comment on attachment 164539 [details]
patch

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

>> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:396
>> +void ChromeClientEfl::reachedMaxAppCacheSize(int64_t)
> 
> /* spaceNeeded */ ?

fixed.

>> Source/WebKit/efl/ewk/ewk_frame.cpp:1828
>> +WTF::PassRefPtr<WebCore::Widget> ewk_frame_plugin_create(Evas_Object* ewkFrame, const WebCore::IntSize& pluginSize, WebCore::HTMLPlugInElement* element, const WebCore::KURL& url, const WTF::Vector<WTF::String>& paramName, const WTF::Vector<WTF::String>& paramValues, const WTF::String& mimeType, bool loadManually)
> 
> I think paramNames is proper for Vector.

fixed the typo. Nice eye to catch the 's'.

>> Source/WebKit/efl/ewk/ewk_view.cpp:620
>> +static void _ewk_view_smart_add_console_message(Ewk_View_Smart_Data*, const char* message, unsigned lineNumber, const char* sourceID)
> 
> You should not change *unsigned int* to *unsigned* in this patch.

okay. I'll not change it in this patch.

>> Source/WebKit/efl/ewk/ewk_view.cpp:1040
>> +static void _ewk_view_smart_move(Evas_Object* ewkView, Evas_Coord, Evas_Coord)
> 
> Need to use /* */ for x, y.

fixed.

>> Source/WebKit/efl/ewk/ewk_view.cpp:4481
>> +bool ewk_view_accelerated_compositing_object_create(Evas_Object* /*ewkView*/, Evas_Native_Surface* /*nativeSurface*/, const WebCore::IntRect& /*rect*/)
> 
> Unneeded to wrap /* */ to ewkView and nativeSurface.

fixed.

>> Source/WebKit/efl/ewk/ewk_view.cpp:4487
>> +WebCore::GraphicsContext3D* ewk_view_accelerated_compositing_context_get(Evas_Object* /*ewkView*/)
> 
> Unneeded /* */ because ewkView is clear in this file.

fixed.

>> Source/WebKit/efl/ewk/ewk_view_tiled.cpp:49
>> +static void* _ewk_view_tiled_updates_process_pre(void* data, Evas_Object* /*ewkView*/)
> 
> ditto.

fixed.

>> Source/WebKit/efl/ewk/ewk_view_tiled.cpp:66
>> +_ewk_view_tiled_contents_size_changed_cb(void* data, Evas_Object* /*ewkView*/, void* eventInfo)
> 
> ditto.

fixed.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:292
>> +static void _ewk_view_on_focus_out(void* data, Evas*, Evas_Object* /*ewkView*/, void* /*eventInfo*/)
> 
> ditto.

fixed.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:300
>> +static void _ewk_view_on_mouse_wheel(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)
> 
> ditto.

fixed.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:309
>> +static void _ewk_view_on_mouse_down(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)
> 
> ditto.

fixed.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:327
>> +static void _ewk_view_on_mouse_move(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)
> 
> ditto.

fixed.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:336
>> +static void _ewk_view_on_key_down(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)
> 
> ditto.

fixed.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:345
>> +static void _ewk_view_on_key_up(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)
> 
> ditto.

fixed.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:455
>> +static void _ewk_view_smart_move(Evas_Object* ewkView, Evas_Coord, Evas_Coord)
> 
> ditto.

fixed.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:266
>> +void WebChromeClient::addMessageToConsole(MessageSource, MessageType, MessageLevel, const String& message, unsigned lineNumber, const String& /*sourceID*/)
> 
> ditto.

fixed.

[Jinwoo Song]

Created attachment 164649 [details]
patch applying Gyuyoung's comments.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:266:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit/efl/ewk/ewk_view.cpp:620:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 2 in 68 files


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

[Jinwoo Song]

(In reply to comment #25)
> Attachment 164649 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1
> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:266:  Omit int when using unsigned  [runtime/unsigned] [1]
> Source/WebKit/efl/ewk/ewk_view.cpp:620:  Omit int when using unsigned  [runtime/unsigned] [1]
> Total errors found: 2 in 68 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

According to Gyuyoung's comment, I did not fixed the 'unsigned int' to 'unsigned' in this patch.

[Jinwoo Song]

Created attachment 164819 [details]
patch for landing

This patch only fixes the warnings related to unused variables.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:266:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit/efl/ewk/ewk_view.cpp:620:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 2 in 68 files


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

[Ryuan Choi]

Comment on attachment 164819 [details]
patch for landing

Tried.

[WebKit Review Bot]

Comment on attachment 164819 [details]
patch for landing

Rejecting attachment 164819 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/efl/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13935055

[Jinwoo Song]

Created attachment 164837 [details]
patch.

Add the reviewer's name in the ChangeLog.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:266:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit/efl/ewk/ewk_view.cpp:620:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 2 in 68 files


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

[WebKit Review Bot]

Comment on attachment 164837 [details]
patch.

Clearing flags on attachment: 164837

Committed r129096: <http://trac.webkit.org/changeset/129096>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.