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

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

This looks great. I don’t understand the round trip from document to frame and
then back to document, and I’m worried that it might even possibly create a
security bug. So please fix at least that.

> WebCore/bindings/js/ScriptValue.cpp:91
> +    CallType callType = getCallData(m_value, callData);
> +    return callType != CallTypeNone;

No need for the callType local variable here.

> WebCore/bindings/js/ScriptValue.h:61
> +    bool isFunction() const;
> +    bool operator==(const ScriptValue& other) const { return m_value ==
other.m_value; }

A little strange to group these two functions together in a paragraph.
isFunction seems to belong with isObject, while operator== perhaps should be
off on its own.

> WebCore/bindings/scripts/CodeGeneratorGObject.pm:217
> +	       $param->type eq "MediaQueryListListener") {

Always a bad sign when we have to special case a new type.

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

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:29
> +#include <wtf/PassRefPtr.h>

No need to include the PassRefPtr header here. Instead you could include
Forward.h.

> WebCore/css/MediaQueryList.h:32
> +#include <wtf/text/WTFString.h>

No need to include the WTFString header here. Instead you could include
Forward.h.

> WebCore/css/MediaQueryList.h:44
> +//  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.

No need for the double spaces at the beginnings of lines here.

> WebCore/css/MediaQueryList.h:66
> +    int m_evaluationRound; // Indicates if the query has been evaluated
after the last style selector change.
> +    int m_changeRound; // Used to know if the query has changed in the last
style selector change.

I think unsigned would be better than int.

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

No need for double spaces here.

> WebCore/css/MediaQueryListListener.h:50
> +    bool isEqualTo(const MediaQueryListListener*);

Why not use operator== for this instead of a named function?

> WebCore/css/MediaQueryMatcher.cpp:90
> +    Document* document = m_document->frame()->document();
> +    if (!document)
> +	   return 0;

No need to null check the document. All frames have documents.

I don’t understand why you go from the document to the frame and then back to
the document.

> WebCore/css/MediaQueryMatcher.cpp:126
> +    if (!m_document || !m_document->frame())
> +	   return;

Why check frame against 0? Seems unnecessary.

> WebCore/css/MediaQueryMatcher.cpp:138
> +    if (!m_document || !m_document->frame())

Why check frame against 0? Seems unnecessary.

> WebCore/css/MediaQueryMatcher.h:34
> +#include "ScriptState.h"
> +
> +#include <wtf/PassRefPtr.h>
> +#include <wtf/RefCounted.h>
> +#include <wtf/Vector.h>
> +#include <wtf/text/WTFString.h>

Includes go in a single paragraph, not two.

No need to include PassRefPtr.h and WTFString.h. Instead you should use
Forward.h.

> WebCore/css/MediaQueryMatcher.h:47
> +//  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.

No need for those extra spaces at the beginning of each comment line.

> WebCore/css/MediaQueryMatcher.h:49
> +class MediaQueryMatcher : public RefCounted<MediaQueryMatcher> {

This seems to be an object with a single owner, the document. And nothing else
seems to use a RefPtr on it. Accordingly, I suggest making it be a single-owner
object, not a reference counted one.

> WebCore/css/MediaQueryMatcher.h:60
> +    int evaluationRound() const { return m_evaluationRound; }

I think unsigned would be better than int. To be pedantic, unsigned has well
defined overflowing semantics, whereas int does not.

> WebCore/css/MediaQueryMatcher.h:90
> +    int m_evaluationRound;

I think unsigned would be better than int.