Closed Bug 585867 Opened 9 years ago Closed 9 years ago

Remove OOM checks from property getters in nsComputedDOMStyle and make them not return nsresult

Categories

(Core :: CSS Parsing and Computation, defect, P4, minor)

defect

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: craig.topper, Assigned: craig.topper)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 7 obsolete files)

85.00 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
8.58 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.39 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
26.91 KB, patch
Details | Diff | Splinter Review
115.52 KB, patch
Details | Diff | Splinter Review
The property getters in nsComputedDOMStyle spend a lot of code handling OOM errors. Since allocation is now infallible all these checks can be removed simplifying the code and allowing the getters to lose their outparam.
Attachment #466927 - Flags: review?(bzbarsky)
Unfortunately, nsCOMArray and nsTArray are not infallible during resize operations. To work around this I have added my own call to mozalloc_handle_oom which will abort making us infallible. I modeled this after the code for moz_xmalloc.
Attachment #466930 - Flags: review?(bzbarsky)
Attachment #466931 - Flags: review?(bzbarsky)
Attachment #466931 - Attachment is patch: true
Attachment #466931 - Attachment mime type: application/octet-stream → text/plain
This patch removes the outparam from all of the property getters and moves to a single AddRef after the getter returns.
Attachment #466932 - Flags: review?(bzbarsky)
Comment on attachment 466927 [details] [diff] [review]
Part 2: Make SetToRGBAColor return void

>diff --git a/layout/style/nsComputedDOMStyle.cpp b/layout/style/nsComputedDOMStyle.cpp
>--- a/layout/style/nsComputedDOMStyle.cpp
>+++ b/layout/style/nsComputedDOMStyle.cpp
>@@ -661,23 +661,22 @@ nsComputedDOMStyle::DoGetStackSizing(nsI
> 
>   val->SetIdent(GetStyleXUL()->mStretchStack ? eCSSKeyword_stretch_to_fit :
>                 eCSSKeyword_ignore);
> 
>   NS_ADDREF(*aValue = val);
>   return NS_OK;
> }
> 
>-nsresult
>+void
> nsComputedDOMStyle::SetToRGBAColor(nsROCSSPrimitiveValue* aValue,
>                                    nscolor aColor)
> {
>   if (NS_GET_A(aColor) == 0) {
>     aValue->SetIdent(eCSSKeyword_transparent);
>-    return NS_OK;
>   }

You still need the return, right?
> You still need the return, right?

Oops! Thanks!
Craig, are those patches pretty much current (in that I should go ahead and review them), or have you had to merge since you posted them in nontrivial ways?
I can post newer patches. Can't remember if any of the merges were non trivial.
Attachment #466925 - Attachment is obsolete: true
Attachment #500641 - Flags: review?(bzbarsky)
Attachment #466925 - Flags: review?(bzbarsky)
Attachment #466927 - Attachment is obsolete: true
Attachment #500642 - Flags: review?(bzbarsky)
Attachment #466927 - Flags: review?(bzbarsky)
This patch converts the nsCOMArray<nsIDOMCSSValue> in nsDOMCSSValueList to an infallible nsTArray<nsCOMPtr<nsIDOMCSSValue>>.
Attachment #466930 - Attachment is obsolete: true
Attachment #500643 - Flags: review?(bzbarsky)
Attachment #466930 - Flags: review?(bzbarsky)
Attachment #466931 - Attachment is obsolete: true
Attachment #500644 - Flags: review?(bzbarsky)
Attachment #466931 - Flags: review?(bzbarsky)
Attachment #466932 - Attachment is obsolete: true
Attachment #500646 - Flags: review?(bzbarsky)
Attachment #466932 - Flags: review?(bzbarsky)
Comment on attachment 500641 [details] [diff] [review]
Part 1: Remove OOM checks on retval of GetROCSSPrimitiveValue and GetROCSSValueList

r=me
Attachment #500641 - Flags: review?(bzbarsky) → review+
Comment on attachment 500642 [details] [diff] [review]
Part 2: Make SetToRGBAColor return void

r=me
Attachment #500642 - Flags: review?(bzbarsky) → review+
Comment on attachment 500643 [details] [diff] [review]
Part 3: Make AppendCSSValue infallible

Why do you need the mozilla/mozalloc_oom.h include in nsDOMCSSValueList.cpp?  r=me with that explained or removed.
Attachment #500643 - Flags: review?(bzbarsky) → review+
Ah, that looks like a leftover from the previous patch version.  Just remove it.
Comment on attachment 500644 [details] [diff] [review]
Part 4: Make SetValueToStyleImage return void

r=me
Attachment #500644 - Flags: review?(bzbarsky) → review+
(In reply to comment #17)
> Comment on attachment 500643 [details] [diff] [review]
> Part 3: Make AppendCSSValue infallible
> 
> Why do you need the mozilla/mozalloc_oom.h include in nsDOMCSSValueList.cpp? 
> r=me with that explained or removed.

Oops that's leftover from before I converted to nsTArray. I was previously aborting myself since nsTArray wasn't infallible.
Comment on attachment 500646 [details] [diff] [review]
Part 5: Remove outparam and AddRef from all property getters

The changes to nsComputedDOMStyle::GetEllipseRadii change behavior (by changing the call to SetValueToCoord).  Merge issue?

Other than that, document in the struct in the header the ownership model for these getters (returned pointer is to a refcounted object that's just been created, with a refcount still at 0).

r=me with those two things fixed.
Attachment #500646 - Flags: review?(bzbarsky) → review+
Note that this will end up conflicting with bug 256274 (which adds some stuff to nsComputedDOMStyle).  I think we should land this first, since the merge in the other direction should be simpler.  None of this is an issue until the tree reopens, of course....
Blocks: css-ruby
Patch to address review comments
Attachment #500643 - Attachment is obsolete: true
Addresses review comments
Attachment #500646 - Attachment is obsolete: true
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.