Fix remained unused parameter warnings during EFL build. Patch is coming.
Created attachment 164274 [details] patch
Created attachment 164278 [details] patch
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.
(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.
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.
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 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.
Created attachment 164374 [details] patch. Changed the title and fixed the style error.
Comment on attachment 164374 [details] patch. Looks fine on EFL port side.
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.
(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.
(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.
Created attachment 164489 [details] patch
(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 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
*** Bug 96901 has been marked as a duplicate of this bug. ***
(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 ?
> 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.
(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.
(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.
Created attachment 164539 [details] patch Keep the primitive variable names in /* */.
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 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.
Created attachment 164649 [details] patch applying Gyuyoung's comments.
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.
(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.
Created attachment 164819 [details] patch for landing This patch only fixes the warnings related to unused variables.
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 on attachment 164819 [details] patch for landing Tried.
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
Created attachment 164837 [details] patch. Add the reviewer's name in the ChangeLog.
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 on attachment 164837 [details] patch. Clearing flags on attachment: 164837 Committed r129096: <http://trac.webkit.org/changeset/129096>
All reviewed patches have been landed. Closing bug.