Bug 192695

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>