RESOLVED FIXED 96742
Fix unused parameter compile warnings in WebKit/WebKit2
https://bugs.webkit.org/show_bug.cgi?id=96742
Summary Fix unused parameter compile warnings in WebKit/WebKit2
Jinwoo Song
Reported 2012-09-14 02:52:42 PDT
Fix remained unused parameter warnings during EFL build. Patch is coming.
Attachments
patch (101.46 KB, patch)
2012-09-14 22:56 PDT, Jinwoo Song
no flags
patch (101.49 KB, patch)
2012-09-15 00:36 PDT, Jinwoo Song
no flags
patch. (88.96 KB, patch)
2012-09-17 04:48 PDT, Jinwoo Song
no flags
patch. (88.00 KB, patch)
2012-09-17 05:33 PDT, Jinwoo Song
no flags
patch (101.46 KB, patch)
2012-09-17 21:44 PDT, Jinwoo Song
no flags
patch (103.83 KB, patch)
2012-09-18 05:46 PDT, Jinwoo Song
no flags
patch applying Gyuyoung's comments. (101.97 KB, patch)
2012-09-18 20:21 PDT, Jinwoo Song
gyuyoung.kim: review+
patch for landing (101.84 KB, patch)
2012-09-19 18:45 PDT, Jinwoo Song
webkit.review.bot: commit-queue-
patch. (101.84 KB, patch)
2012-09-19 22:08 PDT, Jinwoo Song
no flags
Jinwoo Song
Comment 1 2012-09-14 22:56:49 PDT
Jinwoo Song
Comment 2 2012-09-15 00:36:25 PDT
Chris Dumez
Comment 3 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.
Gyuyoung Kim
Comment 4 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.
Jinwoo Song
Comment 5 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.
WebKit Review Bot
Comment 6 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.
Gyuyoung Kim
Comment 7 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.
Jinwoo Song
Comment 8 2012-09-17 05:33:57 PDT
Created attachment 164374 [details] patch. Changed the title and fixed the style error.
Gyuyoung Kim
Comment 9 2012-09-17 07:22:43 PDT
Comment on attachment 164374 [details] patch. Looks fine on EFL port side.
Gyuyoung Kim
Comment 10 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.
Jinwoo Song
Comment 11 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.
Gyuyoung Kim
Comment 12 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.
Jinwoo Song
Comment 13 2012-09-17 21:44:06 PDT
Jinwoo Song
Comment 14 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. :)
Mikhail Pozdnyakov
Comment 15 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
Mikhail Pozdnyakov
Comment 16 2012-09-17 23:52:37 PDT
*** Bug 96901 has been marked as a duplicate of this bug. ***
Gyuyoung Kim
Comment 17 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 ?
Mikhail Pozdnyakov
Comment 18 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.
Gyuyoung Kim
Comment 19 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.
Jinwoo Song
Comment 20 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.
Jinwoo Song
Comment 21 2012-09-18 05:46:38 PDT
Created attachment 164539 [details] patch Keep the primitive variable names in /* */.
Gyuyoung Kim
Comment 22 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.
Jinwoo Song
Comment 23 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.
Jinwoo Song
Comment 24 2012-09-18 20:21:49 PDT
Created attachment 164649 [details] patch applying Gyuyoung's comments.
WebKit Review Bot
Comment 25 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.
Jinwoo Song
Comment 26 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.
Jinwoo Song
Comment 27 2012-09-19 18:45:02 PDT
Created attachment 164819 [details] patch for landing This patch only fixes the warnings related to unused variables.
WebKit Review Bot
Comment 28 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.
Ryuan Choi
Comment 29 2012-09-19 21:52:56 PDT
Comment on attachment 164819 [details] patch for landing Tried.
WebKit Review Bot
Comment 30 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
Jinwoo Song
Comment 31 2012-09-19 22:08:37 PDT
Created attachment 164837 [details] patch. Add the reviewer's name in the ChangeLog.
WebKit Review Bot
Comment 32 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.
WebKit Review Bot
Comment 33 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>
WebKit Review Bot
Comment 34 2012-09-19 23:52:41 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.