[LayoutNG] Move remaining ng_units structs to their own files

[LayoutNG] Move remaining ng_units structs to their own files

R=mstensho@opera.com

Review-Url: https://codereview.chromium.org/2721613003
Cr-Commit-Position: refs/heads/master@{#453878}
Committed: https://chromium.googlesource.com/chromium/src/+/480fa30589acd71427e3f902fd7916e87c41f5d9

[eae, 2017-02-28 06:01:38]

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

[commit-bot: I haz the power, 2017-02-28 06:02:11]

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

[eae, 2017-02-28 06:03:31]

Description was changed from

==========
[LayoutNG] Move remaining ng_units structs to their own files

R=cbiesinger@chromium.org
==========

to

==========
[LayoutNG] Move remaining ng_units structs to their own files

R=mstensho@opera.com
==========

[eae, 2017-02-28 06:03:31]

eae@chromium.org changed reviewers:
+ mstensho@opera.com
- cbiesinger@chromium.org

[eae, 2017-02-28 06:03:42]

Move all the structs!

[eae, 2017-02-28 06:07:27]

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

[commit-bot: I haz the power, 2017-02-28 06:07:50]

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-02-28 07:25:01]

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

[commit-bot: I haz the power, 2017-02-28 07:25:02]

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_...)

[mstensho (USE GERRIT), 2017-02-28 12:19:47]

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/geometry/ng_box_strut.h (right):

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/geometry/ng_box_strut.h:9: #include
"core/layout/ng/ng_writing_mode.h"
Random thought (not something you necessarily need to address right now): Should
files in layout/ng/geometry/ really be allowed to access things in layout/ng/ ,
or should that be considered a layer violation?

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/geometry/ng_edge.h (right):

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/geometry/ng_edge.h:14: struct
CORE_EXPORT NGEdge {
Such an adorable tiny file! <3

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/geometry/ng_margin_strut.h
(right):

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/geometry/ng_margin_strut.h:13: #define
NGSizeIndefinite LayoutUnit(-1)
Also #defined in ng_logical_size.h

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_exclusion.h (right):

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_exclusion.h:39: return stream <<
value.ToString();
move implementation to .cc ?

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_min_max_content_size.h (right):

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_min_max_content_size.h:14: struct
CORE_EXPORT MinAndMaxContentSizes {
Should this be named NGMinMaxContentSize, to match the file name? Or should the
file name be core/layout/ng/min_and_max_content_sizes.h ?

[eae, 2017-02-28 12:26:34]

Thanks Morten!

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/geometry/ng_box_strut.h (right):

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/geometry/ng_box_strut.h:9: #include
"core/layout/ng/ng_writing_mode.h"
On 2017/02/28 12:19:47, mstensho wrote:
> Random thought (not something you necessarily need to address right now):
Should
> files in layout/ng/geometry/ really be allowed to access things in layout/ng/
,
> or should that be considered a layer violation?

Ideally long term (post NG) all of these files should move to
Source/platform/geometry (and drop the ng prefix). As such writing mode should
be in  Source/platform and not in Source/core/layout/ng. For now though this is
probably fine.

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/geometry/ng_edge.h (right):

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/geometry/ng_edge.h:14: struct
CORE_EXPORT NGEdge {
On 2017/02/28 12:19:47, mstensho wrote:
> Such an adorable tiny file! <3

<3

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/geometry/ng_margin_strut.h
(right):

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/geometry/ng_margin_strut.h:13: #define
NGSizeIndefinite LayoutUnit(-1)
On 2017/02/28 12:19:47, mstensho wrote:
> Also #defined in ng_logical_size.h

Opps, good catch!

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_exclusion.h (right):

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_exclusion.h:39: return stream <<
value.ToString();
On 2017/02/28 12:19:47, mstensho wrote:
> move implementation to .cc ?

Will do, thanks!

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_min_max_content_size.h (right):

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_min_max_content_size.h:14: struct
CORE_EXPORT MinAndMaxContentSizes {
On 2017/02/28 12:19:47, mstensho wrote:
> Should this be named NGMinMaxContentSize, to match the file name? Or should
the
> file name be core/layout/ng/min_and_max_content_sizes.h ?

Yes! :) I'll rename one of them.

[mstensho (USE GERRIT), 2017-02-28 12:31:29]

lgtm if you make those changes. :)

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/geometry/ng_box_strut.h (right):

https://codereview.chromium.org/2721613003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/geometry/ng_box_strut.h:9: #include
"core/layout/ng/ng_writing_mode.h"
On 2017/02/28 12:26:34, eae wrote:
> On 2017/02/28 12:19:47, mstensho wrote:
> > Random thought (not something you necessarily need to address right now):
> Should
> > files in layout/ng/geometry/ really be allowed to access things in
layout/ng/
> ,
> > or should that be considered a layer violation?
> 
> Ideally long term (post NG) all of these files should move to
> Source/platform/geometry (and drop the ng prefix). As such writing mode should
> be in  Source/platform and not in Source/core/layout/ng. For now though this
is
> probably fine. 

Acknowledged.

Makes sense. Thanks.

[eae, 2017-03-01 00:20:10]

The CQ bit was checked by eae@chromium.org

[eae, 2017-03-01 00:20:11]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/2721613003/#ps60001
(title: "Address reviewer comments")

[commit-bot: I haz the power, 2017-03-01 00:20:57]

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-03-01 01:06:00]

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

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

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[eae, 2017-03-01 02:48:34]

The CQ bit was checked by eae@chromium.org

[eae, 2017-03-01 02:48:34]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/2721613003/#ps80001
(title: "Fix win compile")

[eae, 2017-03-01 02:48:37]

The CQ bit was unchecked by eae@chromium.org

[eae, 2017-03-01 04:00:15]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2017-03-01 04:00:30]

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

[eae, 2017-03-01 05:27:36]

The CQ bit was checked by eae@chromium.org

[eae, 2017-03-01 05:27:36]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/2721613003/#ps100001
(title: "Try to make win clang happy")

[eae, 2017-03-01 05:27:52]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-03-01 05:28:01]

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

[eae, 2017-03-01 06:17:52]

Hmm, the windows clang bot does not like my ng_box_strut file even though it has
the exact same format as the other ones. Am I missing something here?

[eae, 2017-03-01 06:18:39]

The CQ bit was unchecked by eae@chromium.org

[eae, 2017-03-01 06:24:36]

The CQ bit was checked by eae@chromium.org

[eae, 2017-03-01 06:24:36]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/2721613003/#ps120001
(title: "Don't export NGBoxStrut for now")

[commit-bot: I haz the power, 2017-03-01 06:24:50]

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-03-01 07:55:37]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488349476057480,
"parent_rev": "89127b453ad1909ebad1dbcc729250a380dc097d", "commit_rev":
"480fa30589acd71427e3f902fd7916e87c41f5d9"}

[commit-bot: I haz the power, 2017-03-01 07:56:13]

Description was changed from

==========
[LayoutNG] Move remaining ng_units structs to their own files

R=mstensho@opera.com
==========

to

==========
[LayoutNG] Move remaining ng_units structs to their own files

R=mstensho@opera.com

Review-Url: https://codereview.chromium.org/2721613003
Cr-Commit-Position: refs/heads/master@{#453878}
Committed:
https://chromium.googlesource.com/chromium/src/+/480fa30589acd71427e3f902fd79...
==========

[commit-bot: I haz the power, 2017-03-01 07:56:17]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/480fa30589acd71427e3f902fd79...

[Adam Rice, 2017-03-01 08:15:26]

A revert of this CL (patchset #6 id:120001) has been created in
https://codereview.chromium.org/2723023003/ by ricea@chromium.org.

The reason for reverting is: Causing link error on Android due to duplicate
symbols:
https://build.chromium.org/p/chromium.webkit/builders/Android%20Builder/build....

[eae, 2017-03-01 09:55:25]

Thanks Adam!

[eae, 2017-03-01 09:56:17]

https://codereview.chromium.org/2721613003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/BUILD.gn (right):

https://codereview.chromium.org/2721613003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/BUILD.gn:1303:
"layout/ng/geometry/ng_box_strut.cc",
Ah, this should be ng_box_strut_test.cc. My bad.