[webkit-reviews] review granted: [Bug 203288] MediaQueryList should extend EventTarget : [Attachment 394720] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 203288: MediaQueryList should extend EventTarget
https://bugs.webkit.org/show_bug.cgi?id=203288

Attachment 394720: Patch

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




--- Comment #10 from Darin Adler <darin at apple.com> ---
Comment on attachment 394720
  --> https://bugs.webkit.org/attachment.cgi?id=394720
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.