Bug 199184 - Web Inspector: Implement console.timeLog
Summary: Web Inspector: Implement console.timeLog
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 186833 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-06-24 21:00 PDT by Joseph Pecoraro
Modified: 2019-07-30 13:35 PDT (History)
13 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (37.24 KB, patch)
2019-06-24 21:06 PDT, Joseph Pecoraro
drousso: review+
Details | Formatted Diff | Diff
[PATCH] For Landing (38.47 KB, patch)
2019-06-25 10:53 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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. ***