[webkit-reviews] review denied: [Bug 27968] [HTML5][Forms] <input type=number> UI : [Attachment 39538] Patch part 2: Mac implementation of outer-spin-button appearance

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Sep 23 10:34:10 PDT 2009


Eric Seidel <eric at webkit.org> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 27968: [HTML5][Forms] <input type=number> UI
https://bugs.webkit.org/show_bug.cgi?id=27968

Attachment 39538: Patch part 2: Mac implementation of outer-spin-button
appearance
https://bugs.webkit.org/attachment.cgi?id=39538&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
This change is mostly useless w/o pixel results.  If we can't see what UI
you're adding, we can't really review it.  Also your ChangeLogs are still kinda
empty. :(

Why have this big function if you're only going to return 0?
+const int* stepperMargins(NSControlSize controlSize)
+{
+    static const int margins[3][4] = {
+	 {0, 0, 0, 0},
+	 {0, 0, 0, 0},
+	 {0, 0, 0, 0}
+    };
+    return margins[controlSize];
+}

No need:
+    static NSStepperCell* cell;
+    if (!cell) {
+	 cell = [[NSStepperCell alloc] init];
+    }
Just do:
static NSStepperCell* cell = [[NSStepperCell alloc] init];
and that will only run once, the first time the function is called.

Needs { } due to the comment:
+    if (states & PressedState && states & SpinUpState)
+	 // FIXME: There is no way to draw a NSSteperCell with the up button
hilighted.
+	 // Disables the hilight of the down button if the up button is
pressed.
+	 [cell setHighlighted:NO];


More information about the webkit-reviews mailing list