116032 - Implement computed clip

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Comment 1

[Boris Zbarsky [:bzbarsky]]

working in my tree; will attach patch around Jan 7/8, 2002.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to fix this and a whole bunch of other bugs

Patch also fixes bug 116033, bug 94080, bug 98052

Comment 3

[Peter Van der Beken [:peterv]]

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.

Comment 4

[Johnny Stenback (:jst)]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to all comments and including mac project changes (thanks, peterv!)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Ooops. forgot a file.

Comment 7

[Boris Zbarsky [:bzbarsky]]

fixed.