[webkit-reviews] review denied: [Bug 22429] document.styleSheets collection ignores media=presentation : [Attachment 26014] First attempt

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Rob Buis <rwlbuis at gmail.com>'s request
for review:
Bug 22429: document.styleSheets collection ignores media=presentation
https://bugs.webkit.org/show_bug.cgi?id=22429

Attachment 26014: First attempt
https://bugs.webkit.org/attachment.cgi?id=26014&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   document.styleSheets collection ognores media=presentation

"ognores"

> +	       if (m_sheet)
> +		   m_sheet = 0;

The above is a strange way to write "m_sheet = 0".

But also, is there a reason we need to set m_sheet to 0 separately before
assigning a new value? Sometimes we have done that intentionally in the past
when there was a need to guarantee that the old ref went away before the new
object was created. But I think in this case that code should just be omitted.

> +	       // create a dummy sheet without url and content

I'd prefer to see new comments be written as full sentences. I'd write it like
this:

    // Create a dummy sheet without a URL or content.

> +	       m_sheet = CSSStyleSheet::create(this, m_url, chset);
> +	       m_sheet->setTitle(title());
> +	       m_sheet->setMedia(media.get());

This should be media.release(), not media.get().

Is an empty CSSStyleSheet really what's called for here? Don't we need a style
sheet with a DOM that scripts can look at? Why is an empty one OK?

> +	       document()->updateStyleSelector();

It's a shame to call updateStyleSelector in this case. There are no new style
rules, and there's no need to do work unless there was a non-empty style sheet
before that we're replacing. It would be better to treat an empty style sheet
the same as lack of a style sheet and call updateStyleSelector only when it is
really needed. We don't want to do needless style computation when you add a
<link> for a style sheet that fails the media query.

I'm going to say review- because I'd like to see a justification of the value
of creating an empty style sheet. I really don't understand why it's correct to
not load the style sheet and create an empty style sheet object. Is this what
other browsers do? Or do they load these style sheets?

I'd like to hear Hyatt's thoughts on this too.