Bug 203288

Summary: MediaQueryList should extend EventTarget
Product: WebKit Reporter: Alexey Shvayka <ashvayka>
Component: CSSAssignee: Alexey Shvayka <ashvayka>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, benjamin, calvaris, cdumez, darin, dbates, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, josh, kangil.han, koivisto, kondapallykalyan, macpherson, menard, mjs, nkronlage, philipj, ryuan.choi, sam, sergio, simon.fraser, tomac, webkit-bug-importer, youennf, zcorpan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=105163
https://bugs.webkit.org/show_bug.cgi?id=210655
https://bugs.webkit.org/show_bug.cgi?id=211154
https://bugs.webkit.org/show_bug.cgi?id=213538
Bug Depends on: 204649, 209574    
Bug Blocks:    
Attachments:
Description Flags
Patch
sam: review-
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alexey Shvayka 2019-10-23 00:21:35 PDT
Initially, CSSOM View Module specification (https://www.w3.org/TR/2011/WD-cssom-view-20110804/#mediaquerylist) had a custom callback mechanism with addListener() and removeListener(), and the callback was invoked with the associated media query list as argument.
Now the normal event mechanism is used instead: https://www.w3.org/TR/cssom-view-1/#mediaquerylist.
For backwards compatibility, the addListener() and removeListener() methods are basically aliases for addEventListener() and removeEventListener(), respectively, and the "change" event masquerades as a MediaQueryList.

Other browsers have already shipped the change.
To do list for Safari:
1. Implement MediaQueryListEvent interface.
2. Make MediaQueryList subclass of EventTarget and emit MediaQueryListEvent.
3. Implement MediaQueryList::onchange attribute.
Comment 1 Radar WebKit Bug Importer 2019-11-21 19:25:32 PST
<rdar://problem/57417267>
Comment 2 Alexey Shvayka 2019-11-22 07:31:00 PST
Created attachment 384151 [details]
Patch
Comment 3 Alexey Shvayka 2019-11-22 07:43:29 PST
While this patch is ready for review, WPT tests are coming in https://github.com/web-platform-tests/wpt/pull/20401. I will re-sync wpt/css/cssom-view and rebase this patch as soon as it is merged.
Comment 4 Sam Weinig 2019-11-25 17:49:13 PST
Comment on attachment 384151 [details]
Patch

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

> Source/WebCore/css/MediaQueryList.cpp:60
> +    addEventListener("change", listener.releaseNonNull());

The IDL declares the listener parameter as optional, so I don't think you can remove the null check if you are going to keep this listener.releaseNonNull(). Same with removeListener below.
Comment 5 Alexey Shvayka 2020-01-09 05:15:17 PST
Created attachment 387214 [details]
Patch

Bring back null checks and adjust expectations.
Comment 6 Antti Koivisto 2020-01-10 00:58:44 PST
Comment on attachment 387214 [details]
Patch

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

Looks good in general.

> Source/WebCore/css/MediaQueryMatcher.cpp:116
> +    for (auto& mql : m_mediaQueryLists) {

WebKit coding style doesn't like abbreviations. This should be "mediaQueryList" or maybe just "list" if the context is clear. (the existing media query code is old and doesn't necessarily follow this)

> LayoutTests/imported/w3c/web-platform-tests/css/cssom-view/MediaQueryList-addListener-handleEvent-expected.txt:10
> +CONSOLE MESSAGE: line 90: [object Object]
>  
> -FAIL calls handleEvent method of event listener promise_test: Unhandled rejection with value: object "TypeError: Argument 1 ('listener') to MediaQueryList.addListener must be a function"
> -FAIL looks up handleEvent method on every event dispatch promise_test: Unhandled rejection with value: object "TypeError: Argument 1 ('listener') to MediaQueryList.addListener must be a function"
> -PASS doesn't look up handleEvent method on callable event listeners 
> -FAIL rethrows errors when getting handleEvent promise_rejects_exactly: function "function () { throw e }" threw object "TypeError: Argument 1 ('listener') to MediaQueryList.addListener must be a function" but we expected it to throw object "[object Object]"
> -PASS throws if handleEvent is falsy and not callable 
> -PASS throws if handleEvent is thruthy and not callable 
> +Harness Error (TIMEOUT), message = null
>  
> +PASS calls handleEvent method of event listener 
> +PASS looks up handleEvent method on every event dispatch 
> +PASS doesn't look up handleEvent method on callable event listeners 
> +TIMEOUT rethrows errors when getting handleEvent Test timed out
> +NOTRUN throws if handleEvent is falsy and not callable 
> +NOTRUN throws if handleEvent is thruthy and not callable 

Looks like one test started timing out and the rest are not run. It is not ok to rebase that.
Comment 7 Antti Koivisto 2020-03-13 04:13:40 PDT
Comment on attachment 387214 [details]
Patch

r- due to test timeouts. This looks pretty good though, it would be nice to get it landed.
Comment 8 Alexey Shvayka 2020-03-22 10:14:54 PDT
(In reply to Antti Koivisto from comment #7)
> Comment on attachment 387214 [details]
> Patch
> 
> r- due to test timeouts.

Thank you, Antti.

https://github.com/web-platform-tests/wpt/pull/21186 fixed TIMEOUT issue, yet the tests still remain false-positive.
https://github.com/web-platform-tests/wpt/pull/22384 fixes the tests.
I will sync WPT and update the patch as soon as it gets merged.
Comment 9 Alexey Shvayka 2020-03-27 08:38:16 PDT
Created attachment 394720 [details]
Patch

Rename `mql`, adjust tests, and tweak ChangeLog.
Comment 10 Darin Adler 2020-03-27 09:16:15 PDT
Comment on attachment 394720 [details]
Patch

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

Looks good. I am not sure we have enough test coverage for object lifetime issues.

> Source/WebCore/css/MediaQueryList.cpp:63
> +    addEventListener("change", listener.releaseNonNull());

eventNames().changeEvent rather than "change"

> Source/WebCore/css/MediaQueryList.cpp:71
> +    removeEventListener("change", *listener);

Ditto.

> Source/WebCore/css/MediaQueryList.h:26
>  #include <wtf/Forward.h>
>  #include <wtf/RefCounted.h>
>  #include <wtf/RefPtr.h>

None of these includes are needed now that we include EventTarget.h.

> Source/WebCore/css/MediaQueryList.h:43
> +    static Ref<MediaQueryList> create(ScriptExecutionContext&, MediaQueryMatcher&, Ref<MediaQuerySet>&&, bool matches);

I suggest this take a Document& instead of a ScriptExecutionContext&.

> Source/WebCore/css/MediaQueryList.h:58
> +    MediaQueryList(ScriptExecutionContext&, MediaQueryMatcher&, Ref<MediaQuerySet>&&, bool matches);

I suggest this take a Document& instead of a ScriptExecutionContext&.

> Source/WebCore/css/MediaQueryList.idl:26
> +    void addListener(EventListener? listener);
> +    void removeListener(EventListener? listener);

Is it important for compatibility that the listener argument be nullable? If not we should remove that because it’s peculiar and unnecessary.

> Source/WebCore/css/MediaQueryListEvent.h:29
> +#include <wtf/IsoMallocInlines.h>

I think these inlines might not be needed in the header if we moved the create functions out of the header. Not sure.

> Source/WebCore/css/MediaQueryListEvent.h:39
> +    static Ref<MediaQueryListEvent> create(const AtomString& type, const String& media, bool matches)
> +    {
> +        return adoptRef(*new MediaQueryListEvent(type, media, matches));
> +    }

I don’t think it’s valuable to have this inlined in the header. It would be better for runtime performance if the constructor was inlined in a non-inline create function, and better for compile speed to have less in the header.

> Source/WebCore/css/MediaQueryListEvent.h:42
> +        String media { "" };

emptyString(), not "", is more efficient

But also, is it important that media ia an empty string rather than a null string? Normally I’d expect a null string would suffice.

> Source/WebCore/css/MediaQueryListEvent.h:46
> +    static Ref<MediaQueryListEvent> create(const AtomString& type, const Init& init, IsTrusted isTrusted = IsTrusted::No)

Same thought about not inlining this.

> Source/WebCore/css/MediaQueryMatcher.cpp:118
> +        list->evaluate(evaluator, notify);

Since list is a weak pointer, it seems we would need to check for null here.

> Source/WebCore/css/MediaQueryMatcher.cpp:121
> +            auto event = MediaQueryListEvent::create(eventNames().changeEvent, list->media(), list->matches());
> +            list->dispatchEvent(event);

Do we get value from the local variable here? I think this would be better as a one-liner even if the line is a little longer.
Comment 11 Darin Adler 2020-03-27 09:50:09 PDT
Comment on attachment 394720 [details]
Patch

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

>> Source/WebCore/css/MediaQueryListEvent.h:42
>> +        String media { "" };
> 
> emptyString(), not "", is more efficient
> 
> But also, is it important that media ia an empty string rather than a null string? Normally I’d expect a null string would suffice.

Could answer this question with tests, I think. If null string doesn’t work we should have a test that would fail if this is a null string.

Or Chris Dumez might just know this answer. Surely this has come up in other structures that are part of DOM bindings.
Comment 12 Alexey Shvayka 2020-03-27 11:49:14 PDT
Created attachment 394740 [details]
Patch

Address Darin's comments.
Comment 13 Darin Adler 2020-03-27 12:13:11 PDT
Comment on attachment 394740 [details]
Patch

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

> Source/WebCore/css/MediaQueryListEvent.h:36
> +        String media { nullString() };

No need to write { nullString() } … that’s the default initialization for WTF::String.
Comment 14 Alexey Shvayka 2020-03-27 12:22:53 PDT
Created attachment 394746 [details]
Patch

Drop nullString() and add optional EventListener tests.
Comment 15 Alexey Shvayka 2020-03-27 12:33:41 PDT
(In reply to Darin Adler from comment #13)
> No need to write { nullString() } … that’s the default initialization for
> WTF::String.

Sorry!

Now MediaQueryListEvent::EventInit is consistent with other DOM bindings with its default values tested in web-platform-tests/css/cssom-view/MediaQueryListEvent.

(In reply to Darin Adler from comment #10)
> Is it important for compatibility that the listener argument be nullable? If
> not we should remove that because it’s peculiar and unnecessary.

Sadly it is required, both Chrome and Firefox accept .addListener(null | undefined), which I added tests for in fast/media/media-query-list-07.html. Zero argument .addListener() throwing TypeError is covered by web-platform-tests/css/cssom-view/idlharness.html.

Regarding test failures in web-platform-tests/css/cssom-view/MediaQueryList-addListener-handleEvent.html: those errors are visible in DevTools, but not caught correctly by "error" handler. It seemed like cross-realm issue at first, but it also reproduces in single-realm XPathNSResolver tests (web-platform-tests/domxpath/callback-interface.html). Chrome is also affected. I will open a bug as soon as I triage it a little more.
Comment 16 Alexey Shvayka 2020-04-03 09:41:56 PDT
*** Bug 156730 has been marked as a duplicate of this bug. ***
Comment 17 Alexey Shvayka 2020-04-10 08:01:43 PDT
Created attachment 396083 [details]
Patch

Rebase patch.
Comment 18 Alexey Shvayka 2020-04-13 10:21:51 PDT
(In reply to Darin Adler from comment #10)
> Comment on attachment 394720 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394720&action=review
> 
> Looks good. I am not sure we have enough test coverage for object lifetime
> issues.

Darin, could you please give a hint on how such a test may look like?
I am not sure MediaQueryList::~MediaQueryList() destructor is observable enough so it could be tested.
There are a few tests for wrappers like LayoutTests/crypto/crypto-gc.html, yet I am not positive we need one since MediaQueryList's wrapper is not special.
Is there something else we should do before landing?
Comment 19 Darin Adler 2020-04-13 19:52:22 PDT
(In reply to Alexey Shvayka from comment #18)
> Darin, could you please give a hint on how such a test may look like?
> I am not sure MediaQueryList::~MediaQueryList() destructor is observable
> enough so it could be tested.
> There are a few tests for wrappers like LayoutTests/crypto/crypto-gc.html,
> yet I am not positive we need one since MediaQueryList's wrapper is not
> special.
> Is there something else we should do before landing?

I don't think there is anything we have to do before landing.

Here is what I can think of regarding lifetime:

- If we add a listener to an object that also captures that same object, we need to make sure that the object and the listener can still both be garbage collected and there is no cycle. That's what fast/dom/reference-cycle-leaks.html tests for. But I don't think we need a special test for MediaQueryList since it's just using a normal event listener.

- If we add a listener to an object and all other references to the object are deleted, we need to make sure that the listener still fires when the event should be dispatched and that when it does the object's wrapper still has custom properties (not a new wrapper). An attempt to test for something like this is http/tests/xmlhttprequest/event-listener-gc.html but on review that particular test doesn't seem to be correctly written. Garbage collection is not consistent enough; there is no guarantee a single object will be collected if we only run the test once. I don't know if have good tests for this. I don’t know what keeps a MediaQueryList alive when there are no references to it any more, but its existence can still be observed indirectly by whether events fire or not. This is probably the test that should be written, and there may be a real bug here.

- Generally it's an anti-pattern to have a RefCounted object that is held in a collection in another object that removes itself when it's destroyed. That's because whether the object should be iterated or not shouldn't really be decided by whether it happens to have been destroyed or not. I can't see any practical problem with this, but if we could think of what might go wrong we'd write that kind of test.
Comment 20 Antti Koivisto 2020-04-17 03:02:36 PDT
Comment on attachment 396083 [details]
Patch

Sounds like everyone is ok to land this.
Comment 21 EWS 2020-04-17 03:16:40 PDT
Committed r260243: <https://trac.webkit.org/changeset/260243>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396083 [details].
Comment 22 Antti Koivisto 2020-04-17 04:48:28 PDT
Thanks for the patch!
Comment 23 Alexey Shvayka 2020-04-21 16:55:45 PDT
(In reply to Darin Adler from comment #19)

I appreciate the detailed write-up, thank you!
I will make sure to update http/tests/xmlhttprequest/event-listener-gc.html as soon as I get better understanding of GC design.

MQL seems to be kept alive by an event listener, same with other browsers:

```
;(onchange => {
  mql = matchMedia("(min-width: 600px)")
  mql.onchange = onchange
})(() => console.log("mql.onchange"))
```

> - Generally it's an anti-pattern to have a RefCounted object that is held in
> a collection in another object that removes itself when it's destroyed.

That is a great point, I will think through how we can decouple MediaQueryList from MediaQueryMatcher, or maybe set a flag in destructor and remove MQL from `matcher` during next evaluation round.

(In reply to Antti Koivisto from comment #22)
> Thanks for the patch!

Thank you for reviewing & landing this!