[webkit-reviews] review denied: [Bug 22495] Implement MediaQuery evaluation API on the Window object : [Attachment 34117] Patch, changelog, testcase

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 22495: Implement MediaQuery evaluation API on the Window object
https://bugs.webkit.org/show_bug.cgi?id=22495

Attachment 34117: Patch, changelog, testcase
https://bugs.webkit.org/attachment.cgi?id=34117&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> + * Copyright (C) 2007, 2008 Apple Inc. All rights reserved.

What about 2009?

> +#include "config.h"
> +
> +#include "FrameView.h"
> +#include "Media.h"
> +#include "MediaList.h"
> +#include "MediaQueryEvaluator.h"
> +#include <wtf/RefPtr.h>

The first include after "config.h" always needs to be that file's own header,
in this case Media.h. I don't think all of these includes are needed. For
example, I'm sure the header already takes care of RefPtr.h. Maybe that's the
only unneeded one.

> +String Media::type() const
> +{
> +    if (FrameView* view = m_document->view())
> +	   return view->mediaType();
> +
> +    return String();
> +}

Even in a short function like this one I prefer to use early exit for the
failure case. But I know that gets in the way of scoping the "view" local
variable. I don't feel too strongly about this.

> +bool Media::matchMedium(const String& query)
> +{
> +    MediaQueryEvaluator screenEval(type(), document()->frame(), 0);

What about when frame() is 0? What will happen in that case? Did you make a
test to cover that?

Since you enhanced MediaQueryEvaluator to work without a RenderStyle*, then I
think you need to overload the constructor to not require one, or use a default
argument. The use of "0" here is unnecessarily confusing.

> +    RefPtr<MediaList> media = MediaList::create(query, false);

Oh how I hate boolean arguments. Wonder what that false means.

> +    if (media->mediaText() == "invalid")
> +	   return false;

Is there some better way to test for invalid than a string compare? This seems
weak.

> +#include "Document.h"
> +#include <wtf/PassRefPtr.h>
> +#include <wtf/RefCounted.h>
> +#include <wtf/RefPtr.h>
> +#include "PlatformString.h"

You don't need all these includes, and they're not sorted properly.

> +class Media : public RefCounted<Media> {
> +public:
> +    static PassRefPtr<Media> create(Document* document)
> +    {
> +	   return adoptRef(new Media(document));
> +    }
> +
> +    Document* document() const { return m_document; }
> +
> +    String type() const;
> +
> +    bool matchMedium(const String&);
> +    
> +private:
> +    Media(Document*);
> +
> +    Document* m_document;
> +
> +};

There is an extra blank line here.

Why is it OK here to use a raw pointer? Doesn't the Media need to ref-count the
Document object? What prevents it from outlasting the document?

> +	   RefPtr<RenderStyle> style = m_style;
> +	   if (!style)
> +	       style = RenderStyle::create();

This gets slightly more efficient if you use ? : I think, with a bit less
reference count churn:

    RefPtr<RenderStyle> style = m_style ? m_style : RenderStyle::create();

Does that compile?

I really do prefer to make the error case be the early out, so you should
reverse that if and put the return false earlier.

review- because of the document lifetime issue, at least