Bug 192695 – [Win][Clang] Fix compilation warnings under Source/WebKit directory

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Fujii Hironori]

[Win][Clang] Fix compilation warnings Source/WebKit directory

clang-cl reports the following compilation warnings:

> ..\..\Source\WebKit\NetworkProcess/NetworkResourceLoader.cpp(221,86):  warning: lambda capture 'this' is not used [-Wunused-lambda-capture]
> ..\..\Source\WebKit\NetworkProcess/cache/NetworkCacheData.cpp(145,13):  warning: unused function 'makeSalt' [-Wunused-function]
> ..\..\Source\WebKit\NetworkProcess/cache/NetworkCacheFileSystem.cpp(54,27):  warning: unused function 'directoryEntryType' [-Wunused-function]
> ..\..\Source\WebKit\NetworkProcess\curl\NetworkDataTaskCurl.cpp(238,17):  warning: enumeration value 'Suspend' not handled in switch [-Wswitch]
> ..\..\Source\WebKit\Platform\IPC\win\ConnectionWin.cpp(128,27):  warning: unused variable 'error' [-Wunused-variable]
> ..\..\Source\WebKit\Platform\win\ModuleWin.cpp(54,12):  warning: implicit conversion between pointer-to-function and pointer-to-object is a Microsoft extension [-Wmicrosoft-cast]
> ..\..\Source\WebKit\Shared\win\WebEventFactory.h(44,126):  warning: missing field 'time' initializer [-Wmissing-field-initializers]
> ..\..\Source\WebKit\UIProcess/DrawingAreaProxyImpl.h(79,23):  warning: private field 'm_webPage' is not used [-Wunused-private-field]
> ..\..\Source\WebKit\UIProcess/UserMediaPermissionRequestManagerProxy.cpp(46,17):  warning: unused function 'generateRequestID' [-Wunused-function]
> ..\..\Source\WebKit\UIProcess\Launcher\win\ProcessLauncherWin.cpp(102,15):  warning: unused variable 'error' [-Wunused-variable]
> ..\..\Source\WebKit\UIProcess\Launcher\win\ProcessLauncherWin.cpp(38,8):  warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
> ..\..\Source\WebKit\UIProcess\win/WebPopupMenuProxyWin.h(48,18):  warning: 'showPopupMenu' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
> ..\..\Source\WebKit\UIProcess\win/WebPopupMenuProxyWin.h(49,18):  warning: 'hidePopupMenu' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
> ..\..\Source\WebKit\UIProcess\win\WebInspectorProxyWin.cpp(304,14):  warning: unused variable 'inspectedHeight' [-Wunused-variable]
> ..\..\Source\WebKit\UIProcess\win\WebInspectorProxyWin.cpp(305,14):  warning: unused variable 'inspectedWidth' [-Wunused-variable]
> ..\..\Source\WebKit\UIProcess\win\WebInspectorProxyWin.cpp(330,21):  warning: unused variable 'haveRegisteredClass' [-Wunused-variable]
> ..\..\Source\WebKit\UIProcess\win\WebPopupMenuProxyWin.cpp(53,22):  warning: unused variable 'separatorPadding' [-Wunused-const-variable]
> ..\..\Source\WebKit\UIProcess\win\WebPopupMenuProxyWin.cpp(54,22):  warning: unused variable 'separatorHeight' [-Wunused-const-variable]
> ..\..\Source\WebKit\UIProcess\win\WebView.cpp(478,18):  warning: unused variable 'winRect' [-Wunused-variable]
> ..\..\Source\WebKit\WebProcess\WebCoreSupport\curl\WebFrameNetworkingContext.h(50,28):  warning: 'blockedError' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
> ..\..\Source\WebKit\WebProcess\WebCoreSupport\win\WebPopupMenuWin.cpp(114,23):  warning: unused variable 'direction' [-Wunused-variable]

[Fujii Hironori]

Created attachment 357309 [details]
Patch

[Alex Christensen]

Comment on attachment 357309 [details]
Patch

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

> Source/WebKit/ChangeLog:13
> +        (WebKit::NetworkDataTaskCurl::invokeDidReceiveResponse): Added default case.

Why?  This doesn't need a default case, and having default makes us forget to update this when we change the enum values.  This particular one might need to be changed to WebPolicyAction

> Source/WebKit/UIProcess/win/WebView.cpp:477
> +        for (size_t i = 0; i < unpaintedRects.size(); ++i)

This should be a c++11-style for loop.

[Fujii Hironori]

Comment on attachment 357309 [details]
Patch

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

Thank you for the review.

>> Source/WebKit/ChangeLog:13
>> +        (WebKit::NetworkDataTaskCurl::invokeDidReceiveResponse): Added default case.
> 
> Why?  This doesn't need a default case, and having default makes us forget to update this when we change the enum values.  This particular one might need to be changed to WebPolicyAction

What is WebPolicyAction? By googling the word, I found it in Chromium and webkit-gtk-clutter. 
https://www.google.com/search?biw=1366&bih=667&tbs=li%3A1&ei=I44UXNHGBszt-QaQ16TQCA&q=WebPolicyAction+webkit&oq=WebPolicyAction+webkit&gs_l=psy-ab.3...11688.12208..12717...0.0..0.101.281.2j1......0....1..gws-wiz.uhhXDcMPfrM

The following is the compilation warning I want to fix.
> ..\..\Source\WebKit\NetworkProcess\curl\NetworkDataTaskCurl.cpp(238,17):  warning: enumeration value 'Suspend' not handled in switch [-Wswitch]
I will add missing 'Suspend' case.

>> Source/WebKit/UIProcess/win/WebView.cpp:477
>> +        for (size_t i = 0; i < unpaintedRects.size(); ++i)
> 
> This should be a c++11-style for loop.

Will fix.

[Fujii Hironori]

Comment on attachment 357309 [details]
Patch

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

>>> Source/WebKit/ChangeLog:13
>>> +        (WebKit::NetworkDataTaskCurl::invokeDidReceiveResponse): Added default case.
>> 
>> Why?  This doesn't need a default case, and having default makes us forget to update this when we change the enum values.  This particular one might need to be changed to WebPolicyAction
> 
> What is WebPolicyAction? By googling the word, I found it in Chromium and webkit-gtk-clutter. 
> https://www.google.com/search?biw=1366&bih=667&tbs=li%3A1&ei=I44UXNHGBszt-QaQ16TQCA&q=WebPolicyAction+webkit&oq=WebPolicyAction+webkit&gs_l=psy-ab.3...11688.12208..12717...0.0..0.101.281.2j1......0....1..gws-wiz.uhhXDcMPfrM
> 
> The following is the compilation warning I want to fix.

PolicyAction::Suspend was removed and WebPolicyAction has been introduced in Bug 192701.
But, WebPolicyAction has been introduced only in UIProcess, I should remove PolicyAction::Suspend in Network Process.

[Fujii Hironori]

Created attachment 357426 [details]
Patch

[Fujii Hironori]

Review, please.

[Alex Christensen]

Comment on attachment 357426 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/win/WebPopupMenuProxyWin.h:48
> +    void showPopupMenu(const WebCore::IntRect&, WebCore::TextDirection, double pageScaleFactor, const Vector<WebPopupItem>&, const PlatformPopupMenuData&, int32_t selectedIndex) override;

final?

> Source/WebKit/WebProcess/WebCoreSupport/curl/WebFrameNetworkingContext.h:50
> +    WebCore::ResourceError blockedError(const WebCore::ResourceRequest&) const override;

final?

[Fujii Hironori]

https://stackoverflow.com/q/29412412/2691131
https://en.cppreference.com/w/cpp/language/final

IIUC, 'final', 'final override' and 'override' member function marker are exactly same in this case.
But, IMHO, using 'override' member function marker and 'final' class marker is better in this case.

> class WebPopupMenuProxyWin final : ... {
>   void hidePopupMenu() override;


'final' member function markers are useful only the following case:

> class B {
> public:
>   virtual foo();
>   virtual bar();
> };
> class B1 : public B {
>   foo() final;  
> };
> class D final : B1 {
>   bar() override;
> };

The 'final' member function marker indicate D can't inherit the method foo.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #8)
> The 'final' member function marker indicate D can't inherit the method foo.

So, using 'final' member function marker implies other vitual functions can be overridden.
I'm not sure my understanding right.

[Fujii Hironori]

Unfortunately, WebKit Code Style Guidelines seems to prefer 'final' method marker. I will use it.

https://webkit.org/code-style-guidelines/#override-methods
Bug 154978 – Update style guide to reflect our style of only using "override" or "final" when overriding virtual methods

[Fujii Hironori]

(In reply to Fujii Hironori from comment #10)
> Unfortunately, WebKit Code Style Guidelines seems to prefer 'final' method
> marker. I will use it.
> 
> https://webkit.org/code-style-guidelines/#override-methods
> Bug 154978 – Update style guide to reflect our style of only using
> "override" or "final" when overriding virtual methods

Grepping thought WebKit source code, there are a lot of 'final' class markers.
And, inconsistently marked methods 'final' or 'override'.
I will land this patch. Then. I will have a discussion on it in webkit-dev.

[Fujii Hironori]

Created attachment 357648 [details]
Patch for landing

[Fujii Hironori]

Committed r239366: <https://trac.webkit.org/changeset/239366>

[Radar WebKit Bug Importer]

<rdar://problem/46830492>