[Webkit-unassigned] [Bug 22429] document.styleSheets collection ignores media=presentation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=22429





------- Comment #4 from rwlbuis at gmail.com  2008-12-15 13:44 PDT -------
(In reply to comment #3)
> (From update of attachment 26014 [review])
> > +        document.styleSheets collection ognores media=presentation
> 
> "ognores"

Right, I didn't want to touch the original title, but I guess it is better to
fix it.

> > +            if (m_sheet)
> > +                m_sheet = 0;
> 
> The above is a strange way to write "m_sheet = 0".

Right. I first had some logic in the if to set a bool to decide whether
updateStyleSelector should be called.

> 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.

Possible, I am not sure.

> > +            // 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.

I'll remember for future comments.

> > +            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().

Ok.

> 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?

I sort of did that because of efficiency. But it turns out FF and Opera still
load stylesheets
for links with invalid/uncommon media attributes.

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

Agreed.

> 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?

Right, FF and Opera do load them.
I did a new patch that does load and a start of a testcase, I'll upload what I
have right now.
Cheers,

Rob.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.