Last Comment Bug 665571 - DrawCellWithSnapping should handle special NSZeroSize value in CellRenderSettings
: DrawCellWithSnapping should handle special NSZeroSize value in CellRenderSett...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: mozilla8
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on:
Blocks: 664925
  Show dependency treegraph
 
Reported: 2011-06-20 09:26 PDT by Mounir Lamouri (:mounir)
Modified: 2011-07-12 04:54 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (4.97 KB, patch)
2011-06-23 10:31 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.1 (5.05 KB, patch)
2011-06-23 10:52 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
test script (6.15 KB, text/plain)
2011-07-04 05:38 PDT, Markus Stange [:mstange]
no flags Details
Patch v1.2 (5.03 KB, patch)
2011-07-06 07:04 PDT, Mounir Lamouri (:mounir)
mstange: review+
mounir: checkin+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-06-20 09:26:58 PDT
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) 2011-06-23 10:31:08 PDT
Created attachment 541418 [details] [diff] [review]
Patch v1
Comment 2 Mounir Lamouri (:mounir) 2011-06-23 10:49:55 PDT
Comment on attachment 541418 [details] [diff] [review]
Patch v1

Oups, sorry, small mistake...
Comment 3 Mounir Lamouri (:mounir) 2011-06-23 10:52:50 PDT
Created attachment 541429 [details] [diff] [review]
Patch v1.1
Comment 4 Markus Stange [:mstange] 2011-06-29 13:10:24 PDT
I'll review it tomorrow.
Comment 5 Markus Stange [:mstange] 2011-07-04 05:37:38 PDT
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] 2011-07-04 05:38:30 PDT
Created attachment 543749 [details]
test script
Comment 7 Mounir Lamouri (:mounir) 2011-07-06 07:01:12 PDT
(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) 2011-07-06 07:04:53 PDT
Created attachment 544230 [details] [diff] [review]
Patch v1.2

With all comments applied except the last one, see previous comment.
Comment 9 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-09 20:21:43 PDT
http://hg.mozilla.org/mozilla-central/rev/0bd82b867cf0

Note You need to log in before you can comment on or make changes to this bug.