[LayoutNG] Logicalize NGPaintBorderEdge

[LayoutNG] Logicalize NGPaintBorderEdge

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng

Review-Url: https://codereview.chromium.org/2912273002
Cr-Commit-Position: refs/heads/master@{#476202}
Committed: https://chromium.googlesource.com/chromium/src/+/b9fffd37375ba984168be95d9ea5aa0f2b5e88d8

[kojii, 2017-05-31 04:26:54]

Description was changed from

==========
ngedges

BUG=
==========

to

==========
ngedges

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[kojii, 2017-05-31 04:28:51]

Description was changed from

==========
ngedges

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Logicalize NGPaintBorderEdge

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[kojii, 2017-05-31 04:41:24]

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

[kojii, 2017-05-31 04:41:25]

PTAL. I'm trying to set this from inline layout.

WDYT? I don't have clear idea the best way to do this. "enum class" is
troublesome to bit-or/and. Tried overriding operators before but didn't like it
very much.

Having two namespaces (logical/physical) makes the code redundant; e.g.,
  NGPhysicalBorderEdges::NGPhysicalBorderEdges border_edges;

https://codereview.chromium.org/2912273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/geometry/ng_border_edges.h
(right):

https://codereview.chromium.org/2912273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/geometry/ng_border_edges.h:13:
namespace NGBorderEdges {
"Edges" or "Edge"?

https://codereview.chromium.org/2912273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.cc (right):

https://codereview.chromium.org/2912273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.cc:56: : .0f));
Can move to ng_border_edges.cc to avoid wrapping?

[ikilpatrick, 2017-05-31 13:52:57]

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

[ikilpatrick, 2017-05-31 13:52:58]

lgtm

https://codereview.chromium.org/2912273002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_physical_box_fragment.cc
(right):

https://codereview.chromium.org/2912273002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_physical_box_fragment.cc:30:
border_edge_ = border_edges;
this can't be part of the initializer list?

[kojii, 2017-05-31 15:09:36]

Thank you for the prompt review. See replies inline.

https://codereview.chromium.org/2912273002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_physical_box_fragment.cc
(right):

https://codereview.chromium.org/2912273002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_physical_box_fragment.cc:30:
border_edge_ = border_edges;
On 2017/05/31 at 13:52:58, ikilpatrick wrote:
> this can't be part of the initializer list?

No, because |border_edges_| is in parent's class. I'm not sure how it ended up
in NGPhysicalFragment, not in NGPhysicalBoxFragment, I suppose it's only needed
for Box, but put in NGPhysicalFragment can save memory?

I can:
a. Move this to constructor argument if other subclasses might use it.
b. Move the field and functions down to Box (and thus can use initializer list).
c. Fields can stay in NGPhysicalFragment to save memory, but move functions down
to Box.
d. Add TODO for now, we haven't determined yet if other fragments are going to
use this or not.

I guessed d., too many unknowns in paint yet and we expect cleanup later, but if
you have any directions in mind, I'd be happy to learn.

[ikilpatrick, 2017-05-31 16:44:37]

https://codereview.chromium.org/2912273002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_physical_box_fragment.cc
(right):

https://codereview.chromium.org/2912273002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_physical_box_fragment.cc:30:
border_edge_ = border_edges;
On 2017/05/31 15:09:36, kojii wrote:
> On 2017/05/31 at 13:52:58, ikilpatrick wrote:
> > this can't be part of the initializer list?
> 
> No, because |border_edges_| is in parent's class. I'm not sure how it ended up
> in NGPhysicalFragment, not in NGPhysicalBoxFragment, I suppose it's only
needed
> for Box, but put in NGPhysicalFragment can save memory?
> 
> I can:
> a. Move this to constructor argument if other subclasses might use it.
> b. Move the field and functions down to Box (and thus can use initializer
list).
> c. Fields can stay in NGPhysicalFragment to save memory, but move functions
down
> to Box.
> d. Add TODO for now, we haven't determined yet if other fragments are going to
> use this or not.
> 
> I guessed d., too many unknowns in paint yet and we expect cleanup later, but
if
> you have any directions in mind, I'd be happy to learn.

Oh this is fine then, sorry I didn't realize it was in another class. 

still lgtm.

[kojii, 2017-05-31 16:46:44]

Thank you!

[kojii, 2017-05-31 16:47:19]

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

[commit-bot: I haz the power, 2017-05-31 16:47:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 18:35:40]

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

[commit-bot: I haz the power, 2017-05-31 18:35:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kojii, 2017-06-01 05:47:16]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2017-06-01 05:47:30]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 05:51:46]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496296036472200,
"parent_rev": "aa3ce260e5248cc75e61ca7540652c74dee4bf55", "commit_rev":
"b9fffd37375ba984168be95d9ea5aa0f2b5e88d8"}

[commit-bot: I haz the power, 2017-06-01 05:51:57]

Description was changed from

==========
[LayoutNG] Logicalize NGPaintBorderEdge

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Logicalize NGPaintBorderEdge

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng

Review-Url: https://codereview.chromium.org/2912273002
Cr-Commit-Position: refs/heads/master@{#476202}
Committed:
https://chromium.googlesource.com/chromium/src/+/b9fffd37375ba984168be95d9ea5...
==========

[commit-bot: I haz the power, 2017-06-01 05:52:01]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b9fffd37375ba984168be95d9ea5...