Bug 193428 – Implement PerformanceObserver.supportedEntryTypes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Implement PerformanceObserver.supportedEntryTypes

Created attachment 359123 [details]
Patch

[EWS Watchlist]

Attachment 359123 [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 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 359125 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 359125 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        PerformanceObserver.supportedEntryTypes should return an array of
> +        entryTypes that can be observed per specification[1].
> +
> +        [1] https://w3c.github.io/performance-timeline/#supportedentrytypes-attribute

No need for the Markdown syntax. This would read much clearer as a paragraph:

    PerformanceObserver.supportedEntryTypes should return an array of
    entryTypes that can be observed per specification:
    https://w3c.github.io/performance-timeline/#supportedentrytypes-attribute

> Source/WebCore/ChangeLog:14
> +        This is covered by web-platform-tests[1].
> +        [1] LayoutTests/imported/w3c/web-platform-tests/resource-timing/supported_resource_type.*.html

Same thing here.

> Source/WebCore/page/PerformanceObserver.cpp:119
> +        "navigation",

Does WebKit actually support navigation entries in PerformanceObserver? I didn't think we do that yet, and if so we probably shouldn't broadcast it as supported.

[Joseph Pecoraro]

Comment on attachment 359125 [details]
Patch

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

>> Source/WebCore/page/PerformanceObserver.cpp:119
>> +        "navigation",
> 
> Does WebKit actually support navigation entries in PerformanceObserver? I didn't think we do that yet, and if so we probably shouldn't broadcast it as supported.

Yeah, seems like this should be commented out with a FIXME:

    // FIXME: <https://webkit.org/b/184363> Add support for Navigation Timing Level 2
    // "navigation",

Because we don't yet have "navigation" entries in our performance timeline.

Created attachment 359128 [details]
Patch

Created attachment 359129 [details]
Patch

[Alex Christensen]

Comment on attachment 359129 [details]
Patch

It would be nice if there were web platform tests verifying somehow that the entry types claimed to be supported are actually supported.

[Joseph Pecoraro]

Comment on attachment 359129 [details]
Patch

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

r=me

> Source/WebCore/page/PerformanceObserver.cpp:123
> +        "resource"

Type inference should allow you to drop the explicit type here, and you can more efficiently construct the WTFStrings from literals with the `_s` suffix:

    return {
        // FIXME: <https://webkit.org/b/184363> Add support for Navigation Timing Level 2
        // "navigation"_s,
        "mark"_s,
        "measure"_s,
        "resource"_s
    };

Created attachment 359167 [details]
Patch

(In reply to Alex Christensen from comment #8)
> Comment on attachment 359129 [details]
> Patch
> 
> It would be nice if there were web platform tests verifying somehow that the
> entry types claimed to be supported are actually supported.

Good idea! Will import this[1] if/when I can get it to land.

[1] https://github.com/web-platform-tests/wpt/pull/14867.

Created attachment 359492 [details]
Patch

Created attachment 359518 [details]
Patch

Created attachment 359519 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 359519 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 359519 [details]
Patch

Clearing flags on attachment: 359519

Committed r240454: <https://trac.webkit.org/changeset/240454>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47532654>