1251995 - Consider using struct to pass parameters for nsTextFrame::{Paint,Draw}*

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

Many of those functions have a very long parameter list. It is hard to maintain, and potentially affect performance.

Although using structs may make it a bit verbose in some callsites, it should make things a lot clearer and easier to change.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

This should also cover some of functions in gfxTextRun, so probably also related to Graphics:Text (or probably it is actually more Graphics than Layout?)

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end).

Although this makes some places more complicated, code should generally
be simpler and clearer. It could slightly improve performance on x64 for
functions with more than four parameters, since it makes more data be
passed via registers rather than the stack.

Review commit: https://reviewboard.mozilla.org/r/37521/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37521/

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1251995 part 3 - Use struct to pass params for gfxTextRun::Draw.

Review commit: https://reviewboard.mozilla.org/r/37523/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37523/

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1251995 part 4 - Use struct to pass params for nsTextFrame::DrawText* functions.

Review commit: https://reviewboard.mozilla.org/r/37525/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37525/

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1251995 part 4 - Make nsContextBoxBlur use layout device pixels as unit to avoid repeatedly passing in the conversion factor.

Review commit: https://reviewboard.mozilla.org/r/37527/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37527/

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1251995 part 5 - Unify units of dirty rect used for painting text frame.

Review commit: https://reviewboard.mozilla.org/r/37529/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37529/

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1251995 part 6 - Use struct to pass params for nsTextFrame::PaintText* functions.

Review commit: https://reviewboard.mozilla.org/r/37531/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37531/

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1251995 part 7 - Use struct to pass params for nsTextFrame::Paint*Shadow functions.

Review commit: https://reviewboard.mozilla.org/r/37533/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37533/

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=453f5fe8d78e

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

The motivation of this bug is that I probably need to add a new parameter to some of these functions for bug 1097499...

Using struct to pass parameters we get:
1. callsite verbosity, which is good because it would be eaiser to know what value is for what parameter, and we don't need to take care of the order; and
2. simpler function signature, which is also good as it is not useful to repeat the parameter list in declaration, and fewer function parameters may decrease the overhead of function call and give compilers more opportunity to optimize.

One disadvantage is that we lose the ability to pass reference, which makes it impossible to enforce a must-provide pattern in compile time for some of the parameters. This is probably unsolvable within the current C++.

Comment 11

[Jonathan Kew [:jfkthame]]

I haven't actually read the patches yet, but this seems like good cleanup for some code that's grown pretty crufty over the years -- thanks!

One request: please kick off a try run that includes talos (probably tp5o is the most interesting) tests for the various platforms (better include PGO builds, too), so we can see if there's any visible impact on overall performance. Some of these codepaths can be pretty hot, and it's not always obvious what the perf benefits or costs will be.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Talos: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f401fd595a3

Although it is possible to trigger PGO build on try server, its result may not be comparable, since try server cannot distinguish a PGO build from normal opt builds.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Errrr, I didn't know I need to retrigger talos for my base revision as well... then let's wait...

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

Compare: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1a94e5cfe9e7&newProject=try&newRevision=5f401fd595a3&framework=1&showOnlyImportant=0

It doesn't look like there are significant changes. The two items marked red and orange seem to be noisy themselves. Also I wonder whether my part 4 could have made the result less useful. I probably should separate that one to another bug and investigate it there. I did part 4 for part 5, but it seems it is not that necessary...

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725638 [details]
MozReview Request: Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37521/diff/1-2/

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725639 [details]
MozReview Request: Bug 1251995 part 3 - Use struct to pass params for gfxTextRun::Draw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37523/diff/1-2/

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725640 [details]
MozReview Request: Bug 1251995 part 4 - Use struct to pass params for nsTextFrame::DrawText* functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37525/diff/1-2/

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725642 [details]
MozReview Request: Bug 1251995 part 5 - Unify units of dirty rect used for painting text frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37529/diff/1-2/

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725643 [details]
MozReview Request: Bug 1251995 part 6 - Use struct to pass params for nsTextFrame::PaintText* functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37531/diff/1-2/

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725644 [details]
MozReview Request: Bug 1251995 part 7 - Use struct to pass params for nsTextFrame::Paint*Shadow functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37533/diff/1-2/

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

Removed the old part 4, now there doesn't seem to be any significant change at all: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f9147fcafde4&newProject=try&newRevision=b80d550c4ff5&framework=1

Comment 22

[Jonathan Kew [:jfkthame]]

Comment on attachment 8725638 [details]
MozReview Request: Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end).

https://reviewboard.mozilla.org/r/37521/#review34363

I haven't read all the way through this yet (though I expect it's fine), but I do have a global change to request so I'll go ahead and post this. I added a note inline the first time it came up, but this applies throughout... wherever you've currently got something like:

    SomeMethod(param1, {foo, bar}, moreParams, ...);

to pass a Range, I'd prefer to see

    SomeMethod(param1, Range(foo, bar), moreParams, ...);

as I think that will help readability throughout this code.

::: dom/canvas/CanvasRenderingContext2D.cpp:3503
(Diff revision 2)
> -                                                               mTextRun->GetLength(),
> +        {0, mTextRun->GetLength()},

I think it'd make callsites more readable if you provide a constructor for `gfxTextRun::Range`, and then use it in places like this instead of the anonymous braces.

::: gfx/src/nsFontMetrics.cpp:362
(Diff revision 2)
> +    gfxTextRun::Range range{0, aLength};

And for consistency I'd then go for `range(0, aLength)` here, too, although a declaration like this isn't as cryptic as the typical function callsites.

::: gfx/thebes/gfxTextRun.cpp:977
(Diff revision 2)
> -        *aMetrics = MeasureText(aStart, charsFit, aBoundingBoxType,
> +        *aMetrics = MeasureText({aStart, aStart + charsFit}, aBoundingBoxType,

A local `uint32_t end = aStart + charsFit;` could be used here (three times) to improve readability.

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Jonathan Kew (:jfkthame) from comment #22)
> Comment on attachment 8725638 [details]
> MozReview Request: Bug 1251995 part 1 - Add gfxTextRun::Range to replace
> parameter pairs like (offset, length) and (start, end).
> 
> https://reviewboard.mozilla.org/r/37521/#review34363
> 
> I haven't read all the way through this yet (though I expect it's fine), but
> I do have a global change to request so I'll go ahead and post this. I added
> a note inline the first time it came up, but this applies throughout...
> wherever you've currently got something like:
> 
>     SomeMethod(param1, {foo, bar}, moreParams, ...);
> 
> to pass a Range, I'd prefer to see
> 
>     SomeMethod(param1, Range(foo, bar), moreParams, ...);
> 
> as I think that will help readability throughout this code.

Hmmm, many places would actually need gfxTextRun::Range() rather than just Range(), which would make code much longer and harder to arrange.

The main idea of introducing gfxTextRun::Range is to make start/end (and offset/length) an integrated pair. I think using brace should enough make it clear that it is an integrated pair, which should not be more cryptic, and actually should have been much better than the current form for callsites. It seems to me having their type listed there isn't that important.

Do you agree with this argument?

> ::: dom/canvas/CanvasRenderingContext2D.cpp:3503
> (Diff revision 2)
> > -                                                               mTextRun->GetLength(),
> > +        {0, mTextRun->GetLength()},
> 
> I think it'd make callsites more readable if you provide a constructor for
> `gfxTextRun::Range`, and then use it in places like this instead of the
> anonymous braces.

I agree that adding a constructor for whole range of a text run makes sense.

Comment 24

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #23)
> (In reply to Jonathan Kew (:jfkthame) from comment #22)
> > ::: dom/canvas/CanvasRenderingContext2D.cpp:3503
> > (Diff revision 2)
> > > -                                                               mTextRun->GetLength(),
> > > +        {0, mTextRun->GetLength()},
> > 
> > I think it'd make callsites more readable if you provide a constructor for
> > `gfxTextRun::Range`, and then use it in places like this instead of the
> > anonymous braces.
> 
> I agree that adding a constructor for whole range of a text run makes sense.

Hmmm... 
> gfxTextRun::Range(mTextRun)
is actually longer than
> {0, mTextRun->GetLength()}
which makes me wonder whether I really should add it...

Comment 25

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #23)
> (In reply to Jonathan Kew (:jfkthame) from comment #22)
> Hmmm, many places would actually need gfxTextRun::Range() rather than just
> Range(), which would make code much longer and harder to arrange.

Other classes that call these methods, and need to construct a gfxTextRun::Range to pass, can just do their own

    typedef gfxTextRun::Range Range;

to avoid needing to use the long form at all the callsites.

I'd still prefer to see  Range(a, b)  rather than bare  {a, b}  in all those parameter lists. If we're going to touch all these lines of code, let's go for the most-readable option. IMO that wins over being a few characters shorter.


(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #24)

> Hmmm... 
> > gfxTextRun::Range(mTextRun)
> is actually longer than
> > {0, mTextRun->GetLength()}
> which makes me wonder whether I really should add it...

It becomes just

    Range(mTextRun)

if class (or method) using it has done its own typedef, as above.

Comment 26

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1251995 part 1 - Add helper functions to simplify code.

Review commit: https://reviewboard.mozilla.org/r/38191/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38191/

Comment 27

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725638 [details]
MozReview Request: Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37521/diff/2-3/

Comment 28

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725639 [details]
MozReview Request: Bug 1251995 part 3 - Use struct to pass params for gfxTextRun::Draw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37523/diff/2-3/

Comment 29

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725640 [details]
MozReview Request: Bug 1251995 part 4 - Use struct to pass params for nsTextFrame::DrawText* functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37525/diff/2-3/

Comment 30

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725642 [details]
MozReview Request: Bug 1251995 part 5 - Unify units of dirty rect used for painting text frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37529/diff/2-3/

Comment 31

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725643 [details]
MozReview Request: Bug 1251995 part 6 - Use struct to pass params for nsTextFrame::PaintText* functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37531/diff/2-3/

Comment 32

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725644 [details]
MozReview Request: Bug 1251995 part 7 - Use struct to pass params for nsTextFrame::Paint*Shadow functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37533/diff/2-3/

Comment 33

[Jonathan Kew [:jfkthame]]

Comment on attachment 8726695 [details]
MozReview Request: Bug 1251995 part 1 - Add helper functions to simplify code.

https://reviewboard.mozilla.org/r/38191/#review34713

Yes, this is some nice cleanup, thanks.

Comment 34

[Jonathan Kew [:jfkthame]]

Comment on attachment 8725638 [details]
MozReview Request: Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end).

https://reviewboard.mozilla.org/r/37521/#review34719

Comment 35

[Jonathan Kew [:jfkthame]]

Comment on attachment 8725639 [details]
MozReview Request: Bug 1251995 part 3 - Use struct to pass params for gfxTextRun::Draw.

https://reviewboard.mozilla.org/r/37523/#review34913

Comment 36

[Jonathan Kew [:jfkthame]]

Comment on attachment 8725640 [details]
MozReview Request: Bug 1251995 part 4 - Use struct to pass params for nsTextFrame::DrawText* functions.

https://reviewboard.mozilla.org/r/37525/#review34915

::: layout/generic/nsTextFrame.h:404
(Diff revision 3)
> +  };

For the fields that don't have a default, could we set them in the constructor, so that it's guaranteed that they are never left uninitialized?

::: layout/generic/nsTextFrame.h:415
(Diff revision 3)
> +  };

Likewise, can we simply set framePt, dirtyRect, textStyle and clipEdges with the constructor?

Comment 37

[Jonathan Kew [:jfkthame]]

Comment on attachment 8725642 [details]
MozReview Request: Bug 1251995 part 5 - Unify units of dirty rect used for painting text frame.

https://reviewboard.mozilla.org/r/37529/#review34917

Comment 38

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Jonathan Kew (:jfkthame) from comment #36)
> Comment on attachment 8725640 [details]
> MozReview Request: Bug 1251995 part 4 - Use struct to pass params for
> nsTextFrame::DrawText* functions.
> 
> https://reviewboard.mozilla.org/r/37525/#review34915
> 
> ::: layout/generic/nsTextFrame.h:404
> (Diff revision 3)
> > +  };
> 
> For the fields that don't have a default, could we set them in the
> constructor, so that it's guaranteed that they are never left uninitialized?

I tend not to do so as it ruins callsite verbosity and bloat single function param list again... Actually I'm wondering whether I should also pull |context| out from the constructor.

Using uninitialized field here should cause runtime bustage on ASAN build, since the code paths here should have already been well covered in our tests. So I'm not too concerned about this. Also, hoping ASAN to catch such mistakes is the reason I didn't give every field a default value.

Comment 39

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #38)
> (In reply to Jonathan Kew (:jfkthame) from comment #36)
> Using uninitialized field here should cause runtime bustage on ASAN build,
> since the code paths here should have already been well covered in our
> tests. So I'm not too concerned about this. Also, hoping ASAN to catch such
> mistakes is the reason I didn't give every field a default value.

Will ASAN catch reading an uninitialized field (e.g. a bool like |drawSoftHyphen|)? Valgrind would, but we don't routinely run our test suite under valgrind, afaik...

Comment 40

[Xidorn Quan [:xidorn] UTC+11]

Hmmm... It seems you're right that ASAN does not catch that... But I still don't want to bloat the param list of a single function...

We may need some new technique here...

Comment 41

[Xidorn Quan [:xidorn] UTC+11]

Probably we should just initialize all things by default in the structure, and add assertions in callees if necessary.

Comment 42

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8726695 [details]
MozReview Request: Bug 1251995 part 1 - Add helper functions to simplify code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38191/diff/1-2/

Comment 43

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725638 [details]
MozReview Request: Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37521/diff/3-4/

Comment 44

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725639 [details]
MozReview Request: Bug 1251995 part 3 - Use struct to pass params for gfxTextRun::Draw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37523/diff/3-4/

Comment 45

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725640 [details]
MozReview Request: Bug 1251995 part 4 - Use struct to pass params for nsTextFrame::DrawText* functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37525/diff/3-4/

Comment 46

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725642 [details]
MozReview Request: Bug 1251995 part 5 - Unify units of dirty rect used for painting text frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37529/diff/3-4/

Comment 47

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725643 [details]
MozReview Request: Bug 1251995 part 6 - Use struct to pass params for nsTextFrame::PaintText* functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37531/diff/3-4/

Comment 48

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8725644 [details]
MozReview Request: Bug 1251995 part 7 - Use struct to pass params for nsTextFrame::Paint*Shadow functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37533/diff/3-4/

Comment 49

[Xidorn Quan [:xidorn] UTC+11]

https://reviewboard.mozilla.org/r/37525/#review34915

> For the fields that don't have a default, could we set them in the constructor, so that it's guaranteed that they are never left uninitialized?

In the new revision, I give all of them a default value, so that they would never be left uninitialized. We could observe determinated behavior if they are not initialized correctly.

> Likewise, can we simply set framePt, dirtyRect, textStyle and clipEdges with the constructor?

I'm not too concerned about framePt and dirtyRect. gfxPoint and gfxRect have default constructor which initializes their fields to zero. And I've also changed part 2 to make gfxTextRun::Range also do that. The pointer fields all have a default nullptr value now, so we would see nullptr dereference crash if they are not initialized properly.

Comment 50

[Jonathan Kew [:jfkthame]]

Comment on attachment 8725640 [details]
MozReview Request: Bug 1251995 part 4 - Use struct to pass params for nsTextFrame::DrawText* functions.

https://reviewboard.mozilla.org/r/37525/#review34989

OK, I like this better now - thanks. :)

Comment 51

[Jonathan Kew [:jfkthame]]

Comment on attachment 8725643 [details]
MozReview Request: Bug 1251995 part 6 - Use struct to pass params for nsTextFrame::PaintText* functions.

https://reviewboard.mozilla.org/r/37531/#review34991

Comment 52

[Jonathan Kew [:jfkthame]]

https://reviewboard.mozilla.org/r/37531/#review34997

Comment 53

[Jonathan Kew [:jfkthame]]

Comment on attachment 8725644 [details]
MozReview Request: Bug 1251995 part 7 - Use struct to pass params for nsTextFrame::Paint*Shadow functions.

https://reviewboard.mozilla.org/r/37533/#review34999

Comment 54

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d88221aad093
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf004c055beb
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f9fbbef0294
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b31d6fb1fcb
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac1d42c669be
https://hg.mozilla.org/integration/mozilla-inbound/rev/c36e12ded845
https://hg.mozilla.org/integration/mozilla-inbound/rev/734559c25cb1

Comment 55

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cacb08f6f5ca0f3345288c5ed4d707352a22fe9
Bug 1251995 followup - Fix declaration shadow error.

Comment 56

[Carsten Book [:Tomcat]]

backed out for test failures in reftests like https://treeherder.mozilla.org/logviewer.html#?job_id=23143811&repo=mozilla-inbound

Comment 57

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/383555f19d02
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7008b66f2d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ffb1b561dde
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eb75d38d55e
https://hg.mozilla.org/integration/mozilla-inbound/rev/96ae983fac67
https://hg.mozilla.org/integration/mozilla-inbound/rev/345a475be5f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/90dffe75068e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e50ce2dc72f3

Comment 58

[Jonathan Kew [:jfkthame]]

Comment on attachment 8725638 [details]
MozReview Request: Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end).

https://reviewboard.mozilla.org/r/37521/#review35017

::: layout/generic/nsTextFrame.cpp:6624
(Diff revision 4)
> -      gfxFloat hyphenBaselineX = aTextBaselinePt.x + mTextRun->GetDirection() * aAdvanceWidth -
> +      gfxFloat hyphenBaselineX =

This looks like it's probably the cause of those reftest failures. We still need aAdvanceWidth incorporated into the hyphen's position...

Comment 59

[Xidorn Quan [:xidorn] UTC+11]

https://reviewboard.mozilla.org/r/37521/#review35017

> This looks like it's probably the cause of those reftest failures. We still need aAdvanceWidth incorporated into the hyphen's position...

Hmmm... this is probably because of merge or even automerge...

Comment 60

[Xidorn Quan [:xidorn] UTC+11]

There are several other reftest failures on OS X and Android... Probably I cannot fix them today then :(

Comment 61

[Xidorn Quan [:xidorn] UTC+11]

It seems the failures on OS X and Android are from part 5, which unifies the unit of dirty rect. It seems the units in text frame are pretty... complicated...

Comment 62

[Xidorn Quan [:xidorn] UTC+11]

Hopefully all green now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef89e9923b24

Comment 63

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51f3d3a22d4251b8f4c0e70cb5161d75a46dfea2
Bug 1251995 part 1 - Add helper functions to simplify code. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d462b564421dc46a14c0d2b538413cd623bd37e
Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end). r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dcad93e1045080c6f2476e296e8f52c1549cf7d
Bug 1251995 part 3 - Use struct to pass params for gfxTextRun::Draw. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/788178d64bf45b07070fffc96ea9d1640363f7a3
Bug 1251995 part 4 - Use struct to pass params for nsTextFrame::DrawText* functions. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5a5660267aeb4254d0dd9cc15ff2d1980790e4e
Bug 1251995 part 5 - Unify units of dirty rect used for painting text frame. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/366047809301992f99d3160b099063d16adee2a7
Bug 1251995 part 6 - Use struct to pass params for nsTextFrame::PaintText* functions. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffce50b96ca9482490e590735e694991e9085b11
Bug 1251995 part 7 - Use struct to pass params for nsTextFrame::Paint*Shadow functions. r=jfkthame

Comment 64

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/51f3d3a22d42
https://hg.mozilla.org/mozilla-central/rev/0d462b564421
https://hg.mozilla.org/mozilla-central/rev/8dcad93e1045
https://hg.mozilla.org/mozilla-central/rev/788178d64bf4
https://hg.mozilla.org/mozilla-central/rev/b5a5660267ae
https://hg.mozilla.org/mozilla-central/rev/366047809301
https://hg.mozilla.org/mozilla-central/rev/ffce50b96ca9