Bug 22495 – Implement MediaQuery evaluation API on the Window object

• Component: CSS
• Status: RESOLVED FIXED

[Dean Jackson]

implement the method for media query evaluation as defined by:
http://webkit.org/specs/MediaQueriesExtensions.html
<rdar://problem/6401013>

[Simon Fraser (smfr)]

Actually this is <rdar://problem/6401043>.

[Dean Jackson]

See http://dev.w3.org/csswg/cssom-view/#the-media-interface

Probably should implement that instead.

[Simon Fraser (smfr)]

Created attachment 34109 [details]
Work in progress

[Simon Fraser (smfr)]

Created attachment 34110 [details]
Test file

[Simon Fraser (smfr)]

Created attachment 34117 [details]
Patch, changelog, testcase

[Darin Adler]

Comment on attachment 34117 [details]
Patch, changelog, testcase

> + * 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

[Simon Fraser (smfr)]

Created attachment 34154 [details]
Revised patch

[Darin Adler]

Comment on attachment 34154 [details]
Revised patch

> +++ b/WebCore/css/Media.cpp
> @@ -0,0 +1,67 @@
> +/*
> + * Copyright (C) 2007, 2008 Apple Inc. All rights reserved.

Still no 2009 here. Why are there any other years listed? Isn't this newly-published code?

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

I would normally use a local variable for something like this to make the connection between the null check and the code below more clear.

> +    CSSStyleSelector* styleSelector = m_document->styleSelector();
> +    if (!styleSelector || !m_document->documentElement() || !document()->frame())
> +        return false;

Same comment.

> +    RefPtr<RenderStyle> rootStyle = styleSelector->styleForElement(m_document->documentElement(), 0/*defaultParent*/, false /*allowSharing*/, true /*resolveForRootDefault*/);

You should be consistent about whether there's a space before "/*" or not.

> +++ b/WebCore/css/Media.h
> @@ -0,0 +1,57 @@
> +/*
> + * Copyright (C) 2007, 2008 Apple Inc. All rights reserved.

Same copyright issue.

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

There are extra includes here. I'm sure Document.h includes PassRefPtr.h and RefCounted.h and perhaps it includes PlatformString.h as well.

Alternatively, you could implement an explicit destructor and then this header wouldn't need to include Document.h.

> + * Copyright (C) 2009 Apple Inc.  All rights reserved.

Extra space here after the period. Sorry, I am in super-picky mode.

Is it correct for the media object to be attached to a specific document, even though you get it from a DOMWindow object? What happens if you get the object and then navigate (within the same domain, so cross-site issues don't come up)?

No substantive complains, so r=me but please consider my comments and question.

[Simon Fraser (smfr)]

(In reply to comment #8)
> (From update of attachment 34154 [details])
> > +++ b/WebCore/css/Media.cpp
> > @@ -0,0 +1,67 @@
> > +/*
> > + * Copyright (C) 2007, 2008 Apple Inc. All rights reserved.
> 
> Still no 2009 here. Why are there any other years listed? Isn't this
> newly-published code?

Fixed.

> I would normally use a local variable for something like this to make the
> connection between the null check and the code below more clear.
> 
> Same comment.
> 
> You should be consistent about whether there's a space before "/*" or not.

All fixed.

> There are extra includes here. I'm sure Document.h includes PassRefPtr.h and
> RefCounted.h and perhaps it includes PlatformString.h as well.

Fixed.

> Is it correct for the media object to be attached to a specific document, even
> though you get it from a DOMWindow object? What happens if you get the object
> and then navigate (within the same domain, so cross-site issues don't come up)?

I changed Media to reference a DOMWindow, which solves this issue.

http://trac.webkit.org/changeset/46816

[Mark Rowe (bdash)]

I rolled this out in r46823 as it broke many tests for the audio and video elements that used "media" as a variable name in the global scope.

[Mark Rowe (bdash)]

Comment on attachment 34154 [details]
Revised patch

Marking as r- as it needs revision to address the test breakages.

[Dimitri Glazkov (Google)]

Revision landed as http://trac.webkit.org/changeset/46874, with V8 bindings build fix as http://trac.webkit.org/changeset/46894.