244258 - deCOMtaminate nsIStyledContent

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

GetID (once bug 244249 is fixed), GetAttributeChangeHint, and GetInlineStyleRule
could all use deCOMtamination.

Comment 1

[jpl24]

Attachment: decom_patch

First cut at decomtaminating GetAttributeChangeHint, and GetInlineStyleRule.

Comment 2

[jpl24]

Boris do you mind having a look at this and seeing if this is what you had in
mind? It's my first patch, so it may need some improvement. 

If you can give me a nudge in the right direction I might be able to figure out
bug 244249 so that GetID could be decom'd as well.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The api is about what I had in mind, yes.  Returning addrefed raw pointers is
bad, though.  If at least one callee _has_ to addref, make it return
already_AddRefed<nsIStyleRule>.  Otherwise, don't addreff in the callees?  Then
callers may be able to use raw pointers instead of nsCOMPtrs too.

For GetID, we need to fix SVG and generic XML to store the id somewhere... or
change the GetID code to make better use of attributes.  I'm not sure what the
best approach there is, exactly.

Comment 4

[jpl24]

Thanks Boris, I went back and read the nsCOMPtr guide again and I think I have a
better ideas of how this is supposed to work, and why returning addrefed raw
pointers is a bad thing. 

I looked through the callers(there are only 3) and I think they can avoid
addreffing altogether and just use the raw pointer. If this sounds right, I'll
upload a new patch.

A quick question, the function that is one of the callers,
http://lxr.mozilla.org/seamonkey/source/content/html/style/src/nsDOMCSSAttrDeclaration.cpp#104

looks like it would leak the nsCSSDeclaration if NS_NewCSSStyleRule failed. The
only possible failure would be out of memory, but the code goes to the effort of
aborting the rule. If we are out of memory, do we just say it is horked and not
worry about the leak? Or did I miss something obvious? Thanks again for your help.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> and why returning addrefed raw pointers is a bad thing. 

The simple reason it's bad is that it's too easy to leak if you forget a
getter_AddRefs/dont_AddRef or if you don't assign into a COMPtr... ;)

> If this sounds right, I'll upload a new patch.

That sounds great.  Thank you for doing this!

> looks like it would leak the nsCSSDeclaration if NS_NewCSSStyleRule failed.

See
http://lxr.mozilla.org/seamonkey/source/content/html/style/src/nsCSSDeclaration.h#232
-- RuleAbort() deletes the decl.

Comment 6

[jpl24]

Attachment: decom_patch_ver2

Okay here's the next version of the earlier patch. I think it fixes the
reference counting issues Boris pointed out.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 162964 [details] [diff] [review]
decom_patch_ver2

>Index: content/html/content/src/nsHTMLTextAreaElement.cpp

>   // XXX Bug 50280 - It is unclear why we need to do this here for 
>   // rows and cols and why the AttributeChanged method in
>   // nsTextControlFrame does take care of the entire problem, but
>   // it doesn't and this makes things better

Want to remove this XXX comment while you're here?  The code is perfectly
correct as written; putting it in the frame would be wrong, imo.

r+sr=bzbarsky with that change (in future, just request review via the review
flags on the patch; regular bugmail can get lost too easily...)

Comment 8

[jpl24]

Attachment: decom_patch_ver3

Remove the XXX comment

Comment 9

[jpl24]

Boris could you check this in when you get the chance? Thanks for all your help,
if there are any other deCOM tasks you'd like me to take a crack at let me know.
Leave this bug open for GetID?

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'll check this in when the tree reopens, yes.  I think GetID can be handled in
the GetID bug when/if it happens, so I'll just resolve this when I check this in.

I don't have any other deCOM lying about on hand, but roc ([e-mail])
may.  Also, see the list of bugs at
http://www.mozilla.org/contribute/hacking/first-bugs/ if you're looking for
things to work on.  :)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.8a5.  Thank you for the patch!