warn if result of ConvertAppUnits is unused, rename it to ScaleToOtherAppUnits

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

unspecified
mozilla39
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Posted patch patch (obsolete) — Splinter Review
Attachment #8579686 - Flags: review?(mats)
"AsAppUnitsRoundOut"/"AsAppUnitsRoundIn"?
But it's already in appunits. It converts from appunits to appunits, where there is a different app units per pixel ratio.
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.
(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.
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.
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 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+
(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.)
Posted patch Add warnings.Splinter Review
Changed the comment, add the warnings to other types besides rects.
Attachment #8579686 - Attachment is obsolete: true
Attachment #8583725 - Flags: review?(mats)
Attachment #8583726 - Flags: review?(mats)
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 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+
Summary: warn if result of ConvertAppUnitsRoundOut/In is unused → warn if result of ConvertAppUnits is unused, rename it to ScaleToOtherAppUnits
https://hg.mozilla.org/mozilla-central/rev/a1f0b23f5104
https://hg.mozilla.org/mozilla-central/rev/b617b76326f7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.