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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

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.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

[Seth Fowler [:seth] [:s2h]]

"AsAppUnitsRoundOut"/"AsAppUnitsRoundIn"?

Comment 3

[Timothy Nikkel (:tnikkel)]

But it's already in appunits. It converts from appunits to appunits, where there is a different app units per pixel ratio.

Comment 4

[Seth Fowler [:seth] [:s2h]]

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.

Comment 5

[Timothy Nikkel (:tnikkel)]

(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

[Mats Palmgren (inactive)]

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

[Mats Palmgren (inactive)]

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

[Mats Palmgren (inactive)]

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.

Comment 9

[Seth Fowler [:seth] [:s2h]]

(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.)

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: Add warnings.

Changed the comment, add the warnings to other types besides rects.

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: Rename to ScaleToOtherAppUnits

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 8583725 [details] [diff] [review]
Add warnings.

s/Warn if result/Warn if the result/ in the commit message perhaps?

Comment 13

[Mats Palmgren (inactive)]

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!

Comment 14

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1f0b23f5104
https://hg.mozilla.org/integration/mozilla-inbound/rev/b617b76326f7

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1f0b23f5104
https://hg.mozilla.org/mozilla-central/rev/b617b76326f7