Bug 199200 – Web Inspector: Implement console.countReset

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Implement console.countReset
https://developer.mozilla.org/en-US/docs/Web/API/Console/countReset
https://console.spec.whatwg.org/#countReset

[Joseph Pecoraro]

Created attachment 372856 [details]
[PATCH] Proposed Fix

[EWS Watchlist]

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.

[Devin Rousso]

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*`?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 372937 [details]
[PATCH] For Landing

[EWS Watchlist]

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

[WebKit Commit Bot]

Comment on attachment 372937 [details]
[PATCH] For Landing

Clearing flags on attachment: 372937

Committed r246850: <https://trac.webkit.org/changeset/246850>

[Radar WebKit Bug Importer]

<rdar://problem/52219232>