We have the following compilation warnings on WebKit EFL bots: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/plugins/efl/PluginViewEfl.cpp:158:6: warning: unused parameter ‘rect’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/plugins/efl/PluginViewEfl.cpp:322:6: warning: unused parameter ‘region’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/plugins/PluginView.cpp:590:9: warning: unused parameter ‘type’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/plugins/PluginView.cpp:590:9: warning: unused parameter ‘target’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/plugins/PluginView.cpp:590:9: warning: unused parameter ‘stream’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/plugins/PluginView.cpp:597:9: warning: unused parameter ‘stream’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/plugins/PluginView.cpp:597:9: warning: unused parameter ‘len’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/plugins/PluginView.cpp:597:9: warning: unused parameter ‘buffer’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/plugins/PluginView.cpp:1475:9: warning: unused parameter ‘scheme’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/plugins/PluginView.cpp:1475:9: warning: unused parameter ‘realm’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/plugins/PluginView.cpp:1475:9: warning: unused parameter ‘username’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/plugins/PluginView.cpp:1475:9: warning: unused parameter ‘ulen’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/plugins/PluginView.cpp:1475:9: warning: unused parameter ‘password’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/plugins/PluginView.cpp:1475:9: warning: unused parameter ‘plen’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/API/efl/ewk_view_ui_client.cpp:77:27: warning: unused parameter ‘origin’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:387:16: warning: deleting ‘void*’ is undefined [enabled by default] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:390:13: warning: unused parameter ‘data’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:390:13: warning: unused parameter ‘canvas’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:390:13: warning: unused parameter ‘eventInfo’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:395:13: warning: unused parameter ‘data’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:395:13: warning: unused parameter ‘canvas’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:395:13: warning: unused parameter ‘eventInfo’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:400:13: warning: unused parameter ‘data’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:400:13: warning: unused parameter ‘canvas’ [-Wunused-parameter] /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:400:13: warning: unused parameter ‘eventInfo’ [-Wunused-parameter]
Created attachment 166455 [details] Patch
Comment on attachment 166455 [details] Patch Looks fine to me.
Comment on attachment 166455 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166455&action=review > Source/WebCore/plugins/PluginView.cpp:594 > + UNUSED_PARAM(type); > + UNUSED_PARAM(target); > + UNUSED_PARAM(stream); Or you can just remove type, target and stream from the method arguments?
Created attachment 166459 [details] Patch Take Haraken's feedback into consideration.
Comment on attachment 166459 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166459&action=review > Source/WebCore/plugins/PluginView.cpp:1482 > + UNUSED_PARAM(scheme); > + UNUSED_PARAM(realm); > + UNUSED_PARAM(username); > + UNUSED_PARAM(ulen); > + UNUSED_PARAM(password); > + UNUSED_PARAM(plen); You can remove these arguments too? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:394 > + UNUSED_PARAM(data); > + UNUSED_PARAM(canvas); > + UNUSED_PARAM(eventInfo); Ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:402 > + UNUSED_PARAM(data); > + UNUSED_PARAM(canvas); > + UNUSED_PARAM(eventInfo); Ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:410 > + UNUSED_PARAM(data); > + UNUSED_PARAM(canvas); > + UNUSED_PARAM(eventInfo); Ditto.
(In reply to comment #5) > (From update of attachment 166459 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166459&action=review > > > Source/WebCore/plugins/PluginView.cpp:1482 > > + UNUSED_PARAM(scheme); > > + UNUSED_PARAM(realm); > > + UNUSED_PARAM(username); > > + UNUSED_PARAM(ulen); > > + UNUSED_PARAM(password); > > + UNUSED_PARAM(plen); > > You can remove these arguments too? > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:394 > > + UNUSED_PARAM(data); > > + UNUSED_PARAM(canvas); > > + UNUSED_PARAM(eventInfo); > > Ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:402 > > + UNUSED_PARAM(data); > > + UNUSED_PARAM(canvas); > > + UNUSED_PARAM(eventInfo); > > Ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:410 > > + UNUSED_PARAM(data); > > + UNUSED_PARAM(canvas); > > + UNUSED_PARAM(eventInfo); > > Ditto. Frankly I don't like removing arguments, especially in EFL port. By removing those, we lose information. By using UNUSED_PARAM(), if someone want to extend PluginView::getAuthenticationInfo() in the future, it is easy. If I remove argument names, it is not obvious what the char* refer to and which one is the username, which one is the password.
Comment on attachment 166459 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166459&action=review >>> Source/WebCore/plugins/PluginView.cpp:1482 >>> + UNUSED_PARAM(plen); >> >> You can remove these arguments too? > > Frankly I don't like removing arguments, especially in EFL port. > By removing those, we lose information. > > By using UNUSED_PARAM(), if someone want to extend PluginView::getAuthenticationInfo() in the future, it is easy. If I remove argument names, it is not obvious what the char* refer to and which one is the username, which one is the password. OK
Comment on attachment 166459 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166459&action=review >>>> Source/WebCore/plugins/PluginView.cpp:1482 >>>> + UNUSED_PARAM(plen); >>> >>> You can remove these arguments too? >> >> Frankly I don't like removing arguments, especially in EFL port. >> By removing those, we lose information. >> >> By using UNUSED_PARAM(), if someone want to extend PluginView::getAuthenticationInfo() in the future, it is easy. If I remove argument names, it is not obvious what the char* refer to and which one is the username, which one is the password. > > OK Previous patch used /* */ for fixing unused param build warning in primitive type. I don't like excessive UNUSED_PARAM is not good personally. For example, (const char* /*protocol*/, const char* /*host*/, int32_t /*port*/, const char* /*scheme*/, const char* /*realm*/, char** /*username*/, uint32_t* /*ulen*/, char** /*password*/, uint32_t* /*plen*/) How do you think ?
(In reply to comment #8) > (From update of attachment 166459 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166459&action=review > Previous patch used /* */ for fixing unused param build warning in primitive type. I don't like excessive UNUSED_PARAM is not good personally. > > For example, > (const char* /*protocol*/, const char* /*host*/, int32_t /*port*/, const char* /*scheme*/, const char* /*realm*/, char** /*username*/, uint32_t* /*ulen*/, char** /*password*/, uint32_t* /*plen*/) > > How do you think ? I think that both are used in WebKit. Also there is no coding style rule about it. So it would depend on the convention of the port/module/etc. Which one is used more in the EFL port?
Comment on attachment 166459 [details] Patch As gyuyoung pointed out, let's clarify the rule and make the patch consistent. At least we want to make the rule consistent within this patch. (This patch is inconsistent in a sense that the patch removes parameters in some places and uses UNUSED_PARAM() in other places.)
Well, if Gyuyoung wants to take over. I think UNUSED_PARAM() vs comment is a matter of taste...
(In reply to comment #11) > Well, if Gyuyoung wants to take over. I think UNUSED_PARAM() vs comment is a matter of taste... IMO, let's just use the popular one in the EFL port. But at least let's use the same rule in one patch.
Sorry for the nit-picking.
(In reply to comment #13) > Sorry for the nit-picking. IMO, this is not important problem. But, as you feel, I also think it is good to check if which one is popular in WebKit folks. I just would be check this with you guys. I just sent an email to webkit-dev. So, let's decide a rule or implicit one ,if possible.
Created attachment 166505 [details] Patch Followed advice on mailing list thread.
Comment on attachment 166505 [details] Patch Looks fine.
Comment on attachment 166505 [details] Patch Clearing flags on attachment: 166505 Committed r130061: <http://trac.webkit.org/changeset/130061>
All reviewed patches have been landed. Closing bug.