Bug 131260

Summary: [WK2] Fix unused parameter compile warning
Product: WebKit Reporter: Miyoung Shin <myid.shin>
Component: WebKit2Assignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, gyuyoung.kim, ossy, thiago.lacerda
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch
none
Patch none

Description Miyoung Shin 2014-04-04 21:44:36 PDT
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) { }
Comment 1 Miyoung Shin 2014-04-05 00:12:51 PDT
Created attachment 228663 [details]
Patch
Comment 2 Gyuyoung Kim 2014-04-05 03:58:59 PDT
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.
Comment 3 Miyoung Shin 2014-04-05 05:24:29 PDT
Created attachment 228669 [details]
Patch
Comment 4 Gyuyoung Kim 2014-04-05 05:46:46 PDT
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.
Comment 5 Miyoung Shin 2014-04-05 06:10:51 PDT
Created attachment 228671 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2014-04-05 19:34:03 PDT
Comment on attachment 228671 [details]
Patch for landing

Clearing flags on attachment: 228671

Committed r166848: <http://trac.webkit.org/changeset/166848>
Comment 7 WebKit Commit Bot 2014-04-05 19:34:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Adler 2014-04-06 10:37:15 PDT
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.
Comment 9 Gyuyoung Kim 2014-04-06 20:42:36 PDT
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 ?
Comment 10 Gyuyoung Kim 2014-04-06 21:36:55 PDT
Reopening to attach new patch.
Comment 11 Gyuyoung Kim 2014-04-06 21:37:01 PDT
Created attachment 228717 [details]
Patch
Comment 12 Gyuyoung Kim 2014-04-06 21:37:33 PDT
(In reply to comment #11)
> Created an attachment (id=228717) [details]
> Patch

Darin, do you think this one is fine ?
Comment 13 Csaba Osztrogonác 2014-04-07 02:46:56 PDT
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.
Comment 14 Gyuyoung Kim 2014-04-07 02:57:10 PDT
(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.
Comment 15 WebKit Commit Bot 2014-04-07 02:58:07 PDT
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.
Comment 16 Csaba Osztrogonác 2014-04-07 03:06:14 PDT
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.
Comment 17 Gyuyoung Kim 2014-04-07 03:51:38 PDT
Created attachment 228725 [details]
Patch
Comment 18 Gyuyoung Kim 2014-04-07 03:52:30 PDT
(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.
Comment 19 WebKit Commit Bot 2014-04-07 03:53:37 PDT
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.
Comment 20 WebKit Commit Bot 2014-04-07 23:55:49 PDT
Comment on attachment 228725 [details]
Patch

Clearing flags on attachment: 228725

Committed r166914: <http://trac.webkit.org/changeset/166914>
Comment 21 WebKit Commit Bot 2014-04-07 23:55:56 PDT
All reviewed patches have been landed.  Closing bug.