Most agents/commands can be declared as `final`, and have an `explicit` constructor with a virtual destructor.
Created attachment 376838 [details] Patch
Comment on attachment 376838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376838&action=review These changes undoubtably causes more history churn than is necessary and doesn't really fix any issues. I like the uniformity but some of the change don't make sense. > Source/JavaScriptCore/ChangeLog:8 > + - make constructors `explicit` to ensure that they're called with the right context type I thought `explicit` only made sense when constructors take one argument to avoid implicit construction. What is the advantage here? > Source/JavaScriptCore/ChangeLog:11 > + - use `final` wherever possible If a class is already marked as final there is no need to doubly mark methods as final. The final is clearer. > Source/JavaScriptCore/ChangeLog:12 > + - add comments to indicate where any virtual functions come from In general these comments are great, but the additional code movement and `Inspector::` namespacing seem a net negative. > Source/WebCore/inspector/agents/InspectorCSSAgent.h:86 > + // Inspector::CSSBackendDispatcherHandler The `Inspector::` in these actually makes it harder to read. > Source/WebCore/inspector/agents/WebConsoleAgent.h:49 > +protected: > + explicit WebConsoleAgent(WebAgentContext&); Is this distinction actually important? > Source/WebCore/inspector/agents/worker/WorkerDebuggerAgent.h:46 > + void muteConsole() final { } The class was already marked as final so this feels like code churn. > Source/WebKit/UIProcess/WebPageInspectorTargetAgent.h:35 > + explicit WebPageInspectorTargetAgent(Inspector::FrontendRouter&, Inspector::BackendDispatcher&); The `explicit` keyword doesn't make sense for constructors with multiple arguments. I see you made this change to all constructors, is there an advantage?
> > Source/JavaScriptCore/ChangeLog:11 > > + - use `final` wherever possible > > If a class is already marked as final there is no need to doubly mark > methods as final. The final is clearer. I meant "though final is clearer", so I'm fine with these changes.
Comment on attachment 376838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376838&action=review >> Source/JavaScriptCore/ChangeLog:8 >> + - make constructors `explicit` to ensure that they're called with the right context type > > I thought `explicit` only made sense when constructors take one argument to avoid implicit construction. What is the advantage here? AFAIU, using `explicit` prevented implicit argument conversion in general, but looking at it more, there's no real "benefit" to these cases, so I'll undo this part of the change. >> Source/JavaScriptCore/ChangeLog:11 >> + - use `final` wherever possible > > If a class is already marked as final there is no need to doubly mark methods as final. The final is clearer. Good point! I'll remove the `override` from this (and those like it). >> Source/JavaScriptCore/ChangeLog:12 >> + - add comments to indicate where any virtual functions come from > > In general these comments are great, but the additional code movement and `Inspector::` namespacing seem a net negative. I've found that these comments really help me navigate some of these files, and also generally help with organization. I think they should be moved at some point, so having it all be in a "neutral" patch seems like a simple and straightforward time to do it. >> Source/WebCore/inspector/agents/InspectorCSSAgent.h:86 >> + // Inspector::CSSBackendDispatcherHandler > > The `Inspector::` in these actually makes it harder to read. Looking at it again after a few days, I agree. >> Source/WebCore/inspector/agents/WebConsoleAgent.h:49 >> + explicit WebConsoleAgent(WebAgentContext&); > > Is this distinction actually important? Technically, no, but given how we have actual subclasses for Page and Worker, I'd think we wouldn't want this to be directly constructed anywhere. Alternatively, `WorkerConsoleAgent` doesn't actually do anything, so we could remove `WorkerConsoleAgent` and just use `WebConsoleAgent` instead. I'll put this back in case we decide to do that in the future.
Created attachment 377299 [details] Patch
Created attachment 377306 [details] Patch
Created attachment 377307 [details] Patch
Comment on attachment 377307 [details] Patch Clearing flags on attachment: 377307 Committed r249132: <https://trac.webkit.org/changeset/249132>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54735374>
(In reply to Devin Rousso from comment #4) > Comment on attachment 376838 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376838&action=review > > >> Source/JavaScriptCore/ChangeLog:11 > >> + - use `final` wherever possible > > > > If a class is already marked as final there is no need to doubly mark methods as final. The final is clearer. > > Good point! I'll remove the `override` from this (and those like it). > I don't know if WebKit is planning to enforce a compiler check that any method which overrides a virtual method has to be explicitly marked with either 'override' or 'final'. Chromium did enable it at some point and I've seen it enforced in other projects too. If there is such a plan this would change go against it. Marking methods with 'override'/'final' even if the class is already marked as 'final' will ensure that changes to the method signature in the super class without updating its overrides in subclasses will trigger compilation errors.
Each of the methods not marked with override or final now triggers warning during compilation with clang on GTK: In file included from DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-28.cpp:5: In file included from ../../Source/WebKit/WebProcess/WebPage/WebInspector.cpp:43: DerivedSources/ForwardingHeaders/WebCore/InspectorPageAgent.h:106:10: warning: 'getResourceContent' overrides a member function but is not marked 'override' [-Winconsistent-missing-override] void getResourceContent(ErrorString&, const String& frameId, const String& url, String* content, bool* base64Encoded); ^ Any ideas how to deal with this? I've described my point of view in #c11 above and it seems that the check is already enabled at least on some platforms so I believe we should either suppress the warnings or fix them.
(In reply to Yury Semikhatsky from comment #12) > Each of the methods not marked with override or final now triggers warning > during compilation with clang on GTK: > > In file included from > DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-28.cpp:5: > In file included from > ../../Source/WebKit/WebProcess/WebPage/WebInspector.cpp:43: > DerivedSources/ForwardingHeaders/WebCore/InspectorPageAgent.h:106:10: > warning: 'getResourceContent' overrides a member function but is not marked > 'override' [-Winconsistent-missing-override] > void getResourceContent(ErrorString&, const String& frameId, const > String& url, String* content, bool* base64Encoded); > ^ > > Any ideas how to deal with this? I've described my point of view in #c11 > above and it seems that the check is already enabled at least on some > platforms so I believe we should either suppress the warnings or fix them. They should be marked as either override or final. I was just trying to not make code churn by turning overrides into final if the class was already final.
Created attachment 379512 [details] Patch
Comment on attachment 379512 [details] Patch We don't normally do mass modifies like this (as you've seen Ryosuke's comments on the webkit-dev thread you started). But given this cleans things up so efficiently I'm fine with it. r=me
Comment on attachment 379512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379512&action=review > Source/JavaScriptCore/ChangeLog:11 > + clang-tidy -checks='-*,modernize-use-override' -header-filter='.*inspector.*' -fix -p WebKitBuild/Release/ Most of these should be `final` instead of `override`, so I'd change that. I'm not convinced about marking the destructor as `override`/`final`, but it's more of a personal preference so I won't block this on that. I'd defer to whatever webkit-dev ends up deciding on.
Created attachment 380699 [details] Patch for landing
(In reply to Devin Rousso from comment #16) > Comment on attachment 379512 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=379512&action=review > > > Source/JavaScriptCore/ChangeLog:11 > > + clang-tidy -checks='-*,modernize-use-override' -header-filter='.*inspector.*' -fix -p WebKitBuild/Release/ > > Most of these should be `final` instead of `override`, so I'd change that. My way of thinking is the following: by default methods are marked as override with rare exceptions of final when there is a strong reason to seal the method. One example is type() that returns constant passed in the class's constructor. E.g. for protocol handler methods it should usually be allowed to override them in subclasses. Somewhat orthogonal to that is marking class as final to show that there are no descendants. This restriction can be easily lifted later byr removing one keyword from the class declaration if we need to inherit from it. If each method is marked as final you'd have to go through each of them and see if it's ok to unseal it. If there is a strong preference toward one way over the other it'd be nice to articulate it in the WebKit style guide so that it's implemented consistently across the code base. This patch simply fixes violations of https://webkit.org/code-style-guidelines/#override-methods
For posterity, these are the commands I used to prepare the patch: Tools/Scripts/build-webkit --gtk --cmakeargs="-GNinja -DCMAKE_CXX_COMPILER=/usr/lib/ccache/clang++ -DCMAKE_C_COMPILER=/usr/lib/ccache/clang -DCMAKE_EXPORT_COMPILE_COMMANDS=ON" clang-tidy -checks='-*,modernize-use-override' -header-filter='.*inspector.*' -fix -p WebKitBuild/Release/ WebKitBuild/Release/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-84c9f43f-*.cpp WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource-84c9f43f-*.cpp
Comment on attachment 380699 [details] Patch for landing Clearing flags on attachment: 380699 Committed r250996: <https://trac.webkit.org/changeset/250996>