Bug 199200 - Web Inspector: Implement console.countReset
Summary: Web Inspector: Implement console.countReset
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
Depends on:
Blocks:
 
Reported: 2019-06-25 13:12 PDT by Joseph Pecoraro
Modified: 2019-06-26 16:54 PDT (History)
12 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (25.28 KB, patch)
2019-06-25 13:13 PDT, Joseph Pecoraro
hi: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (13.61 MB, application/zip)
2019-06-25 20:49 PDT, EWS Watchlist
no flags Details
[PATCH] For Landing (25.25 KB, patch)
2019-06-26 11:23 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-25 13:13:34 PDT
Created attachment 372856 [details]
[PATCH] Proposed Fix
Comment 2 EWS Watchlist 2019-06-25 13:16:02 PDT
Attachment 372856 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Devin Rousso 2019-06-25 15:11:05 PDT
Comment on attachment 372856 [details]
[PATCH] Proposed Fix

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

r=me

> LayoutTests/inspector/console/console-count.html:30
> +    for (let i = 0; i < 10; ++i) {

NIT: since `console.count` starts at 1, perhaps we should have it as `for (let i = 1; i <= 10; ++i)` so that you can "match" `if (i === 6 || i === 8)` with the test expectation (where the `6` and `8` are "missing").

> LayoutTests/inspector/console/console-count.html:39
> +    InspectorTest.debug();

Oops :P

> Source/JavaScriptCore/inspector/agents/InspectorConsoleAgent.cpp:211
> +void InspectorConsoleAgent::getCounterLabel(Ref<ScriptArguments>&& arguments, String& title, String& identifier)

Maybe return a `std::pair` or a custom `struct` instead?  I don't like out-arguments :(

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

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

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

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

> Source/JavaScriptCore/inspector/agents/InspectorConsoleAgent.cpp:263
> +    // FIXME: Web Inspector should have a better UI for counters, but for now we just log an updated counter value.

This isn't technically true, as we don't log anything when we call `console.countReset` :P

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

Style: `auto*`?

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

Style: `auto*`?
Comment 4 EWS Watchlist 2019-06-25 20:49:54 PDT
Comment on attachment 372856 [details]
[PATCH] Proposed Fix

Attachment 372856 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12576721

New failing tests:
svg/text/textpath-reference-update.html
Comment 5 EWS Watchlist 2019-06-25 20:49:57 PDT
Created attachment 372900 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 6 Joseph Pecoraro 2019-06-26 11:18:28 PDT
Comment on attachment 372856 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/inspector/agents/InspectorConsoleAgent.cpp:211
>> +void InspectorConsoleAgent::getCounterLabel(Ref<ScriptArguments>&& arguments, String& title, String& identifier)
> 
> Maybe return a `std::pair` or a custom `struct` instead?  I don't like out-arguments :(

I haven't seen that happening in a lot of places. In fact this function makes use of an out string already (getFirstArgumentAsString). Maybe that is a new norm?

>> Source/JavaScriptCore/inspector/agents/InspectorConsoleAgent.cpp:263
>> +    // FIXME: Web Inspector should have a better UI for counters, but for now we just log an updated counter value.
> 
> This isn't technically true, as we don't log anything when we call `console.countReset` :P

A better UI might inform the frontend that the counter reset to zero so the frontend could update.
Comment 7 Joseph Pecoraro 2019-06-26 11:23:20 PDT
Created attachment 372937 [details]
[PATCH] For Landing
Comment 8 EWS Watchlist 2019-06-26 13:20:55 PDT
Comment on attachment 372937 [details]
[PATCH] For Landing

Attachment 372937 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12582853

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
apiTests
Comment 9 WebKit Commit Bot 2019-06-26 15:25:54 PDT
Comment on attachment 372937 [details]
[PATCH] For Landing

Clearing flags on attachment: 372937

Committed r246850: <https://trac.webkit.org/changeset/246850>
Comment 10 Radar WebKit Bug Importer 2019-06-26 16:54:21 PDT
<rdar://problem/52219232>