Implement computed clip

RESOLVED FIXED in mozilla0.9.8

Status

()

P1
normal
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks: 1 bug)

Trunk
mozilla0.9.8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

17 years ago
 
(Assignee)

Updated

17 years ago
Blocks: 42417
(Assignee)

Comment 1

17 years ago
working in my tree; will attach patch around Jan 7/8, 2002.
Priority: -- → P1
Target Milestone: --- → mozilla0.9.8
(Assignee)

Comment 2

17 years ago
Created attachment 64003 [details] [diff] [review]
Patch to fix this and a whole bunch of other bugs

Patch also fixes bug 116033, bug 94080, bug 98052
Comment on attachment 64003 [details] [diff] [review]
Patch to fix this and a whole bunch of other bugs

>+  return val->QueryInterface(NS_GET_IID(nsIDOMCSSPrimitiveValue),
>+                             (void **)&aValue);

Use CallQueryInterface here.

>+      val->SetString(NS_LITERAL_STRING("iherit"));

"i_n_herit" ;)

>+        if (topVal)
>+          delete topVal;
>+        if (rightVal)
>+          delete rightVal;
>+        if (bottomVal)
>+          delete bottomVal;
>+        if (leftVal)
>+          delete leftVal;
>+        rv = NS_ERROR_OUT_OF_MEMORY;
>+      }
>+    }
>+  }
>+
>+  if (NS_FAILED(rv)) {
>+    delete val;
>+    return rv;
>+  }

As discussed, don't leak topVal etc, move it down to where you delete val.

>+NS_IMETHODIMP
>+nsDOMCSSRect::GetTop(nsIDOMCSSPrimitiveValue** aTop)
>+{
>+  NS_ENSURE_TRUE(mTop, NS_ERROR_NOT_INITIALIZED);

I think you should NS_ENSURE_ARG_POINTER(aTop) in the getters in nsDOMCSSRect.

>-    case CSS_RECT :
>     case CSS_RGBCOLOR :
>+      aCssText.Truncate();
>       return NS_ERROR_DOM_INVALID_ACCESS_ERR;

just setting result = NS_ERROR_DOM_INVALID_ACCESS_ERR would work here too. 

>   }
> 
>-  aCssText.Assign(tmpStr);
>+  if (NS_FAILED(result)) {
>+    aCssText.Truncate();
>+  } else {
>+    aCssText.Assign(tmpStr);
>+  }

With these changes (NS_ENSURE_ARG_POINTER is optional), r=peterv.
Attachment #64003 - Flags: review+
Comment on attachment 64003 [details] [diff] [review]
Patch to fix this and a whole bunch of other bugs

What peterv said, and I'd vote for not adding NS_ENSURE_ARG_POINTER()'s,
whoever calls these methods w/o passing in valid out parameters needs to
suffer.

>+      val->SetString(style.get());

Is it time to make SetString() take a nsACString& in stead of char*?

>-    if(str_weight.Length()>0) {
>+    if(! str_weight.IsEmpty()) {

Ew, eek, space after negation operator, horror! :-)

>+        if (topVal)
>+          delete topVal;
>+        if (rightVal)
>+          delete rightVal;
>+        if (bottomVal)
>+          delete bottomVal;
>+        if (leftVal)
>+          delete leftVal;

|delete| is null safe, no need for all those if checks.

> nsROCSSPrimitiveValue::GetCssText(nsAWritableString& aCssText)
> {
>   nsAutoString tmpStr;
>-
>-  aCssText.Truncate();

Not sure I agree with this change, it's basically a nop, and it's good for
maintainability, so I'd say leave the Truncate() call there.

>+    case CSS_RECT :
>+      {
>+        nsCOMPtr<nsIDOMCSSPrimitiveValue> sideCSSValue;
>+        nsAutoString sideValue;
>+        tmpStr = NS_LITERAL_STRING("rect(");
>+        // get the top
>+        result = mRect->GetTop(getter_AddRefs(sideCSSValue));
>+        if (NS_FAILED(result))
>+          break;
>+        result = sideCSSValue->GetCssText(sideValue);
>+        if (NS_FAILED(result))
>+          break;
>+        tmpStr.Append(sideValue + NS_LITERAL_STRING(", "));

You have 3 occurances of the literal string ", ", use NS_NAMED_LITERAL_STRING()
and use the variable in stead of duplicating the literal.

> nsROCSSPrimitiveValue::GetStringValue(nsAWritableString& aReturn)
> {
>+  if (mType != CSS_STRING) {

Truncate() aReturn here.

>+    return NS_ERROR_DOM_INVALID_ACCESS_ERR;
>+  }

...

> nsROCSSPrimitiveValue::GetRectValue(nsIDOMRect** aReturn)
> {
>-  return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
>+  if (mType != CSS_RECT || !mRect) {

Null out *aReturn here, returning from methods with out parameters w/o
initializing them is not cool, even if you return an error.

>+    return NS_ERROR_DOM_INVALID_ACCESS_ERR;
>+  }
>+  return CallQueryInterface(mRect, aReturn);
> }

With that, sr=jst
Attachment #64003 - Flags: superreview+
(Assignee)

Comment 5

17 years ago
Created attachment 64342 [details] [diff] [review]
Patch updated to all comments and including mac project changes (thanks, peterv!)
Attachment #64003 - Attachment is obsolete: true
(Assignee)

Comment 6

17 years ago
Created attachment 64348 [details] [diff] [review]
Ooops.  forgot a file.
Attachment #64342 - Attachment is obsolete: true
(Assignee)

Comment 7

17 years ago
fixed.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.