Bug 209216 – PerformanceObserver should work with 'type' and not just `entryTypes`

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Philip Walton]

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

[Radar WebKit Bug Importer]

<rdar://problem/60611562>

[Rob Buis]

Created attachment 405287 [details]
Patch

[EWS Watchlist]

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

[Rob Buis]

Created attachment 405289 [details]
Patch

[Rob Buis]

Created attachment 405301 [details]
Patch

[youenn fablet]

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.

[Darin Adler]

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&

[Rob Buis]

Created attachment 405375 [details]
Patch

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Rob Buis]

Created attachment 405388 [details]
Patch

[EWS]

Committed r265001: <https://trac.webkit.org/changeset/265001>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405388 [details].

[Darin Adler]

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.

[youenn fablet]

(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.

[Darin Adler]

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".