[webkit-reviews] review granted: [Bug 199184] Web Inspector: Implement console.timeLog : [Attachment 372818] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 199184: Web Inspector: Implement console.timeLog
https://bugs.webkit.org/show_bug.cgi?id=199184

Attachment 372818: [PATCH] Proposed Fix

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




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 372818
  --> https://bugs.webkit.org/attachment.cgi?id=372818
[PATCH] Proposed Fix

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

r=me, nice!

> LayoutTests/inspector/console/console-time.html:17
>	   name: "DefaultLabel",

Style: we usually prefix all test case names with the name of the suite:
`console.time.DefaultLabel`.

(repeated below)

> LayoutTests/inspector/console/console-time.html:21
> +	       const expected = 6;

Might be worth a comment saying that `console.time()` doesn't log anything, so
this is the count of `console.timeLog` and `console.timeEnd`.

(repeated below)

> Source/JavaScriptCore/inspector/agents/InspectorConsoleAgent.cpp:149
> +    if
(!m_injectedScriptManager.inspectorEnvironment().developerExtrasEnabled())

NIT: should we put this inside a `LIKELY`?

> Source/JavaScriptCore/runtime/ConsoleObject.cpp:322
> +    ConsoleClient* client = exec->lexicalGlobalObject()->consoleClient();

Style: `auto*`?

> Source/WebCore/inspector/InspectorInstrumentation.cpp:891
> +    if
(!instrumentingAgents.inspectorEnvironment().developerExtrasEnabled())

NIT: should we put this inside a `LIKELY`?

> Source/WebCore/inspector/InspectorInstrumentation.cpp:894
> +    if (WebConsoleAgent* consoleAgent =
instrumentingAgents.webConsoleAgent())

Style: `auto*`?

> Source/WebCore/inspector/InspectorInstrumentation.h:1416
> +    if (InstrumentingAgents* instrumentingAgents =
instrumentingAgentsForFrame(frame))

Style: `auto*`?

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:293
> +		   if (this._extraParameters)

IIUC, `this._extraParameters` is equal to `this._message.parameters`?

At some point we should probably update the other users of
`this._message.parameters` in this class to use `this._extraParameters` instead
🤔