Closed Bug 1088781 Opened 5 years ago Closed 5 years ago

Rename nsLayoutUtils::GetStringWidth to nsLayoutUtils::AppUnitWidthOfStringBidi

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file)

nsLayoutUtils::GetStringWidth essentially does the same as the nsLayoutUtils::AppUnitWidthOfString methods. We should give it a similar name, but probably with an indication that it honors bidi text.
Attached patch patchSplinter Review
This also reorders the methods to be similar to the other AppUnitWidthOfString methods.
Attachment #8511154 - Flags: review?(dholbert)
I also moved the declaration/implementation to put it beside the other AppUnitWidthOfString methods. And also changed pointer args to references since we know they can't be null.
Comment on attachment 8511154 [details] [diff] [review]
patch

One notable difference from AppUnitWidthOfString is: AppUnitWidthOfString takes an unsigned arg 'aLength', whereas the Bidi wrapper-version takes a *signed* aLength.

(but then it passes this signed value into AppUnitWidthOfString, as the unsigned arg, in its non-BidiEnabled() codepath.)

Perhaps we should assert that this arg is nonnegative, to give us confidence that this conversion isn't bogus?
(In reply to Jonathan Watt [:jwatt] from comment #2)
> And also changed pointer args to references
> since we know they can't be null.

I'm not sure I agree with this sort of change in general, but in this case, I suppose it's worth doing, for consistency with AppUnitWidthOfString.

(I'm not sure I generally agree that args that are expected to be non-null should be converted to C++ references; e.g. note that our nsIFrame* arg in this function must be non-null, and yet you're not making that a reference.  It particular feels non-mozilla-style (to me at least) for heap-allocated refcounted objects to be passed around as C++ references; it seems like normally we pass those sorts of things around as pointers, and have assertions as-needed to promise that things are non-null.)
(In searching MXR for AppUnitWidthOfString, I'm realizing it's only just been added; setting "depends on" to bug 1088550, which added it.)
Depends on: 1088550
Comment on attachment 8511154 [details] [diff] [review]
patch

It seems like almost all callers of this API end up passing in e.g.
  someString.get(), someString.Length()

Feels a bit silly to have that pattern  over the place, when we could just be passing in someString directly.  Maybe this function should just take a nsAString, and the few callers that really do just have a character pointer & a length can construct a lightweight string on the fly and pass that in?

(Or alternately, we could provide a wrapper that takes a string, like we do for AppUnitWidthOfString, and have most callers use that wrapper.)

r=me as-is, or with either of those changes.
Attachment #8511154 - Flags: review?(dholbert) → review+
Comment on attachment 8511154 [details] [diff] [review]
patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd6397445f62

(In reply to Daniel Holbert [:dholbert] from comment #4)
> I'm not sure I agree with this sort of change in general, but in this case,
> I suppose it's worth doing, for consistency with AppUnitWidthOfString.

You're right that a lot of Mozilla code has pointer arguments when nullptr is not allowed. I personally think that's a bad thing though. It just adds one more potential thing to think about for anyone looking at the implementations of such functions. Anyway, for consistency with the existing AppUnitWidthOfString variants this should be consistent as you point out.

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Feels a bit silly to have that pattern  over the place, when we could just
> be passing in someString directly.

I added an additional helper taking an nsString.

(In reply to Daniel Holbert [:dholbert] from comment #3)
> One notable difference from AppUnitWidthOfString is: AppUnitWidthOfString
> takes an unsigned arg 'aLength', whereas the Bidi wrapper-version takes a
> *signed* aLength.

After adding the nsString variant there was only one caller of the original variant in the patch. I change the arg to be unsigned since in that case the argument being passed is unsigned.
Attachment #8511154 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/bd6397445f62
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1089768
No longer depends on: 1089768
You need to log in before you can comment on or make changes to this bug.