Bug 193428 - Implement PerformanceObserver.supportedEntryTypes
Summary: Implement PerformanceObserver.supportedEntryTypes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-14 20:18 PST by cvazac
Modified: 2019-01-24 16:05 PST (History)
10 users (show)

See Also:


Attachments
Patch (4.69 KB, patch)
2019-01-14 20:19 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (5.06 KB, patch)
2019-01-14 20:38 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (5.11 KB, patch)
2019-01-14 20:55 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (5.13 KB, patch)
2019-01-14 20:59 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (77.71 KB, patch)
2019-01-15 09:37 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (88.10 KB, patch)
2019-01-18 08:34 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (19.88 KB, patch)
2019-01-18 12:10 PST, cvazac
no flags Details | Formatted Diff | Diff
Patch (19.34 KB, patch)
2019-01-18 12:13 PST, cvazac
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description cvazac 2019-01-14 20:18:32 PST
Implement PerformanceObserver.supportedEntryTypes
Comment 1 cvazac 2019-01-14 20:19:25 PST
Created attachment 359123 [details]
Patch
Comment 2 Build Bot 2019-01-14 20:22:52 PST
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.
Comment 3 cvazac 2019-01-14 20:38:05 PST
Created attachment 359125 [details]
Patch
Comment 4 Joseph Pecoraro 2019-01-14 20:43:15 PST
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.
Comment 5 Joseph Pecoraro 2019-01-14 20:53:07 PST
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.
Comment 6 cvazac 2019-01-14 20:55:25 PST
Created attachment 359128 [details]
Patch
Comment 7 cvazac 2019-01-14 20:59:15 PST
Created attachment 359129 [details]
Patch
Comment 8 Alex Christensen 2019-01-14 20:59:50 PST
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.
Comment 9 Joseph Pecoraro 2019-01-14 21:03:52 PST
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
    };
Comment 10 cvazac 2019-01-15 09:37:46 PST
Created attachment 359167 [details]
Patch
Comment 11 cvazac 2019-01-15 09:38:23 PST
(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.
Comment 12 cvazac 2019-01-18 08:34:08 PST
Created attachment 359492 [details]
Patch
Comment 13 cvazac 2019-01-18 12:10:10 PST
Created attachment 359518 [details]
Patch
Comment 14 cvazac 2019-01-18 12:13:10 PST
Created attachment 359519 [details]
Patch
Comment 15 Joseph Pecoraro 2019-01-18 12:59:38 PST
Comment on attachment 359519 [details]
Patch

r=me
Comment 16 WebKit Commit Bot 2019-01-24 16:04:58 PST
Comment on attachment 359519 [details]
Patch

Clearing flags on attachment: 359519

Committed r240454: <https://trac.webkit.org/changeset/240454>
Comment 17 WebKit Commit Bot 2019-01-24 16:05:00 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2019-01-24 16:05:32 PST
<rdar://problem/47532654>