[webkit-reviews] review granted: [Bug 22495] Implement MediaQuery evaluation API on the Window object : [Attachment 34154] Revised patch

[bugzilla-daemon at webkit.org]

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +++ 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.