[webkit-reviews] review granted: [Bug 37205] Implement CSSOM View matchMedia interface : [Attachment 74474] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Luiz Agostini <luiz at webkit.org>'s
request for review:
Bug 37205: Implement CSSOM View matchMedia interface
https://bugs.webkit.org/show_bug.cgi?id=37205

Attachment 74474: patch
https://bugs.webkit.org/attachment.cgi?id=74474&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=74474&action=review

> WebCore/ChangeLog:54
> +	   publishes methods matchMedia but for page cache to work properly 
the reference to the

Stray space here between in “ properly	the reference”.

> WebCore/css/MediaQueryList.cpp:96
> +	   setMatches(m_matcher->evaluate(m_media.get()));

A comment I made last time: I don’t think the factoring here is quite optimal.
I’d name the helper function updateMatches instead of setMatches, and have it
do the m_matcher->evaluate call as well.

> WebCore/css/MediaQueryList.h:54
> +    void addListener(PassRefPtr<MediaQueryListListener>);
> +    void removeListener(PassRefPtr<MediaQueryListListener>);

I understand why the addListener function takes a PassRefPtr, but the
removeListener function should not.

> WebCore/css/MediaQueryMatcher.cpp:125
> +	   if (*m_listeners[i]->listener() == *listener.get() &&
m_listeners[i]->query() == query.get())

Do we need to say get() here? I think we could remove both calls to get() and
the code would still compile and work fine.

> WebCore/css/MediaQueryMatcher.cpp:137
> +    for (int i = m_listeners.size() - 1; i >= 0; --i) {

Why iterate backwards? This unnecessarily truncates a size_t to an int, and
since we always stop as soon as we find what we are looking for, there is no
benefit to iterating in a certain direction.

> WebCore/css/MediaQueryMatcher.cpp:138
> +	   if (*m_listeners[i]->listener() == *listener.get() &&
m_listeners[i]->query() == query.get()) {

Do we need to say get() here? I think we could remove both calls to get() and
the code would still compile and work fine.

> WebCore/css/MediaQueryMatcher.cpp:149
> +    ScriptState* exec = mainWorldScriptState(m_document->frame());

I don’t think “exec” is good name. How about “state” or “scriptState”?

> WebCore/css/MediaQueryMatcher.h:54
> +    void addListener(PassRefPtr<MediaQueryListListener>,
PassRefPtr<MediaQueryList>);
> +    void removeListener(PassRefPtr<MediaQueryListListener>,
PassRefPtr<MediaQueryList>);

I understand why the addListener command takes PassRefPtr, but the
removeListener command should not.

> WebCore/page/DOMWindow.idl:146
> +	   // styleMedia has been removed from the cssom-view specification.
>	   readonly attribute StyleMedia styleMedia;
> +	   MediaQueryList matchMedia(in DOMString query);

That comment is only about styleMedia, so it should be paragraphed with
styleMedia and not with matchMedia; you could put matchMedia first in the file
and add a blank line.

Also, “cssom-view specification” doesn’t look quite right in all lower case.