Bug 97504 – Don't use StyleSheetList internally

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Antti Koivisto]

StyleSheetList is a DOM type and should not be used internally. Use plain Vector instead and construct StyleSheetList on DOM access only.

[Antti Koivisto]

Created attachment 165484 [details]
patch

[Ryosuke Niwa]

Comment on attachment 165484 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165484&action=review

> Source/WebCore/ChangeLog:10
> +        Also hot rid of the StyleSheetVector typedef as Vector<RefPtr<StyleSheet> > is less opaque and not much longer.

Typo: hot rid of

> Source/WebCore/dom/Document.cpp:3201
> +    if (!m_authorStyleSheetList)
> +        m_authorStyleSheetList = StyleSheetList::create(this);
> +    return m_authorStyleSheetList.get();

So now we wouldn't create StyleSheetList until it's used by scripts? I think that should be explained in the change log.
Doesn't this imply that if the author did
document.styleSheets.foo = 'bar';
gc(); // Force GC here.
alert(document.styleSheets.foo); // would give us "undefined"?

> Source/WebCore/dom/Document.h:1322
>      OwnPtr<DocumentStyleSheetCollection> m_styleSheetCollection;
> +    RefPtr<StyleSheetList> m_authorStyleSheetList;

This change of ownership of m_authorStyleSheetList should be explained in the change log.

> Source/WebCore/dom/DocumentStyleSheetCollection.cpp:461
> -    m_authorStyleSheets->swap(newStylesheets);
> +    m_authorStyleSheets = newStylesheets;

Why do we not want to swap?

[Ryosuke Niwa]

Comment on attachment 165484 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165484&action=review

>> Source/WebCore/dom/Document.cpp:3201
>> +    return m_authorStyleSheetList.get();
> 
> So now we wouldn't create StyleSheetList until it's used by scripts? I think that should be explained in the change log.
> Doesn't this imply that if the author did
> document.styleSheets.foo = 'bar';
> gc(); // Force GC here.
> alert(document.styleSheets.foo); // would give us "undefined"?

Ugh... nvm. Document has a RefPtr to StyleSheetList.

[Antti Koivisto]

(In reply to comment #2)

> So now we wouldn't create StyleSheetList until it's used by scripts? I think that should be explained in the change log.
> Doesn't this imply that if the author did
> document.styleSheets.foo = 'bar';
> gc(); // Force GC here.
> alert(document.styleSheets.foo); // would give us "undefined"?

No since document refs the StyleSheetList and always returns the same instance after initial  construction. There shouldn't be any externally observable changes.

> Why do we not want to swap?

Good point, I forgot Vector has swap() too.

[Antti Koivisto]

Created attachment 165508 [details]
updated patch

Updates based on comments

[Antti Koivisto]

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