Implement native vertical meters for MacOS X

RESOLVED FIXED in mozilla16

Status

()

Product:
Core
Component:
Widget: Cocoa
Status:
RESOLVED FIXED
Reported:
8 years ago
Modified:
7 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks: 2 bugs)

Version:
Trunk
Target:
mozilla16
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Patch ready to land
6.52 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

•
8 years ago 
Given that there is no native vertical meter, we will have to use the native horizontal meter and rotate it.
(Assignee)

Comment 1

•
8 years ago 
Created attachment 538314 [details] [diff] [review]
Patch v1
Attachment #538314 - Flags: review?(joshmoz)
 (Assignee)

Updated

•
8 years ago
Whiteboard: [needs review]
 (Assignee)

Comment 2

•
8 years ago 
Created attachment 538316 [details] [diff] [review]
Patch v1

Adding a test for RTL meter.
Attachment #538314 - Attachment is obsolete: true
Attachment #538314 - Flags: review?(joshmoz)
Attachment #538316 - Flags: review?(joshmoz)
 (Assignee)

Comment 3

•
8 years ago 
Created attachment 538319 [details] [diff] [review]
Patch v1

That was not for this bug... Re-attaching the correct file.
Attachment #538316 - Attachment is obsolete: true
Attachment #538316 - Flags: review?(joshmoz)
Attachment #538319 - Flags: review?(joshmoz)

Comment 4

•
8 years ago 
+    CGContextTranslateCTM(cgContext, CGRectGetMidX(rect), CGRectGetMidY(rect));
+    CGContextRotateCTM(cgContext, -M_PI / 2.f);
+    CGContextTranslateCTM(cgContext, -CGRectGetMidX(rect), -CGRectGetMidY(rect));
+  }
+
+  DrawCellWithSnapping(cell, cgContext, rect,
                        meterSetting, VerticalAlignFactor(aFrame),
-                       mCellDrawView, IsFrameRTL(aFrame));
+                       mCellDrawView, !vertical && IsFrameRTL(aFrame));
 
I think you need to wrap this all into CGContextSaveGState / CGContextRestoreGState, otherwise the rotation transform might affect things that are painted after the meter.
 (Assignee)

Comment 5

•
8 years ago 
Created attachment 538461 [details] [diff] [review]
Patch v1

Adding save/restore state. Though, I didn't add that because it was working well without it.

By the way, Steven and Markus, feel free to still this review. I asked Josh because I'm used to ask him to review my Cocoa widget patches but I would bet he wouldn't mind to save some time :)
Attachment #538319 - Attachment is obsolete: true
Attachment #538319 - Flags: review?(joshmoz)
Attachment #538461 - Flags: review?(joshmoz)
 (Assignee)

Updated

•
8 years ago
Blocks: 663340

Comment 6

•
8 years ago 
Comment on attachment 538461 [details] [diff] [review]
Patch v1

Benoit - can you review this?
Attachment #538461 - Flags: review?(joshmoz) → review?(bgirard)

Comment 7

•
8 years ago 
Comment on attachment 538461 [details] [diff] [review]
Patch v1

Review of attachment 538461 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r+ with these fixed.

::: widget/reftests/meter-vertical-native-style.html
@@ +5,5 @@
> +  <body>
> +<!-- For some reasons, the ref has a small offset when there are spaces between meters.
> +     Given that we don't want to test -moz-transform and even a perfect match but just
> +     the general rendering, we are going to keep this dirty test.
> +     It's very similar to the non-vertical test with a difference, for the RTL: RTL

This acronym wasn't obvious to me at first, I suggest writing out Right to Left.

::: widget/src/cocoa/nsNativeThemeCocoa.mm
@@ +1329,5 @@
>  
> +  HIRect rect = CGRectStandardize(inBoxRect);
> +  BOOL vertical = IsVerticalMeter(aFrame);
> +
> +  CGContextSaveGState(cgContext);

System calls should be prefixed with |::|. All the CGContext*/CGRect* calls here. For example:
|::CGContextSaveGState(cgContext);|

@@ +1342,5 @@
> +     * to get a rectangle with horizontal dimensions.
> +     * Finally, we want to show a vertical meter so we want to rotate the result
> +     * so it is vertical. We do that by changing the context.
> +     */
> +    float tmp = rect.size.width;

A bit of a gotcha here, CGFloat is a typedef for either float (32) or double (64) so I suggest replacing your usage of float here and '.f' by CGFloat here. This has caused us a few bugs when porting to 64-bit so I suggest we use it consistently. In this case we would be assigning a double to a float on 64-bit.
Attachment #538461 - Flags: review?(bgirard) → review+
Whiteboard: [needs review]
 (Assignee)

Comment 8

•
8 years ago 
(In reply to comment #7)
> ::: widget/reftests/meter-vertical-native-style.html
> @@ +5,5 @@
> > +  <body>
> > +<!-- For some reasons, the ref has a small offset when there are spaces between meters.
> > +     Given that we don't want to test -moz-transform and even a perfect match but just
> > +     the general rendering, we are going to keep this dirty test.
> > +     It's very similar to the non-vertical test with a difference, for the RTL: RTL
> 
> This acronym wasn't obvious to me at first, I suggest writing out Right to
> Left.

RTL is the same acronym used for the HTML attribute 'dir' (|dir="rtl"|). I think most devs know what it means and if they don't they will learn about it at some point. In the widget code, for example, RTL appears multiple time.

> ::: widget/src/cocoa/nsNativeThemeCocoa.mm
> @@ +1329,5 @@
> >  
> > +  HIRect rect = CGRectStandardize(inBoxRect);
> > +  BOOL vertical = IsVerticalMeter(aFrame);
> > +
> > +  CGContextSaveGState(cgContext);
> 
> System calls should be prefixed with |::|. All the CGContext*/CGRect* calls
> here. For example:
> |::CGContextSaveGState(cgContext);|

In this file, only one method is prefixed with |::|, it's |GetThemeMetric|. Obviously, this rule doesn't apply for this file.

> @@ +1342,5 @@
> > +     * to get a rectangle with horizontal dimensions.
> > +     * Finally, we want to show a vertical meter so we want to rotate the result
> > +     * so it is vertical. We do that by changing the context.
> > +     */
> > +    float tmp = rect.size.width;
> 
> A bit of a gotcha here, CGFloat is a typedef for either float (32) or double
> (64) so I suggest replacing your usage of float here and '.f' by CGFloat
> here. This has caused us a few bugs when porting to 64-bit so I suggest we
> use it consistently. In this case we would be assigning a double to a float
> on 64-bit.

Indeed, I should use CGFloat. Though, I do not understand what you meant by "I suggest replacing your usage of float here *and '.f'* by CGFloat here". Do you want me to use CGFloat(2) instead of 2.f?

Comment 9

•
8 years ago 
(In reply to comment #8)
> Indeed, I should use CGFloat. Though, I do not understand what you meant by
> "I suggest replacing your usage of float here *and '.f'* by CGFloat here".
> Do you want me to use CGFloat(2) instead of 2.f?

Yes, that's what I meant but now that I think of it I don't think that is required. We just need float -> CGFloat.
 (Assignee)

Comment 10

•
8 years ago 
Created attachment 539540 [details] [diff] [review]
Patch ready to land
Attachment #538461 - Attachment is obsolete: true
 (Assignee)

Updated

•
8 years ago
Whiteboard: [ready to land][waits for dependencies]
 (Assignee)

Comment 11

•
7 years ago 
https://hg.mozilla.org/mozilla-central/rev/0f2b9a6e36c7
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [ready to land][waits for dependencies]
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.