665571 - DrawCellWithSnapping should handle special NSZeroSize value in CellRenderSettings

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Mounir Lamouri (:mounir)]

The CellRenderSettings structure passed to DrawCellWithSnapping can contain a or multiple NSZeroSize to reflect that a given ControlSize isn't available for the widget.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 2

[Mounir Lamouri (:mounir)]

Comment on attachment 541418 [details] [diff] [review]
Patch v1

Oups, sorry, small mistake...

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Comment 4

[Markus Stange [:mstange]]

I'll review it tomorrow.

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 541429 [details] [diff] [review]
Patch v1.1

>diff --git a/widget/src/cocoa/nsNativeThemeCocoa.mm b/widget/src/cocoa/nsNativeThemeCocoa.mm
>--- a/widget/src/cocoa/nsNativeThemeCocoa.mm
>+++ b/widget/src/cocoa/nsNativeThemeCocoa.mm
>@@ -275,16 +275,29 @@ static int EnumSizeForCocoaSize(NSContro
>   if (cocoaControlSize == NSMiniControlSize)
>     return miniControlSize;
>   else if (cocoaControlSize == NSSmallControlSize)
>     return smallControlSize;
>   else
>     return regularControlSize;
> }
> 
>+static NSControlSize CocoaSizeForEnum(PRInt32 enumControlSize) {
>+  if (enumControlSize == miniControlSize) {
>+    return NSMiniControlSize;
>+  }
>+  if (enumControlSize == smallControlSize) {
>+    return smallControlSize;

return NSSmallControlSize;

>+  }
>+
>+  NS_ASSERTION(enumControlSize == regularControlSize,
>+               "enumControlSize isn't valid!");
>+  return NSRegularControlSize;
>+}

I don't think the assertion and the braces are necessary. Established coding style in the same file overrides global coding style, so I think you should make this function look like EnumSizeForCocoaSize above. Shorter == more readable in this case.

> /*
>+ * This is a helper method that returns the required NSControlSize given a size
>+ * and the size of the three constrols plus a tolerance.

constrols -> controls

>+ * size - The width or the height of the element to draw.
>+ * sizes - An array with the all the width/height of the element for its
>+ *         different sizes.
>+ * tolerance - The tolerance as passed to DrawCellWithSnapping.

Please mention that it returns NSRegularControlSize when all sizes are zero.

>+ */
>+static NSControlSize FindControlSize(CGFloat size, CGFloat* sizes, CGFloat tolerance)
>+{
>+  for (PRUint32 i = miniControlSize; i <= regularControlSize; ++i) {
>+    if (sizes[i] == 0) {
>+      continue;
>+    }
>+
>+    CGFloat next = 0;
>+    // Find next value.
>+    for (PRUint32 j = i+1; j <= regularControlSize; ++j) {
>+      if (sizes[j] != 0) {
>+        next = sizes[j];
>+        break;
>+      }
>+    }
>+
>+    // If it's the last value, we pick it.
>+    if (next == 0) {
>+      return CocoaSizeForEnum(i);
>+    }
>+
>+    if (size <= sizes[i] + tolerance && size < next) {
>+      return CocoaSizeForEnum(i);
>+    }
>+  }
>+
>+  // If we are here, that means sizes[] was an array with only empty values
>+  // or the algorithm above is wrong.
>+  // The former can happen but the later would be wrong.
>+  NS_ASSERTION(sizes[0] == 0 && sizes[1] == 0 && sizes[2] == 0,
>+               "We found no control! We shouldn't be there!");
>+  return CocoaSizeForEnum(regularControlSize);
>+}

How about this:

static NSControlSize FindControlSize(CGFloat size, CGFloat* sizes, CGFloat tolerance)
{
  PRInt32 controlSize = regularControlSize;
  CGFloat minKeepControlSize = CGFLOAT_MAX;
  for (PRInt32 i = regularControlSize; i >= miniControlSize; --i) {
    if (sizes[i] == 0)
      continue;

    if (size < minKeepControlSize && size <= sizes[i] + tolerance) {
      controlSize = i;
      minKeepControlSize = sizes[i];
    }
  }

  return CocoaSizeForEnum(controlSize);
}

Comment 6

[Markus Stange [:mstange]]

Attachment: test script

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to comment #5)
> Comment on attachment 541429 [details] [diff] [review] [review]
> >+  }
> >+
> >+  NS_ASSERTION(enumControlSize == regularControlSize,
> >+               "enumControlSize isn't valid!");
> >+  return NSRegularControlSize;
> >+}
> 
> I don't think the assertion and the braces are necessary. Established coding
> style in the same file overrides global coding style, so I think you should
> make this function look like EnumSizeForCocoaSize above. Shorter == more
> readable in this case.

If you want.

> How about this:
> 
> static NSControlSize FindControlSize(CGFloat size, CGFloat* sizes, CGFloat
> tolerance)
> {
>   PRInt32 controlSize = regularControlSize;
>   CGFloat minKeepControlSize = CGFLOAT_MAX;
>   for (PRInt32 i = regularControlSize; i >= miniControlSize; --i) {
>     if (sizes[i] == 0)
>       continue;
> 
>     if (size < minKeepControlSize && size <= sizes[i] + tolerance) {
>       controlSize = i;
>       minKeepControlSize = sizes[i];
>     }
>   }
> 
>   return CocoaSizeForEnum(controlSize);
> }

It seems correct too but I don't think it's easier to understand. I can use it if you want to but I would prefer to add an assert at the end in that case. Seems safer.

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.2

With all comments applied except the last one, see previous comment.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/0bd82b867cf0