[LayoutNG] Use NGLogical[Offset|Size] for NGFragment

[LayoutNG] Use NGLogical[Offset|Size] for NGFragment

Replace use of raw LayoutUnits and custom physical to logical conversion
logic in NGFragment and instead expose Size() and Offset() methods using
the NGLogicalOffset and NGLogicalOffset types respectively. This reduces
not only code duplication but also the number of coordinate conversions.

More importantly it limits the use of raw LayoutUnits and the logical vs
physical coordinate ambiguity by using compiler enforced explicit types.

R=ikilpatrick@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng

[eae, 2017-04-10 10:09:06]

Description was changed from

==========
[LayoutNG] Use NGLogical[Offset|Size] for NGFragment

Replace use of raw LayoutUnits and custom physical to logical conversion
logic in NGFragment and instead expose Size() and Offset() methods using
the NGLogical[Offset|Size]. This reduces code duplication and the number
of times a conversion need to take place. More importantly it limits the
use of raw LayoutUnits and the logical vs physical coordinate ambiguity.

R=ikilpatrick@chromium.org
==========

to

==========
[LayoutNG] Use NGLogical[Offset|Size] for NGFragment

Replace use of raw LayoutUnits and custom physical to logical conversion
logic in NGFragment and instead expose Size() and Offset() methods using
the NGLogical[Offset|Size]. This reduces code duplication and the number
of times a conversion need to take place. More importantly it limits the
use of raw LayoutUnits and the logical vs physical coordinate ambiguity.

R=ikilpatrick@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[eae, 2017-04-10 10:12:27]

The CQ bit was checked by eae@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 10:12:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eae, 2017-04-10 11:22:44]

Description was changed from

==========
[LayoutNG] Use NGLogical[Offset|Size] for NGFragment

Replace use of raw LayoutUnits and custom physical to logical conversion
logic in NGFragment and instead expose Size() and Offset() methods using
the NGLogical[Offset|Size]. This reduces code duplication and the number
of times a conversion need to take place. More importantly it limits the
use of raw LayoutUnits and the logical vs physical coordinate ambiguity.

R=ikilpatrick@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Use NGLogical[Offset|Size] for NGFragment

Replace use of raw LayoutUnits and custom physical to logical conversion
logic in NGFragment and instead expose Size() and Offset() methods using
the NGLogicalOffset and NGLogicalOffset types respectively. This reduces
not only code duplication but also the number of coordinate conversions.

More importantly it limits the use of raw LayoutUnits and the logical vs
physical coordinate ambiguity by using compiler enforced explicit types.

R=ikilpatrick@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[commit-bot: I haz the power, 2017-04-10 11:54:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 11:54:19]

Dry run: This issue passed the CQ dry run.

[eae, 2017-04-10 16:00:14]

eae@chromium.org changed reviewers:
+ cbiesinger@chromium.org

[cbiesinger, 2017-04-10 20:55:51]

https://codereview.chromium.org/2807113002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/geometry/ng_physical_offset.cc
(right):

https://codereview.chromium.org/2807113002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/geometry/ng_physical_offset.cc:14: :
NGLogicalOffset(top, left);
Is that correct? For vertical-rl, I am not sure that left is the inline offset,
shouldn't it be width-left?

[atotic, 2017-04-10 22:44:34]

atotic@chromium.org changed reviewers:
+ atotic@chromium.org

[atotic, 2017-04-10 22:44:35]

https://codereview.chromium.org/2807113002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/geometry/ng_physical_offset.cc
(right):

https://codereview.chromium.org/2807113002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/geometry/ng_physical_offset.cc:14: :
NGLogicalOffset(top, left);
On 2017/04/10 at 20:55:51, cbiesinger wrote:
> Is that correct? For vertical-rl, I am not sure that left is the inline
offset, shouldn't it be width-left?

I agree. 

Computing physical offset of fragment from logical might be even more
complicated.

In algorithms:
for physical position, you want offset of top/left corner.
for logical position, you want offset of inline-start/block-start corner.

These corners are not the same in vertical modes. inline-start/block-start is
right/bottom corner in vertical-rl.

To compute these, you need two sizes, parent and fragment.

There should be methods on NGPhysicalFragment that also expose PhysicalSize and
PhysicalOffset.

[eae, 2017-04-11 03:59:02]

On 2017/04/10 20:55:51, cbiesinger wrote:
>
https://codereview.chromium.org/2807113002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/ng/geometry/ng_physical_offset.cc
> (right):
> 
>
https://codereview.chromium.org/2807113002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/ng/geometry/ng_physical_offset.cc:14: :
> NGLogicalOffset(top, left);
> Is that correct? For vertical-rl, I am not sure that left is the inline
offset,
> shouldn't it be width-left?

I believe you are right, yet another reason we need to use the proper units
rather than raw pixel values! And clearly we need more tests given this change
passed all of the existing ones!

[eae, 2017-04-11 05:09:41]

On 2017/04/10 22:44:35, atotic wrote:
>
https://codereview.chromium.org/2807113002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/ng/geometry/ng_physical_offset.cc
> (right):
> 
>
https://codereview.chromium.org/2807113002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/ng/geometry/ng_physical_offset.cc:14: :
> NGLogicalOffset(top, left);
> On 2017/04/10 at 20:55:51, cbiesinger wrote:
> > Is that correct? For vertical-rl, I am not sure that left is the inline
> offset, shouldn't it be width-left?
> 
> I agree. 
> 
> Computing physical offset of fragment from logical might be even more
> complicated.
> 
> In algorithms:
> for physical position, you want offset of top/left corner.
> for logical position, you want offset of inline-start/block-start corner.
> 
> These corners are not the same in vertical modes. inline-start/block-start is
> right/bottom corner in vertical-rl.
> 
> To compute these, you need two sizes, parent and fragment.
> 
> There should be methods on NGPhysicalFragment that also expose PhysicalSize
and
> PhysicalOffset.

Those are great suggestions, I'll update the Cl.

Thanks!

[eae, 2017-04-11 05:09:44]

On 2017/04/10 22:44:35, atotic wrote:
>
https://codereview.chromium.org/2807113002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/ng/geometry/ng_physical_offset.cc
> (right):
> 
>
https://codereview.chromium.org/2807113002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/ng/geometry/ng_physical_offset.cc:14: :
> NGLogicalOffset(top, left);
> On 2017/04/10 at 20:55:51, cbiesinger wrote:
> > Is that correct? For vertical-rl, I am not sure that left is the inline
> offset, shouldn't it be width-left?
> 
> I agree. 
> 
> Computing physical offset of fragment from logical might be even more
> complicated.
> 
> In algorithms:
> for physical position, you want offset of top/left corner.
> for logical position, you want offset of inline-start/block-start corner.
> 
> These corners are not the same in vertical modes. inline-start/block-start is
> right/bottom corner in vertical-rl.
> 
> To compute these, you need two sizes, parent and fragment.
> 
> There should be methods on NGPhysicalFragment that also expose PhysicalSize
and
> PhysicalOffset.

Those are great suggestions, I'll update the Cl.

Thanks!