Bug 199184

Summary: Web Inspector: Implement console.timeLog
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, domfarolino, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, mkwst, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
hi: review+
[PATCH] For Landing none

Comment 1 Joseph Pecoraro 2019-06-24 21:06:32 PDT
Created attachment 372818 [details]
[PATCH] Proposed Fix
Comment 2 EWS Watchlist 2019-06-24 21:08:18 PDT
Attachment 372818 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/JSGlobalObjectConsoleClient.h:57:  The parameter name "arguments" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Devin Rousso 2019-06-25 01:22:23 PDT
Comment on attachment 372818 [details]
[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 🤔
Comment 4 Joseph Pecoraro 2019-06-25 02:47:02 PDT
Comment on attachment 372818 [details]
[PATCH] Proposed Fix

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

>> 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 🤔

`this._extraParameters` sometimes gets cleared based on what has been read. It is kind of tricky given how different console messages are sent across.
Comment 5 Joseph Pecoraro 2019-06-25 10:53:43 PDT
Created attachment 372843 [details]
[PATCH] For Landing
Comment 6 WebKit Commit Bot 2019-06-25 11:36:36 PDT
Comment on attachment 372843 [details]
[PATCH] For Landing

Clearing flags on attachment: 372843

Committed r246798: <https://trac.webkit.org/changeset/246798>
Comment 7 Radar WebKit Bug Importer 2019-06-26 16:48:14 PDT
<rdar://problem/52218904>
Comment 8 Yury Semikhatsky 2019-07-30 13:35:41 PDT
*** Bug 186833 has been marked as a duplicate of this bug. ***