Bug 14455 – Autogenerate the JS bindings for the StyleSheetList

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Sam Weinig]

Patch forthcoming.

[Sam Weinig]

Created attachment 15307 [details]
patch

[Darin Adler]

Comment on attachment 15307 [details]
patch

+    HTMLStyleElement* element = thisObj->impl()->getNamedItem(propertyName);
+    return toJS(exec, element->sheet());

Looks like this won't do the right thing if element is 0. Maybe a pre-existing problem?

-    // IE also supports retrieving a stylesheet by name, using the name/id of the <style> tag
-    // (this is consistent with all the other collections)
-    // ### Bad implementation because returns a single element (are IDs always unique?)
-    // and doesn't look for name attribute (see implementation above).
-    // But unicity of stylesheet ids is good practice anyway ;)

Where did all these comments go?

+HTMLStyleElement* StyleSheetList::getNamedItem(const String& name) const
+{
+    Element* element = m_doc->getElementById(name);

What guarantees the document in m_doc has not yet been destroyed?

[Sam Weinig]

(In reply to comment #2)
> (From update of attachment 15307 [details] [edit])
> +    HTMLStyleElement* element = thisObj->impl()->getNamedItem(propertyName);
> +    return toJS(exec, element->sheet());
> 
> Looks like this won't do the right thing if element is 0. Maybe a pre-existing
> problem?

This will never be called if element is 0 as it is only called after the canGetItemsForName() function which checks.

> -    // IE also supports retrieving a stylesheet by name, using the name/id of
> the <style> tag
> -    // (this is consistent with all the other collections)
> -    // ### Bad implementation because returns a single element (are IDs always
> unique?)
> -    // and doesn't look for name attribute (see implementation above).
> -    // But unicity of stylesheet ids is good practice anyway ;)
> 
> Where did all these comments go?

Oops, forgot to move that, fixing it.

> +HTMLStyleElement* StyleSheetList::getNamedItem(const String& name) const
> +{
> +    Element* element = m_doc->getElementById(name);
> 
> What guarantees the document in m_doc has not yet been destroyed?

Now, I am not 100% sure, but since Document is the only one that creates and the StyleSheetList and stores it and it should be deleted when it is destroyed this should not be an issue. 

[Darin Adler]

(In reply to comment #3)
> This will never be called if element is 0 as it is only called after the
> canGetItemsForName() function which checks.

I'd like to see a comment and perhaps an assertion to make that clear.

> > +HTMLStyleElement* StyleSheetList::getNamedItem(const String& name) const
> > +{
> > +    Element* element = m_doc->getElementById(name);
> > 
> > What guarantees the document in m_doc has not yet been destroyed?
> 
> Now, I am not 100% sure, but since Document is the only one that creates and
> the StyleSheetList and stores it and it should be deleted when it is destroyed
> this should not be an issue. 

Unfortunately, that's not true. If some JavaScript code gets a reference to a StyleSheetList and then the document goes away, the JavaScript wrapper will keep the StyleSheetList alive. We need to add code to either ref the document (and make sure we don't create any circular references) or to zero out the reference to the document when the document is deleted.

[Sam Weinig]

Created attachment 16012 [details]
updated patch

[Nikolas Zimmermann]

Comment on attachment 16012 [details]
updated patch

Looks fine, r=me.
Don't forget to readd the comments though (regarding IE supports..) before comitting :-)

Greetings,
Niko

[Eric Seidel (no email)]

I added the comment back to the implementation and fixed the project file.  Then I landed this as r26094 on the feature-branch.  I expect that Wenig won't mind.  Just cleaning out the commit queue.