Bug 98020 – Fix compilation warnings

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

[Chris Dumez]

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]

[Chris Dumez]

Created attachment 166455 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 166455 [details]
Patch

Looks fine to me.

[Kentaro Hara]

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?

[Chris Dumez]

Created attachment 166459 [details]
Patch

Take Haraken's feedback into consideration.

[Kentaro Hara]

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.

[Chris Dumez]

(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.

[Kentaro Hara]

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

[Gyuyoung Kim]

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 ?

[Kentaro Hara]

(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?

[Kentaro Hara]

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.)

[Chris Dumez]

Well, if Gyuyoung wants to take over. I think UNUSED_PARAM() vs comment is a matter of taste...

[Kentaro Hara]

(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.

[Kentaro Hara]

Sorry for the nit-picking.

[Gyuyoung Kim]

(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.

[Chris Dumez]

Created attachment 166505 [details]
Patch

Followed advice on mailing list thread.

[Gyuyoung Kim]

Comment on attachment 166505 [details]
Patch

Looks fine.

[WebKit Review Bot]

Comment on attachment 166505 [details]
Patch

Clearing flags on attachment: 166505

Committed r130061: <http://trac.webkit.org/changeset/130061>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.