Bug 133932 - Subpixel layout: Switch inlines' baseline positioning from int to LayoutUnit.
Summary: Subpixel layout: Switch inlines' baseline positioning from int to LayoutUnit.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks: 133575 136332 158441
  Show dependency treegraph
 
Reported: 2014-06-15 20:51 PDT by zalan
Modified: 2021-04-02 02:08 PDT (History)
19 users (show)

See Also:


Attachments
Patch (68.79 KB, patch)
2021-03-29 09:18 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (69.15 KB, patch)
2021-03-30 06:27 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (72.17 KB, patch)
2021-03-30 09:00 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (72.11 KB, patch)
2021-03-30 10:41 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (72.11 KB, patch)
2021-03-31 00:30 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (71.73 KB, patch)
2021-04-02 00:30 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2014-06-15 20:51:25 PDT
This is mainly about *::baselinePosition() *::firstLineBaseline() *::inlineBlockBaseline()
Comment 1 Rob Buis 2021-03-29 09:18:05 PDT
Created attachment 424534 [details]
Patch
Comment 2 Rob Buis 2021-03-30 06:27:42 PDT
Created attachment 424633 [details]
Patch
Comment 3 Rob Buis 2021-03-30 09:00:52 PDT
Created attachment 424645 [details]
Patch
Comment 4 Rob Buis 2021-03-30 10:41:12 PDT
Created attachment 424658 [details]
Patch
Comment 5 zalan 2021-03-30 20:24:50 PDT
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)).
Comment 6 Rob Buis 2021-03-31 00:30:54 PDT
Created attachment 424739 [details]
Patch
Comment 7 Rob Buis 2021-03-31 07:58:55 PDT
(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.
Comment 8 zalan 2021-04-01 13:11:11 PDT
(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?
Comment 9 Rob Buis 2021-04-01 13:30:07 PDT
(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.
Comment 10 zalan 2021-04-01 13:47:05 PDT
(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.
Comment 11 zalan 2021-04-01 13:47:51 PDT
>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.
Comment 12 Rob Buis 2021-04-01 13:49:27 PDT
(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 :)
Comment 13 zalan 2021-04-01 14:06:03 PDT
(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.
Comment 14 Rob Buis 2021-04-02 00:30:27 PDT
Created attachment 424993 [details]
Patch
Comment 15 EWS 2021-04-02 01:29:53 PDT
commit-queue failed to commit attachment 424993 [details] to WebKit repository. To retry, please set cq+ flag again.
Comment 16 EWS 2021-04-02 02:07:46 PDT
Committed r275413: <https://commits.webkit.org/r275413>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424993 [details].
Comment 17 Radar WebKit Bug Importer 2021-04-02 02:08:19 PDT
<rdar://problem/76143175>