|
6.52 KB,
patch
|
Details | Diff | Splinter Review |
Given that there is no native vertical meter, we will have to use the native horizontal meter and rotate it.
|
(Assignee) | |
Comment 1•6 years ago
|
||
Created attachment 538314 [details] [diff] [review] Patch v1
|
|
(Assignee) | |
Updated•6 years ago
|
||
|
|
(Assignee) | |
Comment 2•6 years ago
|
||
Created attachment 538316 [details] [diff] [review] Patch v1 Adding a test for RTL meter.
|
|
(Assignee) | |
Comment 3•6 years ago
|
||
Created attachment 538319 [details] [diff] [review] Patch v1 That was not for this bug... Re-attaching the correct file.
|
||
Comment 4•6 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•6 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 :)
Comment on attachment 538461 [details] [diff] [review] Patch v1 Benoit - can you review this?
|
||
Comment 7•6 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.
|
|
||
Updated•6 years ago
|
||
|
|
(Assignee) | |
Comment 8•6 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•6 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•6 years ago
|
||
Created attachment 539540 [details] [diff] [review] Patch ready to land
|
|
(Assignee) | |
Updated•6 years ago
|
||
|
|
(Assignee) | |
Comment 11•5 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0f2b9a6e36c7
Description
•