[webkit-reviews] review denied: [Bug 37205] Implement CSSOM View matchMedia interface : [Attachment 69310] 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 69310: patch
https://bugs.webkit.org/attachment.cgi?id=69310&action=review

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

Looks good. Still some work to be done I think.

> WebCore/bindings/scripts/CodeGeneratorJS.pm:289
> +    if ($type eq "MediaQueryListListener") {
> +	   $implIncludes{"MediaQueryListListener.h"} = 1;
> +    }

It’s really disappointing that this is necessary. Adding a new type to the
bindings generator is a lot of extra work.

Specifically, you need to add test cases to the bindings tests. Take a look at
the run-bindings-tests script, the test cases in WebCore/bindings/scripts/test,
and the expected test results in the subdirectories of that directory. You’ll
need to add test cases, and update the expected test results.

> WebCore/css/MediaQueryList.cpp:44
> +    , m_evalRound(m_matcher->evaluationRound())

If the full name is evaluationRound, then I suggest m_evaluationRound rather
than m_evalRound.

> WebCore/css/MediaQueryList.h:46
> +/*
> +  MediaQueryList interface is specified at
http://dev.w3.org/csswg/cssom-view/#the-mediaquerylist-interface
> +  The objects of this class are returned by window.matchMedia. They may be
used to
> +  retrieve the current value of the given media query and to add/remove
listeners that
> +  will be called whenever the value of the query changes.
> +*/

In the WebKit project we normally use // comments, not /* */ comments, except
for the license comment.

> WebCore/css/MediaQueryList.h:50
> +    ~MediaQueryList();

Why do we need to explicitly define the destructor? Did you try with the
default constructor.

> WebCore/css/MediaQueryListListener.cpp:33
> +#if USE(JSC)
> +#include "JSMediaQueryList.h"
> +#else
> +#include "V8MediaQueryList.h"
> +#endif

Includes inside an #if normally go in their own paragraph separate from the
main one.

> WebCore/css/MediaQueryListListener.cpp:53
> +bool MediaQueryListListener::isEqualTo(ScriptState* state, const
MediaQueryListListener* other)
> +{
> +    return other && other->m_value.isEqual(state, m_value);
> +}

The use of JavaScript equality to define listener equality is extremely
unusual; other listeners use object identity, not object equality, which is
both more efficient and never involves running arbitrary JavaScript, while
isEqualTo can involve running script. We need test cases covering this, and
clarifying that it’s equality and not strict equality. I don’t see coverage in
the tests you added.

> WebCore/css/MediaQueryListListener.h:41
> +/*
> +  See http://dev.w3.org/csswg/cssom-view/#the-mediaquerylist-interface
> +*/

Comment should be "//" style.

> WebCore/css/MediaQueryListListener.h:52
> +    MediaQueryListListener(ScriptValue value) : m_value(value) {}

WebKit style is to use a space inside the {}.

> WebCore/css/MediaQueryListListener.idl:33
> +	   void queryChanged(in MediaQueryList mql);

Please use the word “list” here rather than “mql”; we use words rather than
acronyms when possible.

> WebCore/css/MediaQueryMatcher.cpp:138
> +    for (size_t i = 0; i < m_listeners.size(); ++i) {
> +	   if (m_listeners[i]->listener()->isEqualTo(exec, listener.get()) &&
m_listeners[i]->query() == query.get())
> +	       return;
> +    }

This code can reenter because a JavaScript equality comparison can involve
running arbitrary script. We need to add some test cases covering that. If we
do this wrong we could end up adding a security vulnerability. I doubt very
much that the JavaScript equality rule is a good one.

> WebCore/css/MediaQueryMatcher.h:100
> +/*
> +  MediaQueryMatcher class is responsible for keeping a vector of pairs
> +  MediaQueryList x MediaQueryListListener. It is responsible for evaluating
the queries
> +  whenever it is needed and to call the listeners if the corresponding query
has changed.
> +  The listeners must be called in the very same order in which they have
been added.
> +
> +  Object diagram:
> +
> +
> +							   
MediaQueryListListener
> +								       ^
> +								       |
> +								       |
> +	Document -------->  MediaQueryMatcher ------------------>  Listener
> +				    ^				*      |
> +				    |				       |
> +				    |				       |
> +				    +------  MediaQueryList  <---------+
> +						   ^
> +						   |
> +						   |
> +					    JSMediaQueryList
> +
> +  * All arrows in this diagram, except the one between MediaQueryMatcher and
Listener,
> +    are RefPtr<>.
> +
> +  Objects lifecycle:
> +
> +  No reference to MediaQueryList instances is kept by class
MediaQueryMatcher itself. The reason
> +  is that if MediaQueryList::addListener is not called the only reference to
the MediaQueryList
> +  object will be in Javascript and it will be destroyed as soon as the JS
object is garbage
> +  collected.
> +
> +  Class MediaQueryList does not keep a list of MediaQueryListListener. It
calls
> +  MediaQueryMatcher::addListener every time its addListener method is
called, passing the
> +  received MediaQueryListListener object and itself.
MediaQueryMatcher::addListener will
> +  then append a new MediaQueryMatcher::Listener to its internal vector, if
the pair
> +  MediaQueryListListener x MediaQueryList is not duplicated.
> +
> +  This new Listener object has a reference to the MediaQueryList object,
that will keep
> +  it alive to be passed to the callback. If all listeners are removed from
that
> +  MediaQueryList object, all Listener instances pointing to that
MediaQueryList object will be
> +  destroyed and again the only reference to the query will be in JS.
> +
> +  It may be that a MediaQueryMatcher instance has one or more Listener
instances pointing to a
> +  MediaQueryList and this MediaQueryList has a reference to that same
MediaQueryMatcher instance,
> +  preventing them from been destroyed (the cycle in the diagram). That is
why the
> +  MediaQueryMatcher object cannot be owned by the Document instance and an
special termination
> +  handling is required.
> +
> +  Method MediaQueryMatcher::terminate, called by Document just before
releasing the
> +  MediaQueryMatcher reference, will remove all references to MediaQueryList
objects in
> +  MediaQueryMatcher and will guarantee that no new one will be created. The
only references to
> +  MediaQueryList objects will be again in JS. All MediaQueryList objects
will be destroyed as soon
> +  as their corresponding JS objects get garbage collected and
MediaQueryMatcher will die as soon
> +  as the last MediaQueryList object dies.
> +*/

There are many who would applaud you for writing extensive documentation. I am
concerned that this is a long explanation which may quickly get out of sync
with the code. A lot of this is worded in a way that I find confusing, with
hypothetical situations that I don’t completely understand.

> WebCore/css/MediaQueryMatcher.h:104
> +    ~MediaQueryMatcher();

Once you change m_listeners, you probably won’t need an explicit destructor
declaration.

> WebCore/css/MediaQueryMatcher.h:120
> +	   ~Listener();

You don’t need this explicit destructor declaration.

> WebCore/css/MediaQueryMatcher.h:137
> +    Vector<Listener*> m_listeners;

This should be a Vector<OwnPtr<Listener> >. Making it use that type will allow
you to remove much of the code.

> WebCore/css/MediaQueryMatcher.h:140
> +			      // and to avoid notifing a change more then once.


This long comment is indented in a way we normally don’t do. Also, I don’t know
what “notifing a change” means.

> WebCore/dom/Document.cpp:572
> +    if (m_mediaQueryMatcher)
> +	   m_mediaQueryMatcher->terminate();

I would call this function documentDestroyed rather than terminate.

> WebCore/dom/Document.cpp:2855
> +    if (m_mediaQueryMatcher)
> +	   m_mediaQueryMatcher->evaluate();

This doesn’t seem right to me. I don’t understand why the operation is named
evaluate, nor why styleSelectorChanged is the right time to do it. It seems to
me we don’t want to do any new evaluation until someone calls matchMedia.

> WebCore/dom/Document.h:203
> +    PassRefPtr<MediaQueryList> matchMedia(const String&);

I think the document class should just have an accessor to return the media
query matcher object. There’s no need to have the matchMedia forwarding
function here. The code in DOMWindow can handle that part.

> WebCore/page/DOMWindow.idl:144
> +	   readonly attribute StyleMedia styleMedia; // FIXME: deprecated

This is a confusing comment. Please don’t write that in the .idl file. If you
are saying FIXME, then I presume you are recommending we remove this from
WebKit. The comment should explain the circumstances where we would remove it.
Simply saying deprecated is far too vague. It doesn’t say who deprecated it or
why it wasn’t simply removed.