[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]
Created attachment 357309 [details] Patch
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.
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.
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.
Created attachment 357426 [details] Patch
Review, please.
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?
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.
(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.
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
(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.
Created attachment 357648 [details] Patch for landing
Committed r239366: <https://trac.webkit.org/changeset/239366>
<rdar://problem/46830492>