Bug 209216

Summary: PerformanceObserver should work with 'type' and not just `entryTypes`
Product: WebKit Reporter: Philip Walton <philip>
Component: DOMAssignee: Noam Rosenthal <noam>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, clopez, darin, esprehn+autocc, ews-watchlist, kondapallykalyan, noam, rbuis, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Safari 13   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Philip Walton 2020-03-17 19:45:50 PDT
** Steps to reproduce: **

1. Navigate to example.com (or any web page) in Safari and run the following code in the console:

```
new PerformanceObserver((list) => {
  for (const entry of list.getEntries()) {
    console.log(entry);
  }
}).observe({type: 'resource'});
```

2. Notice that the following error is thrown:

TypeError: Member PerformanceObserverInit.entryTypes is required and must be an instance of sequence


** What should happen: **

No error should be thrown if the `type` options is used instead of `entryTypes`.


** Justification: **

The WebIDL for PerformanceObseverInit [1] shows that it accepts either an `entryTypes` option OR a `type` option:

```
dictionary PerformanceObserverInit {
  sequence<DOMString> entryTypes;
  DOMString type;
  boolean buffered;
};
```

[1] https://w3c.github.io/performance-timeline/#dom-performanceobserverinit
Comment 1 Radar WebKit Bug Importer 2020-03-18 17:56:22 PDT
<rdar://problem/60611562>
Comment 2 Rob Buis 2020-07-27 09:36:46 PDT
Created attachment 405287 [details]
Patch
Comment 3 EWS Watchlist 2020-07-27 09:37:34 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 4 Rob Buis 2020-07-27 10:35:32 PDT
Created attachment 405289 [details]
Patch
Comment 5 Rob Buis 2020-07-27 11:57:07 PDT
Created attachment 405301 [details]
Patch
Comment 6 youenn fablet 2020-07-28 09:02:54 PDT
Comment on attachment 405301 [details]
Patch

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

> Source/WebCore/page/PerformanceObserver.cpp:68
> +        for (const String& entryType : *init.entryTypes) {

const auto&

> Source/WebCore/page/PerformanceObserver.cpp:-74
> -

You could replace the two above if by:
if (filter.isEmpty())
    return { };
That would allow to remove validEntryTypes for instance.

> LayoutTests/imported/w3c/web-platform-tests/resource-timing/buffered-flag.any.worker-expected.txt:2
> +Harness Error (TIMEOUT), message = null

Do you know why all these tests are timing out?
Since they are timing out, we should probably skip them in TestExpectations.
Comment 7 Darin Adler 2020-07-28 09:59:27 PDT
Comment on attachment 405301 [details]
Patch

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

>> Source/WebCore/page/PerformanceObserver.cpp:68
>> +        for (const String& entryType : *init.entryTypes) {
> 
> const auto&

I’d go further and suggest just auto&
Comment 8 Rob Buis 2020-07-28 10:26:17 PDT
Created attachment 405375 [details]
Patch
Comment 9 EWS 2020-07-28 12:11:11 PDT
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
Comment 10 Rob Buis 2020-07-28 12:15:33 PDT
Created attachment 405388 [details]
Patch
Comment 11 EWS 2020-07-28 13:03:14 PDT
Committed r265001: <https://trac.webkit.org/changeset/265001>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405388 [details].
Comment 12 Darin Adler 2020-07-28 14:08:26 PDT
Comment on attachment 405388 [details]
Patch

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

> Source/WebCore/page/PerformanceObserver.cpp:69
> +                filter.add(*type);

Could remove a little bit of churn with WTFMove.

> Source/WebCore/page/PerformanceObserver.cpp:77
>              filter.add(*type);

Could remove a little bit of churn with WTFMove.
Comment 13 youenn fablet 2020-07-28 14:50:02 PDT
(In reply to Darin Adler from comment #12)
> Comment on attachment 405388 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405388&action=review
> 
> > Source/WebCore/page/PerformanceObserver.cpp:69
> > +                filter.add(*type);
> 
> Could remove a little bit of churn with WTFMove.
> 
> > Source/WebCore/page/PerformanceObserver.cpp:77
> >              filter.add(*type);
> 
> Could remove a little bit of churn with WTFMove.

Probably not, type is an Optional of an enum and filter is an OptionSet.
Comment 14 Darin Adler 2020-07-28 14:51:50 PDT
Comment on attachment 405388 [details]
Patch

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

>>> Source/WebCore/page/PerformanceObserver.cpp:69
>>> +                filter.add(*type);
>> 
>> Could remove a little bit of churn with WTFMove.
> 
> Probably not, type is an Optional of an enum and filter is an OptionSet.

Got it. I thought it was a string, not a "parsed string".