[webkit-reviews] review granted: [Bug 200959] Web Inspector: use more C++ keywords for defining agents : [Attachment 376838] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 200959: Web Inspector: use more C++ keywords for defining agents
https://bugs.webkit.org/show_bug.cgi?id=200959

Attachment 376838: Patch

https://bugs.webkit.org/attachment.cgi?id=376838&action=review




--- Comment #2 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 376838
  --> https://bugs.webkit.org/attachment.cgi?id=376838
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?