418497 - optimize cocoa native theme drawing

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: optimize mac native theme stuff

When drawing cells, scrollbars, etc., we can in almost all cases avoid using a temporary surface by just setting the right scaling on the CGContext before rendering.  For the scrollbar case, we have to create an offscreen surface, but we can do so more efficiently using CG directly.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  void DrawCellWithScaling(NSCell *cell,
+                           CGContextRef cgContext,
+                           const HIRect& destRect,
+                           NSControlSize controlSize,
+                           float naturalWidth, float naturalHeight,
+                           float minWidth, float minHeight,
+                           const float marginSet[][3][4],
+                           PRBool doSaveCTM);

Document all these parameters?

One problem here, which already existed I guess, is that focus rings will be scaled up. Is that really want we want? It actually breaks things because nsNativeThemeCocoa::GetWidgetOverflow doesn't account for such scaling. Oh well.

-        drawRect.size.width >= NATURAL_MINI_ROUNDED_BUTTON_MIN_WIDTH) {
+        drawRect.size.width >= NATURAL_MINI_ROUNDED_BUTTON_MIN_WIDTH)
+    {

Ick

+  if (savedCTM.a == 1.0f && savedCTM.b == 0.0f &&
+      savedCTM.c == 0.0f && (savedCTM.d == 1.0f || savedCTM.d == -1.0f))
+  {
+    drawRect.origin.x = drawRect.origin.y = 0.0f;
+    drawDirect = FALSE;

I'm confused. Shouldn't this be TRUE and the default be FALSE?

Comment 2

[Josh Aas]

Just a note as a reminder, when you've got another revision up I'd like to review it before it goes in.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated patch

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Er, bugzilla jumped the gun on me there.  Updated patch to latest trunk, also fixed a bug with scrollbar rendering (silly flipped context issues).

Comment 5

[Josh Aas]

Comment on attachment 306429 [details] [diff] [review]
updated patch

The new method, DrawCellWithScaling, needs to be wrapped with the following macros just like the method below it:

NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
NS_OBJC_END_TRY_ABORT_BLOCK;

+  if (naturalWidth != 0.0f) {
+    xscale = drawRect.size.width / naturalWidth;
+    drawRect.size.width = naturalWidth;
+  } else if (minWidth != 0.0f &&
+             drawRect.size.width < minWidth)
+  {
+    xscale = drawRect.size.width / minWidth;
+    drawRect.size.width = minWidth;
+  }

In Cocoa widgets we structure things as follows in terms of formatting:

if (...) {
  ...
}
else if (...) {
  ...
}

It is nice to have really consistent style across all of cocoa widgets. This kind of thing should be fixed in a few places in the patch.

+// XXX this should be a h:w aspect ratio!

This comment should go, the point of the height cutoff is that rounded buttons will only naturally draw at a height of about 22px. If you blow them up to more than 26 they start looking really bad. Square buttons look nice at any size, they don't get fuzzy. You might want to replace your comment with that explanation so people don't miss that in the future.

Other than that looks good, thanks!

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

None of my comments seem to have been addressed.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hrm, wonder if I started from the wrong patch =/  Fixing shortly, sorry!

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated with roc's comments

Carrying over josh's r+, updated with roc's comments -- good catch on the drawDirect bit!

Comment 9

[Markus Stange [:mstange]]

Attachment: Rendering differences introduced by patch

Unfortunately, the patch has regressed button appearance at certain sizes. The middle part of the button sometimes looks bad.

The upper screenshot is from a modified version of http://dev.l-c-n.com/forms/button-height.html , the lower screenshot (with the empty button and the vertical white line in the middle) from http://www.designdetector.com/demos/buttons-padding-demo.html

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

What heights are you using there?  I don't see this with sizes going from 14 to 20px in 1px increments.  (Also, what OS?)

Comment 11

[Markus Stange [:mstange]]

Attachment: Testcase

I see it with button heights 17px, 20px and 24px. I'm on Mac OS X 10.4.11, so maybe this is a Tiger-only issue.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I see it on 10.4. Markus, please open another bug for this. The effect is quite subtle so it might only be wanted1.9 instead of blocking.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Although backing out this patch is also an option.

Comment 14

[Markus Stange [:mstange]]

I've filed bug 422248.