Closed
Bug 1144951
Opened 8 years ago
Closed 8 years ago
warn if result of ConvertAppUnits is unused, rename it to ScaleToOtherAppUnits
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: tnikkel, Assigned: tnikkel)
References
Details
Attachments
(2 files, 1 obsolete file)
6.24 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
29.18 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
bug 1143974 comment 6 The name ConvertAppUnitsRoundOut/In suggests that the function modifies |this|, but it doesn't, it returns the result instead. If we can't think of a better name we should at least warn if the result is unused, which will always be a problem.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8579686 -
Flags: review?(mats)
Comment 2•8 years ago
|
||
"AsAppUnitsRoundOut"/"AsAppUnitsRoundIn"?
Assignee | ||
Comment 3•8 years ago
|
||
But it's already in appunits. It converts from appunits to appunits, where there is a different app units per pixel ratio.
Comment 4•8 years ago
|
||
Hmm, that's a good point. It's hard to come up with a good short name. "WithAppUnitConversionRoundOut"? I actually think that part of the problem here is that this maybe doesn't make so much sense as a method. "ConvertAppUnitsRoundOut" works better to me as a free function (or perhaps a static method), and then there's no ambiguity about whether it mutates |this| or not.
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Seth Fowler [:seth] from comment #4) > "ConvertAppUnitsRoundOut" works better to me as a free function (or perhaps > a static method), and then there's no ambiguity about whether it mutates > |this| or not. I would be fine with that.
Comment 6•8 years ago
|
||
This is what nsRect::ConvertAppUnitsRoundOut does: nsRect rect; nscoord right = NSToCoordCeil(NSCoordScale(XMost(), aFromAPP, aToAPP)); nscoord bottom = NSToCoordCeil(NSCoordScale(YMost(), aFromAPP, aToAPP)); rect.x = NSToCoordFloor(NSCoordScale(x, aFromAPP, aToAPP)); rect.y = NSToCoordFloor(NSCoordScale(y, aFromAPP, aToAPP)); rect.width = (right - rect.x); rect.height = (bottom - rect.y); return rect; Based on what it does, "Scale" seems like a better description than "Convert". I think it should also have "To" in the name like the other methods in this class, and like the naming convention established in layout/base/Units.h. So I'd suggest ScaleToAppUnitsRoundOut or ToScaledAppUnitsRoundOut. If you want to stress that it's a different app unit, you could make it ScaleToOtherAppUnitsRoundOut, or simply ToOtherAppUnitsRoundOut.
Comment 7•8 years ago
|
||
Regarding methods vs. functions: I think I'm slightly favor methods for aesthetic reasons: aRect.Foo().Bar() vs Bar(Foo(aRect)) or nsRect::Bar(nsRect::Foo(aRect)) In general, I think we should avoid adding methods that modifies |this| for simple "data types" like nsRect. For example, nsRect has: void SaturatingUnionRect(const nsRect& aRect1, const nsRect& aRect2) { *this = aRect1.SaturatingUnion(aRect2); } which looks like this when called: rect.SaturatingUnionRect(rect1, rect2); without it we would be forced to write: rect = rect1.SaturatingUnion(rect2); which is much clearer. Without such methods the question of "does this method modify |this|?" should come up less often, so taking the static method / global function option seems unnecessary.
Comment 8•8 years ago
|
||
Comment on attachment 8579686 [details] [diff] [review] patch Thanks. Could you also make the code comment less ambiguous? It currently says "Converts this rect from aFromAPP, ..." which could easily be misread for "Converts |this| rect from aFromAPP, ..." which hints at modifying |this|. Perhaps it's time we make this a proper doc comment, like so: /** * Return this rect scaled to a different appunits per pixel (APP) ratio. * In the RoundOut version we make the rect the smallest rect containing the * unrounded result. In the RoundIn version we make the rect the largest rect * contained in the unrounded result. * @param aFromAPP the APP to scale from * @param aToAPP the APP to scale to * @note this can turn an empty rectangle into a non-empty rectangle */ Also, is there a reason not to add MOZ_WARN_UNUSED_RESULT to all the other methods in this class? (and nsIntRect too) We might as well do that while we're here unless there's massive fallout from that.
Attachment #8579686 -
Flags: review?(mats) → review+
Comment 9•8 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #8) > Also, is there a reason not to add MOZ_WARN_UNUSED_RESULT to > all the other methods in this class? (and nsIntRect too) > We might as well do that while we're here unless there's massive > fallout from that. I wrote a patch to do that in bug 1147706. (I left ConvertAppUnits* alone, though, so the patch in this bug isn't obsolete.)
Assignee | ||
Comment 10•8 years ago
|
||
Changed the comment, add the warnings to other types besides rects.
Attachment #8579686 -
Attachment is obsolete: true
Attachment #8583725 -
Flags: review?(mats)
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8583726 -
Flags: review?(mats)
Comment 12•8 years ago
|
||
Comment on attachment 8583725 [details] [diff] [review] Add warnings. s/Warn if result/Warn if the result/ in the commit message perhaps?
Attachment #8583725 -
Flags: review?(mats) → review+
Comment 13•8 years ago
|
||
Comment on attachment 8583726 [details] [diff] [review] Rename to ScaleToOtherAppUnits Thanks to you and Seth for cleaning this up. This is much better and it looks like we already caught a bug with it. Great!
Attachment #8583726 -
Flags: review?(mats) → review+
Assignee | ||
Updated•8 years ago
|
Summary: warn if result of ConvertAppUnitsRoundOut/In is unused → warn if result of ConvertAppUnits is unused, rename it to ScaleToOtherAppUnits
Assignee | ||
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1f0b23f5104 https://hg.mozilla.org/integration/mozilla-inbound/rev/b617b76326f7
Comment 15•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a1f0b23f5104 https://hg.mozilla.org/mozilla-central/rev/b617b76326f7
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•