Bug 96742

Summary: Fix unused parameter compile warnings in WebKit/WebKit2
Product: WebKit Reporter: Jinwoo Song <jinwoo7.song>
Component: WebKit EFLAssignee: Jinwoo Song <jinwoo7.song>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, eric.carlson, feature-media-reviews, gyuyoung.kim, lucas.de.marchi, mikhail.pozdnyakov, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
patch
none
patch
none
patch.
none
patch.
none
patch
none
patch
none
patch applying Gyuyoung's comments.
gyuyoung.kim: review+
patch for landing
webkit.review.bot: commit-queue-
patch. none

Description Jinwoo Song 2012-09-14 02:52:42 PDT
Fix remained unused parameter warnings during EFL build. Patch is coming.
Comment 1 Jinwoo Song 2012-09-14 22:56:49 PDT
Created attachment 164274 [details]
patch
Comment 2 Jinwoo Song 2012-09-15 00:36:25 PDT
Created attachment 164278 [details]
patch
Comment 3 Chris Dumez 2012-09-15 01:05:52 PDT
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.
Comment 4 Gyuyoung Kim 2012-09-15 01:45:23 PDT
(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.
Comment 5 Jinwoo Song 2012-09-17 04:48:07 PDT
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.
Comment 6 WebKit Review Bot 2012-09-17 04:51:56 PDT
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.
Comment 7 Gyuyoung Kim 2012-09-17 04:52:51 PDT
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.
Comment 8 Jinwoo Song 2012-09-17 05:33:57 PDT
Created attachment 164374 [details]
patch.

Changed the title and fixed the style error.
Comment 9 Gyuyoung Kim 2012-09-17 07:22:43 PDT
Comment on attachment 164374 [details]
patch.

Looks fine on EFL port side.
Comment 10 Gyuyoung Kim 2012-09-17 07:43:51 PDT
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.
Comment 11 Jinwoo Song 2012-09-17 07:59:50 PDT
(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.
Comment 12 Gyuyoung Kim 2012-09-17 08:19:22 PDT
(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.
Comment 13 Jinwoo Song 2012-09-17 21:44:06 PDT
Created attachment 164489 [details]
patch
Comment 14 Jinwoo Song 2012-09-17 21:48:56 PDT
(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. :)
Comment 15 Mikhail Pozdnyakov 2012-09-17 23:45:58 PDT
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
Comment 16 Mikhail Pozdnyakov 2012-09-17 23:52:37 PDT
*** Bug 96901 has been marked as a duplicate of this bug. ***
Comment 17 Gyuyoung Kim 2012-09-17 23:59:16 PDT
(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 ?
Comment 18 Mikhail Pozdnyakov 2012-09-18 00:30:42 PDT
> 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.
Comment 19 Gyuyoung Kim 2012-09-18 00:34:54 PDT
(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.
Comment 20 Jinwoo Song 2012-09-18 00:44:53 PDT
(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.
Comment 21 Jinwoo Song 2012-09-18 05:46:38 PDT
Created attachment 164539 [details]
patch

Keep the primitive variable names in /* */.
Comment 22 Gyuyoung Kim 2012-09-18 19:08:46 PDT
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.
Comment 23 Jinwoo Song 2012-09-18 20:19:57 PDT
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.
Comment 24 Jinwoo Song 2012-09-18 20:21:49 PDT
Created attachment 164649 [details]
patch applying Gyuyoung's comments.
Comment 25 WebKit Review Bot 2012-09-18 20:24:33 PDT
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.
Comment 26 Jinwoo Song 2012-09-18 20:26:04 PDT
(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.
Comment 27 Jinwoo Song 2012-09-19 18:45:02 PDT
Created attachment 164819 [details]
patch for landing

This patch only fixes the warnings related to unused variables.
Comment 28 WebKit Review Bot 2012-09-19 18:46:32 PDT
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.
Comment 29 Ryuan Choi 2012-09-19 21:52:56 PDT
Comment on attachment 164819 [details]
patch for landing

Tried.
Comment 30 WebKit Review Bot 2012-09-19 21:54:51 PDT
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
Comment 31 Jinwoo Song 2012-09-19 22:08:37 PDT
Created attachment 164837 [details]
patch.

Add the reviewer's name in the ChangeLog.
Comment 32 WebKit Review Bot 2012-09-19 22:10:10 PDT
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.
Comment 33 WebKit Review Bot 2012-09-19 23:52:36 PDT
Comment on attachment 164837 [details]
patch.

Clearing flags on attachment: 164837

Committed r129096: <http://trac.webkit.org/changeset/129096>
Comment 34 WebKit Review Bot 2012-09-19 23:52:41 PDT
All reviewed patches have been landed.  Closing bug.