Bug 14455 - Autogenerate the JS bindings for the StyleSheetList
Summary: Autogenerate the JS bindings for the StyleSheetList
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 13779
  Show dependency treegraph
 
Reported: 2007-06-28 22:14 PDT by Sam Weinig
Modified: 2007-10-07 01:15 PDT (History)
1 user (show)

See Also:


Attachments
patch (24.28 KB, patch)
2007-06-28 22:33 PDT, Sam Weinig
darin: review-
Details | Formatted Diff | Diff
updated patch (25.24 KB, patch)
2007-08-18 00:04 PDT, Sam Weinig
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2007-06-28 22:14:16 PDT
Patch forthcoming.
Comment 1 Sam Weinig 2007-06-28 22:33:37 PDT
Created attachment 15307 [details]
patch
Comment 2 Darin Adler 2007-06-30 11:37:41 PDT
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?
Comment 3 Sam Weinig 2007-06-30 14:54:59 PDT
(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. 
Comment 4 Darin Adler 2007-06-30 17:47:09 PDT
(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.
Comment 5 Sam Weinig 2007-08-18 00:04:04 PDT
Created attachment 16012 [details]
updated patch
Comment 6 Nikolas Zimmermann 2007-08-19 13:06:26 PDT
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
Comment 7 Eric Seidel (no email) 2007-10-07 01:15:48 PDT
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.