Summary: | PerformanceObserver should work with 'type' and not just `entryTypes` | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philip Walton <philip> | ||||||||||||
Component: | DOM | Assignee: | 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
Philip Walton
2020-03-17 19:45:50 PDT
Created attachment 405287 [details]
Patch
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 Created attachment 405289 [details]
Patch
Created attachment 405301 [details]
Patch
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 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& Created attachment 405375 [details]
Patch
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Created attachment 405388 [details]
Patch
Committed r265001: <https://trac.webkit.org/changeset/265001> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405388 [details]. 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. (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 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". |