Bug 133932 – Subpixel layout: Switch inlines' baseline positioning from int to LayoutUnit.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

This is mainly about *::baselinePosition() *::firstLineBaseline() *::inlineBlockBaseline()

[Rob Buis]

Created attachment 424534 [details]
Patch

[Rob Buis]

Created attachment 424633 [details]
Patch

[Rob Buis]

Created attachment 424645 [details]
Patch

[Rob Buis]

Created attachment 424658 [details]
Patch

[zalan]

My general advice (but it's really up to you) is to use auto whenever possible as well as uniform initialization (and also make this patch strictly about int -> LayoutUnit (e.g. do not convert special values to Optional)).

[Rob Buis]

Created attachment 424739 [details]
Patch

[Rob Buis]

(In reply to zalan from comment #5)
> My general advice (but it's really up to you) is to use auto whenever
> possible as well as uniform initialization (and also make this patch
> strictly about int -> LayoutUnit (e.g. do not convert special values to
> Optional)).

Thanks, I tried to do this in the latest patch. Also I try to get away with the minimum amount of explicit toInt() calls, just enough to keep the bots green.

Follow up work (removing more toInt()) will likely be smaller (code) patches, since these small changes may need a lot of rebaselining of the tests, big changes may cause huge patches/rebaselining.

[zalan]

(In reply to Rob Buis from comment #7)
> (In reply to zalan from comment #5)
> > My general advice (but it's really up to you) is to use auto whenever
> > possible as well as uniform initialization (and also make this patch
> > strictly about int -> LayoutUnit (e.g. do not convert special values to
> > Optional)).
> 
> Thanks, I tried to do this in the latest patch. Also I try to get away with
> the minimum amount of explicit toInt() calls, just enough to keep the bots
> green.
> 
> Follow up work (removing more toInt()) will likely be smaller (code)
> patches, since these small changes may need a lot of rebaselining of the
> tests, big changes may cause huge patches/rebaselining.
Sounds good. Any particular reason you left in the "-1 -> Optional" transition?

[Rob Buis]

(In reply to zalan from comment #8)
> (In reply to Rob Buis from comment #7)
> > (In reply to zalan from comment #5)
> > > My general advice (but it's really up to you) is to use auto whenever
> > > possible as well as uniform initialization (and also make this patch
> > > strictly about int -> LayoutUnit (e.g. do not convert special values to
> > > Optional)).
> > 
> > Thanks, I tried to do this in the latest patch. Also I try to get away with
> > the minimum amount of explicit toInt() calls, just enough to keep the bots
> > green.
> > 
> > Follow up work (removing more toInt()) will likely be smaller (code)
> > patches, since these small changes may need a lot of rebaselining of the
> > tests, big changes may cause huge patches/rebaselining.
> Sounds good. Any particular reason you left in the "-1 -> Optional"
> transition?

In GridBaselineAlignment? When doing some work on flexbox, there was a preference for doing -1 -> Optional changes, which is also my preference, but if you want to keep that as-is, that is also fine by me. I can't quite remember if not doing that would make tests fail, but I lean towards thinking there is no difference in behaviour either way.

[zalan]

(In reply to Rob Buis from comment #9)
> (In reply to zalan from comment #8)
> > (In reply to Rob Buis from comment #7)
> > > (In reply to zalan from comment #5)
> > > > My general advice (but it's really up to you) is to use auto whenever
> > > > possible as well as uniform initialization (and also make this patch
> > > > strictly about int -> LayoutUnit (e.g. do not convert special values to
> > > > Optional)).
> > > 
> > > Thanks, I tried to do this in the latest patch. Also I try to get away with
> > > the minimum amount of explicit toInt() calls, just enough to keep the bots
> > > green.
> > > 
> > > Follow up work (removing more toInt()) will likely be smaller (code)
> > > patches, since these small changes may need a lot of rebaselining of the
> > > tests, big changes may cause huge patches/rebaselining.
> > Sounds good. Any particular reason you left in the "-1 -> Optional"
> > transition?
> 
> In GridBaselineAlignment? When doing some work on flexbox, there was a
> preference for doing -1 -> Optional changes, which is also my preference,
> but if you want to keep that as-is, that is also fine by me. I can't quite
> remember if not doing that would make tests fail, but I lean towards
> thinking there is no difference in behaviour either way.
Ok, I just prefer a clean int -> LayoutUnit patch to make things like tracking down regressions easier.

[zalan]

>there was a preference for doing -1 -> Optional changes, which is also my preference
that's absolutely my preference too. just not in this patch.

[Rob Buis]

(In reply to zalan from comment #11)
> >there was a preference for doing -1 -> Optional changes, which is also my preference
> that's absolutely my preference too. just not in this patch.

Sure, makes sense. I'll remove that part tomorrow and check nothing breaks :)

[zalan]

(In reply to Rob Buis from comment #12)
> (In reply to zalan from comment #11)
> > >there was a preference for doing -1 -> Optional changes, which is also my preference
> > that's absolutely my preference too. just not in this patch.
> 
> Sure, makes sense. I'll remove that part tomorrow and check nothing breaks :)
Thanks. Let me just r+/cq- this patch so that tomorrow you don't need to ask for review again.

[Rob Buis]

Created attachment 424993 [details]
Patch

[EWS]

commit-queue failed to commit attachment 424993 [details] to WebKit repository. To retry, please set cq+ flag again.

[EWS]

Committed r275413: <https://commits.webkit.org/r275413>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424993 [details].

[Radar WebKit Bug Importer]

<rdar://problem/76143175>