Bug 131260 – [WK2] Fix unused parameter compile warning

• Component: WebKit2
• Status: RESOLVED FIXED

[Miyoung Shin]

Building CXX object Source/WebKit2/CMakeFiles/WebKit2.dir/UIProcess/WebDatabaseManagerProxyClient.cpp.o
In file included from /home/myshin/webkit/WebKit/Source/WebKit2/UIProcess/WebContext.cpp:30:0:
/home/myshin/webkit/WebKit/Source/WebKit2/UIProcess/API/APIDownloadClient.h:51:18: warning: unused parameter ‘length’ [-Wunused-parameter]
     virtual void didReceiveData(WebKit::WebContext*, WebKit::DownloadProxy*, uint64_t length) { }
                  ^
/home/myshin/webkit/WebKit/Source/WebKit2/UIProcess/API/APIDownloadClient.h:52:18: warning: unused parameter ‘mimeType’ [-Wunused-parameter]
     virtual bool shouldDecodeSourceDataOfMIMEType(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String& mimeType) { return true; }
                  ^
/home/myshin/webkit/WebKit/Source/WebKit2/UIProcess/API/APIDownloadClient.h:53:25: warning: unused parameter ‘filename’ [-Wunused-parameter]
     virtual WTF::String decideDestinationWithSuggestedFilename(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String& filename, bool& allowOverwrite) { return {}; }
                         ^
/home/myshin/webkit/WebKit/Source/WebKit2/UIProcess/API/APIDownloadClient.h:53:25: warning: unused parameter ‘allowOverwrite’ [-Wunused-parameter]
/home/myshin/webkit/WebKit/Source/WebKit2/UIProcess/API/APIDownloadClient.h:54:18: warning: unused parameter ‘path’ [-Wunused-parameter]
     virtual void didCreateDestination(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String& path) { }

[Miyoung Shin]

Created attachment 228663 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 228663 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228663&action=review

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2] Fix unused parameter compile warning.

This patch is not only for [EFL]. Please remove [EFL] prefix.

> Source/WebKit2/UIProcess/API/APIDownloadClient.h:51
> +    virtual void didReceiveData(WebKit::WebContext*, WebKit::DownloadProxy*, uint64_t) { }

I prefer to use UNUSED_PARAM when parameter name is critical for clarity. In this case, length is that case.

UNUSED_PARAM(length); ?

Besides there was a discussion how to handle unused parameter in below thread.
https://lists.webkit.org/pipermail/webkit-dev/2012-October/022369.html

> Source/WebKit2/UIProcess/API/APIDownloadClient.h:52
> +    virtual bool shouldDecodeSourceDataOfMIMEType(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String&) { return true; }

ditto.

> Source/WebKit2/UIProcess/API/APIDownloadClient.h:53
> +    virtual WTF::String decideDestinationWithSuggestedFilename(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String&, bool&) { return { }; }

ditto.

> Source/WebKit2/UIProcess/API/APIDownloadClient.h:54
> +    virtual void didCreateDestination(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String&) { }

ditto.

[Miyoung Shin]

Created attachment 228669 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 228669 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228669&action=review

This patch modifies WK2 common part though, this is a trivial build warning fix related to unused param.

This change is based on below discussion 
https://lists.webkit.org/pipermail/webkit-dev/2012-October/022369.html

LGTM.

> Source/WebKit2/ChangeLog:7
> +

Please add description before landing.

[Miyoung Shin]

Created attachment 228671 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 228671 [details]
Patch for landing

Clearing flags on attachment: 228671

Committed r166848: <http://trac.webkit.org/changeset/166848>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 228671 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=228671&action=review

> Source/WebKit2/UIProcess/API/APIDownloadClient.h:63
> +    virtual void didReceiveData(WebKit::WebContext*, WebKit::DownloadProxy*, uint64_t length) { UNUSED_PARAM(length); }
> +    virtual bool shouldDecodeSourceDataOfMIMEType(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String& mimeType)
> +    {
> +        UNUSED_PARAM(mimeType);
> +        return true;
> +    }
> +    virtual WTF::String decideDestinationWithSuggestedFilename(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String& filename, bool& allowOverwrite)
> +    {
> +        UNUSED_PARAM(filename);
> +        UNUSED_PARAM(allowOverwrite);
> +        return { };
> +    }
> +    virtual void didCreateDestination(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String& path) { UNUSED_PARAM(path); }

This is not what we want. We should just remove the argument names, not use UNUSED_PARAM and not reformat the code.

[Gyuyoung Kim]

Comment on attachment 228671 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=228671&action=review

>> Source/WebKit2/UIProcess/API/APIDownloadClient.h:63
>> +    virtual void didCreateDestination(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String& path) { UNUSED_PARAM(path); }
> 
> This is not what we want. We should just remove the argument names, not use UNUSED_PARAM and not reformat the code.

APIFindClient.h, APIHistoryClient.h, APILoaderClient.h and APIUIClient.h have already used this style. Do you think we should remove the arguments as well ?

[Gyuyoung Kim]

Reopening to attach new patch.

[Gyuyoung Kim]

Created attachment 228717 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #11)
> Created an attachment (id=228717) [details]
> Patch

Darin, do you think this one is fine ?

[Csaba Osztrogonác]

I agree with Darin, we shouldn't add UNUSED_PARAM everywhere. We should
simple remove argument names if it is possible. But sometimes a parameter
is used inside an #if PLATFORM(FOO) guard, in these cases we can use the
UNUSED_PARAM macro in the #else case.

cc Thiago and Benajmin too - author and reviewer of r165794 which 
added many UNUSED_PARAM to these headers to revise that patch too.

[Gyuyoung Kim]

(In reply to comment #13)
> I agree with Darin, we shouldn't add UNUSED_PARAM everywhere. We should
> simple remove argument names if it is possible. But sometimes a parameter
> is used inside an #if PLATFORM(FOO) guard, in these cases we can use the
> UNUSED_PARAM macro in the #else case.
> 
> cc Thiago and Benajmin too - author and reviewer of r165794 which 
> added many UNUSED_PARAM to these headers to revise that patch too.

There was already a rule to handle UNUSED_PARAM().
https://lists.webkit.org/pipermail/webkit-dev/2012-October/022369.html 

In this case, I thought primitive type param is better to use UNUSED_PARAM() to clarify the parameter role. Besides r165794 also used it. However, it is better not to use UNUSED_PARAM in this case, I will follow it.

[WebKit Commit Bot]

Attachment 228717 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/APIPolicyClient.h:54:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebKit2/UIProcess/API/APIPolicyClient.h:58:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebKit2/UIProcess/API/APIPolicyClient.h:62:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Csaba Osztrogonác]

Comment on attachment 228717 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228717&action=review

The patch looks good to me, but I let Darin to do the final review.
Additionally we can remove UNUSED_PARAM macros from APIUIClient.h too.

> Source/WebKit2/UIProcess/API/APIDownloadClient.h:55
> +    virtual bool shouldDecodeSourceDataOfMIMEType(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String&)
> +    { return true; }
> +    virtual WTF::String decideDestinationWithSuggestedFilename(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String&, bool&)
> +    { return { }; }

I think we can avoid these linebreaks.

[Gyuyoung Kim]

Created attachment 228725 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #16)
> (From update of attachment 228717 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228717&action=review
> 
> The patch looks good to me, but I let Darin to do the final review.
> Additionally we can remove UNUSED_PARAM macros from APIUIClient.h too.

Fixed.
 
> > Source/WebKit2/UIProcess/API/APIDownloadClient.h:55
> > +    virtual bool shouldDecodeSourceDataOfMIMEType(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String&)
> > +    { return true; }
> > +    virtual WTF::String decideDestinationWithSuggestedFilename(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String&, bool&)
> > +    { return { }; }
> 
> I think we can avoid these linebreaks.

Fixed. Thanks.

[WebKit Commit Bot]

Attachment 228725 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/APIPolicyClient.h:54:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebKit2/UIProcess/API/APIPolicyClient.h:58:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebKit2/UIProcess/API/APIPolicyClient.h:62:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 228725 [details]
Patch

Clearing flags on attachment: 228725

Committed r166914: <http://trac.webkit.org/changeset/166914>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.