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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 67227: patch
https://bugs.webkit.org/attachment.cgi?id=67227&action=review

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

Generally looks ambitious and pretty good. But the V8/JS part of this is taking
the wrong approach. An approach we are not taking anywhere else in WebKit that
is pretty heavyweight. Please look at ScriptFunctionCall and such.

> LayoutTests/platform/chromium/test_expectations.txt:2498
> +BUG40680 SKIP : fast/media/media-query-list-02.html = FAIL
> +BUG40680 SKIP : fast/media/media-query-list-03.html = FAIL
I don’t get it. Why are we skipping these tests at the same time we are adding
them?

> LayoutTests/platform/mac/Skipped:146
> +fast/media/media-query-list-02.html
> +fast/media/media-query-list-03.html
Same question.

> LayoutTests/platform/win/Skipped:854
> +fast/media/media-query-list-02.html
> +fast/media/media-query-list-03.html
Here too. Can we make these work instead of skipping them?

> WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:30
> +#include <JavaScriptCore/APICast.h>
> +#include <JavaScriptCore/JSValueRef.h>
It’s wrong to be including this here. WebCore doesn’t use the public
JavaScriptCore API.

> WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:55
> +    return JSValueIsEqual(toRef(exec), toRef(exec, m_data->callback()),
toRef(exec, jsListener->m_data->callback()), 0);
This should use JSValue::equal if weu’re trying to get the same result as
JavaScript "==". But is that really the right behavior of this? I think this is
strange behavior. Unprecedented even. Also, maybe we want "===" instead of
"=="?

I also don’t see why we need a special case for the null JSValue. Very
surprising. Is there a test case that covers this?

> WebCore/css/MediaQueryList.cpp:64
> +void MediaQueryList::removeListener(PassRefPtr<MediaQueryListListener>
listener)
A remove function usually should not take a PassRefPtr. A raw pointer would
make more sense.

> WebCore/css/MediaQueryList.cpp:82
> +    m_evalRound = m_matcher->evalRound();
> +    if (newValue != m_matches) {
> +	   m_matches = newValue;
> +	   m_changeRound = m_evalRound;
> +    }
The usual WebKit pattern is early return rather than nesting the work inside an
if statement.

> WebCore/css/MediaQueryList.h:41
> +/**
We don’t use that "**" thing.

> WebCore/css/MediaQueryList.h:43
> +  The objects of this class are retuned by window.matchMedia. They may be
used to
Typo: "retuned".

> WebCore/css/MediaQueryList.h:52
> +    static PassRefPtr<MediaQueryList> create(PassRefPtr<MediaQueryMatcher>,
PassRefPtr<MediaList>, bool);
> +    ~MediaQueryList();
> +    String media() const;
> +    bool matches();
Need a blank line here after the destructor.

Why isn’t matches a cost?

> WebCore/css/MediaQueryList.h:58
> +    bool changed();
This is a very confusing function name. What changed? Changed since when?
Should this be const?

> WebCore/css/MediaQueryListListener.h:36
> +  MediaQueryListListener is an abstraction of the callbacks.
No idea what that means.

But also you should not have to have two different types of listener at
runtime. We don’t have builds that mix JavaScript and V8. We do not want to use
virtual functions just to get to the code from different JavaScript engines.

> WebCore/css/MediaQueryMatcher.cpp:133
> +    for (size_t i = 0; i < m_listeners.size(); ++i) {
> +	   if (m_listeners[i]->listener()->isEqualTo(listener.get()) &&
m_listeners[i]->query() == query.get())
> +	       return;
> +    }
Is this really the needed behavior? Is a listener considered the same based on
the JavaScript language "==" operator?

I think the way we’re doing the abstraction of the different script engines
here is not good. Given that the need to accommodate both script engines is
needed the pattern of classes like ScriptValue should be used, rather than
having a custom runtime abstraction here.

> WebCore/css/MediaQueryMatcher.h:101
> +    static PassRefPtr<MediaQueryMatcher> create(Document* doc) { return
adoptRef(new MediaQueryMatcher(doc)); }
Please don’t abbreviate document as "doc".

> WebCore/css/MediaQueryMatcher.h:110
> +    int evalRound() const { return m_evalRound; }
There has to be a better name for this. I don’t think “eval round” is a good
term for it.

> WebCore/css/MediaQueryMatcher.h:130
> +private:
No reason to repeat private: here.

> WebCore/dom/Document.h:1319
> +    RefPtr<MediaQueryMatcher> m_mediaQueryMatcher;
Does this need to be owned by each document? Can it instead be global or
per-Page?