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

RESOLVED FIXED in mozilla5

Status

()

P4
minor
RESOLVED FIXED
8 years ago
3 years ago

People

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

Tracking

(Blocks: 1 bug)

Trunk
mozilla5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 7 obsolete attachments)

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
(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
Created attachment 466925 [details] [diff] [review]
Part 1: Remove OOM checks on retval of GetROCSSPrimitiveValue and GetROCSSValueList
Attachment #466925 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

8 years ago
Created attachment 466927 [details] [diff] [review]
Part 2: Make SetToRGBAColor return void
Attachment #466927 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

8 years ago
Created attachment 466930 [details] [diff] [review]
Part 3: Make AppendCSSValue infallible

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)
(Assignee)

Comment 4

8 years ago
Created attachment 466931 [details] [diff] [review]
Part 4: Make SetValueToStyleImage return void
Attachment #466931 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 years ago
Attachment #466931 - Attachment is patch: true
Attachment #466931 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 5

8 years ago
Created attachment 466932 [details] [diff] [review]
Part 5: Remove outparam and AddRef from all property getters

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?
(Assignee)

Comment 7

8 years ago
> 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?
(Assignee)

Comment 9

8 years ago
I can post newer patches. Can't remember if any of the merges were non trivial.
(Assignee)

Comment 10

8 years ago
Created attachment 500641 [details] [diff] [review]
Part 1: Remove OOM checks on retval of GetROCSSPrimitiveValue and GetROCSSValueList
Attachment #466925 - Attachment is obsolete: true
Attachment #500641 - Flags: review?(bzbarsky)
Attachment #466925 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

8 years ago
Created attachment 500642 [details] [diff] [review]
Part 2: Make SetToRGBAColor return void
Attachment #466927 - Attachment is obsolete: true
Attachment #500642 - Flags: review?(bzbarsky)
Attachment #466927 - Flags: review?(bzbarsky)
(Assignee)

Comment 12

8 years ago
Created attachment 500643 [details] [diff] [review]
Part 3: Make AppendCSSValue infallible

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)
(Assignee)

Comment 13

8 years ago
Created attachment 500644 [details] [diff] [review]
Part 4: Make SetValueToStyleImage return void
Attachment #466931 - Attachment is obsolete: true
Attachment #500644 - Flags: review?(bzbarsky)
Attachment #466931 - Flags: review?(bzbarsky)
(Assignee)

Comment 14

8 years ago
Created attachment 500646 [details] [diff] [review]
Part 5: Remove outparam and AddRef from all property getters
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+
(Assignee)

Comment 20

8 years ago
(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: 256274
(Assignee)

Comment 23

8 years ago
Created attachment 516900 [details] [diff] [review]
Part 3: Make AppendCSSValue infallible

Patch to address review comments
Attachment #500643 - Attachment is obsolete: true
(Assignee)

Comment 24

8 years ago
Created attachment 516901 [details] [diff] [review]
Part 5: Remove outparam and AddRef from all property getters

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.