[LayoutNG] Remove one NGConstraintSpace constructor, mark others for removal. (issue 2442123002 by ikilpatrick@chromium.org)

ikilp...@chromium.org

unread,

Oct 21, 2016, 6:52:12 PM10/21/16

to gl...@chromium.org, e...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

Reviewers: glebl, eae
CL: https://codereview.chromium.org/2442123002/

Description:
[LayoutNG] Remove one NGConstraintSpace constructor, mark others for removal.

Once the builder is added only the:
NGConstraintSpace(NGWritingMode, NGDirection, NGPhysicalConstraintSpace*)

should remain, this mirrors the NGFragment constructor which is just a "view"
on top of the NGPhysicalFragment.

This mirrors:
NGFragment(NGWritingMode, NGDirection, NGPhysicalFragment*)

BUG=635619

Affected files (+16, -25 lines):
M third_party/WebKit/Source/core/layout/ng/ng_box.cc
M third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h
M third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc
M third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc

Index: third_party/WebKit/Source/core/layout/ng/ng_box.cc
diff --git a/third_party/WebKit/Source/core/layout/ng/ng_box.cc b/third_party/WebKit/Source/core/layout/ng/ng_box.cc
index cf75f73f7fc365a98bbef36a123ce01eb34e7db1..5da0059c9d2e9820ef9e7ce3ca09ff1e6c32614a 100644
--- a/third_party/WebKit/Source/core/layout/ng/ng_box.cc
+++ b/third_party/WebKit/Source/core/layout/ng/ng_box.cc
@@ -38,7 +38,8 @@ bool NGBox::Layout(const NGConstraintSpace* constraint_space,
// Change the coordinate system of the constraint space.
NGConstraintSpace* child_constraint_space = new NGConstraintSpace(
FromPlatformWritingMode(Style()->getWritingMode()),
- FromPlatformDirection(Style()->direction()), constraint_space);
+ FromPlatformDirection(Style()->direction()),
+ constraint_space->PhysicalSpace());

NGPhysicalFragment* fragment = nullptr;
if (!algorithm_->Layout(child_constraint_space, &fragment))
Index: third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc
diff --git a/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc b/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc
index 460f4372fed583967dd8f3eb6585528e11a0ec8f..c2ad0bcc7ea41aa261531ade2f4f0ef7113cf755 100644
--- a/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc
+++ b/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc
@@ -13,15 +13,6 @@ namespace blink {

NGConstraintSpace::NGConstraintSpace(NGWritingMode writing_mode,
NGDirection direction,
- NGLogicalSize container_size)
- : physical_space_(new NGPhysicalConstraintSpace(
- container_size.ConvertToPhysical(writing_mode))),
- size_(container_size),
- writing_mode_(writing_mode),
- direction_(direction) {}
-
-NGConstraintSpace::NGConstraintSpace(NGWritingMode writing_mode,
- NGDirection direction,
NGPhysicalConstraintSpace* physical_space)
: physical_space_(physical_space),
size_(physical_space->ContainerSize().ConvertToLogical(writing_mode)),
@@ -30,10 +21,10 @@ NGConstraintSpace::NGConstraintSpace(NGWritingMode writing_mode,

NGConstraintSpace::NGConstraintSpace(NGWritingMode writing_mode,
NGDirection direction,
- const NGConstraintSpace* constraint_space)
- : physical_space_(constraint_space->PhysicalSpace()),
- offset_(constraint_space->Offset()),
- size_(constraint_space->Size()),
+ NGLogicalSize container_size)
+ : physical_space_(new NGPhysicalConstraintSpace(
+ container_size.ConvertToPhysical(writing_mode))),
+ size_(container_size),
writing_mode_(writing_mode),
direction_(direction) {}

Index: third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h
diff --git a/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h b/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h
index 131dbb918730e85ec9d65421cd3e883720c14304..80ec43b00b7cd1f238cda76e6fa7ad720e96fae5 100644
--- a/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h
+++ b/third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h
@@ -25,25 +25,22 @@ class NGLayoutOpportunityIterator;
class CORE_EXPORT NGConstraintSpace final
: public GarbageCollected<NGConstraintSpace> {
public:
- // Constructs a constraint space with a new backing NGPhysicalConstraintSpace.
- // The size will be used for both for the physical constraint space's
- // container size and this constraint space's Size().
- NGConstraintSpace(NGWritingMode, NGDirection, NGLogicalSize);
-
// Constructs a constraint space based on an existing backing
// NGPhysicalConstraintSpace. Sets this constraint space's size to the
// physical constraint space's container size, converted to logical
// coordinates.
- // TODO(layout-ng): Do we need this constructor?
NGConstraintSpace(NGWritingMode, NGDirection, NGPhysicalConstraintSpace*);

- // Constructs a constraint space with a different NGWritingMode and
- // NGDirection that's otherwise identical.
- NGConstraintSpace(NGWritingMode, NGDirection, const NGConstraintSpace*);
+ // Constructs a constraint space with a new backing NGPhysicalConstraintSpace.
+ // The size will be used for both for the physical constraint space's
+ // container size and this constraint space's Size().
+ // TODO(layout-dev): Remove once NGConstraintSpaceBuilder exists.
+ NGConstraintSpace(NGWritingMode, NGDirection, NGLogicalSize);

// Constructs a derived constraint space sharing the same backing
// NGPhysicalConstraintSpace, NGWritingMode and NGDirection. Primarily for use
// by NGLayoutOpportunityIterator.
+ // TODO(layout-dev): Remove once NGConstraintSpaceBuilder exists.
NGConstraintSpace(const NGConstraintSpace& other,
NGLogicalOffset,
NGLogicalSize);
@@ -52,6 +49,7 @@ class CORE_EXPORT NGConstraintSpace final
// input constraint space, but has a different container size, writing mode
// and direction. Sets the offset to zero. For use by layout algorithms
// to use as the basis to find layout opportunities for children.
+ // TODO(layout-dev): Remove once NGConstraintSpaceBuilder exists.
NGConstraintSpace(NGWritingMode,
NGDirection,
const NGConstraintSpace& other,
@@ -128,6 +126,7 @@ class CORE_EXPORT NGConstraintSpace final
String ToString() const;

private:
+ // TODO(layout-dev): Make const once NGConstraintSpaceBuilder exists.
Member<NGPhysicalConstraintSpace> physical_space_;
NGLogicalOffset offset_;
NGLogicalSize size_;
Index: third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc
diff --git a/third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc b/third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc
index 04022dd323400ab783606dab40a89b20c93f60fc..ed69ceceb430dcacc6ed8fc49ca3328a57f87fcc 100644
--- a/third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc
+++ b/third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc
@@ -19,8 +19,8 @@ TEST(NGConstraintSpaceTest, WritingMode) {
horz_space->SetFixedSize(true, false);
horz_space->SetFragmentationType(FragmentColumn);

- NGConstraintSpace* vert_space =
- new NGConstraintSpace(VerticalRightLeft, LeftToRight, horz_space);
+ NGConstraintSpace* vert_space = new NGConstraintSpace(
+ VerticalRightLeft, LeftToRight, horz_space->PhysicalSpace());

EXPECT_EQ(LayoutUnit(200), horz_space->ContainerSize().inline_size);
EXPECT_EQ(LayoutUnit(200), vert_space->ContainerSize().block_size);

gl...@chromium.org

unread,

Oct 21, 2016, 7:21:22 PM10/21/16

to ikilp...@chromium.org, e...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

lgtm

https://codereview.chromium.org/2442123002/

e...@chromium.org

unread,

Oct 21, 2016, 8:17:02 PM10/21/16

to ikilp...@chromium.org, gl...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

I like it, LGTM

https://codereview.chromium.org/2442123002/

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,

Oct 21, 2016, 8:17:52 PM10/21/16

to ikilp...@chromium.org, gl...@chromium.org, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2442123002/1

https://codereview.chromium.org/2442123002/

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,

Oct 21, 2016, 10:58:55 PM10/21/16

to ikilp...@chromium.org, gl...@chromium.org, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

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_rel_ng/builds/321858)

https://codereview.chromium.org/2442123002/

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,

Oct 22, 2016, 3:04:39 AM10/22/16

to ikilp...@chromium.org, gl...@chromium.org, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,

Oct 22, 2016, 3:24:09 AM10/22/16

to ikilp...@chromium.org, gl...@chromium.org, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

Try jobs failed on following builders:

chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/287439)

https://codereview.chromium.org/2442123002/

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,

Oct 22, 2016, 12:54:58 PM10/22/16

to ikilp...@chromium.org, gl...@chromium.org, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2442123002/40001

https://codereview.chromium.org/2442123002/

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,

Oct 23, 2016, 9:39:45 AM10/23/16

to ikilp...@chromium.org, gl...@chromium.org, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

Committed patchset #3 (id:40001)

https://codereview.chromium.org/2442123002/

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,

Oct 23, 2016, 9:42:03 AM10/23/16

to ikilp...@chromium.org, gl...@chromium.org, e...@chromium.org, commi...@chromium.org, chromium...@chromium.org, cbies...@chromium.org, ojan+...@chromium.org, szager+la...@chromium.org, glebl+...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

Patchset 3 (id:??) landed as
https://crrev.com/ce86bdc2a63e021733d86af975d0ce47e38ec935
Cr-Commit-Position: refs/heads/master@{#426995}

https://codereview.chromium.org/2442123002/

Reply all

Reply to author

Forward