[LayoutNG] Remove derived constraint spaces from opportunity iterator.

[LayoutNG] Remove derived constraint spaces from opportunity iterator.

Creating derived constraint spaces from the layout opportunity iterator wasn't being used for the float case, and isn't needed for the text case either. Removing!

BUG=635619
Committed: https://crrev.com/64a8a9fadd701d4049974d569fbea4b99ac49ea0
Cr-Commit-Position: refs/heads/master@{#427201}

[ikilpatrick, 2016-10-21 20:24:10]

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

[ikilpatrick, 2016-10-21 20:24:11]

I'm a bit rusty on struct + const etc, please point out if i've done anything
silly :)

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

https://codereview.chromium.org/2438313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc:225:
const NGLogicalRect* NGLayoutOpportunityIterator::Next() {
?

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

https://codereview.chromium.org/2438313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_tree_node.h:44:
NGLogicalRect layout_area;
I renamed this area, as if it has an exclusion it really isn't a layout
opportunity? More than happy to rename back :)

[ikilpatrick, 2016-10-21 20:25:26]

Description was changed from

==========
[LayoutNG] Remove derived constraint spaces from opportunity iterator.

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

to

==========
[LayoutNG] Remove derived constraint spaces from opportunity iterator.

Creating derived constraint spaces from the layout opportunity iterator wasn't
being used for the float case, and isn't needed for the text case either.
Removing!

BUG=635619
==========

[Gleb Lanbin, 2016-10-21 21:06:59]

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

https://codereview.chromium.org/2438313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc:32:
bool IsExclusionWithinSpace(const NGLogicalRect& layout_area,
let's stick with "layout opportunity" name? i.e.
bool IsExclusionWithinLayoutOpportunity(const NGLayoutOpportunity& opportunity,
..

https://codereview.chromium.org/2438313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc:52:
const NGLogicalRect& parent_layout_area = parent_node->layout_area;
const NGLayoutOpportunity& parent_opportunity

https://codereview.chromium.org/2438313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc:225:
const NGLogicalRect* NGLayoutOpportunityIterator::Next() {
On 2016/10/21 20:24:10, ikilpatrick wrote:
> ?

yes, this iterator needs to be fixed. you can just return a copy of 
NGLogicalRect for now.

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

https://codereview.chromium.org/2438313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.h:18:
typedef const NGConstraintSpace NGLayoutOpportunity;
change
typedef const NGConstraintSpace NGLayoutOpportunity;
to 
typedef NGLogicalRect NGLayoutOpportunity;

and then use it below like
const NGLayoutOpportunity* Next()

https://codereview.chromium.org/2438313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.h:19:
typedef Vector<NGLogicalRect> NGLayoutOpportunities;
typedef Vector<const NGLayoutOpportunity> NGLayoutOpportunities

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

https://codereview.chromium.org/2438313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_tree_node.h:44:
NGLogicalRect layout_area;
On 2016/10/21 20:24:11, ikilpatrick wrote:
> I renamed this area, as if it has an exclusion it really isn't a layout
> opportunity? More than happy to rename back :)

how about this

struct NGLayoutOpportunityTreeNode(const NGLayoutOpportunity* opportunity) :
layout_area(opportunity) {}

NGLayoutOpportunity* Opportunity() {
  if (IsLeafNode()) return layout_area_.get();
  return nullptr;
}

private:
  std::unique_ptr<NGLayoutOpportunity> layout_area_;

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

https://codereview.chromium.org/2438313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_units.cc:27: return *this ==
NGLogicalRect();
.nit We will need to change that later as it allocates an empty object every
time we need to check NGLogicalRect for emptiness. I think we can either use
static constexpr NGLogicalRect kEmptyNGLogicalRect = NGLogicalRect(); or
std::is_empty if it's supported. Feel free to leave it as is now or add a TODO.

[ikilpatrick, 2016-10-21 21:45:05]

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

[ikilpatrick, 2016-10-21 21:45:27]

PTAL

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

https://codereview.chromium.org/2438313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc:32:
bool IsExclusionWithinSpace(const NGLogicalRect& layout_area,
On 2016/10/21 21:06:58, glebl wrote:
> let's stick with "layout opportunity" name? i.e.
> bool IsExclusionWithinLayoutOpportunity(const NGLayoutOpportunity&
opportunity,
> ..

Done.

https://codereview.chromium.org/2438313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc:52:
const NGLogicalRect& parent_layout_area = parent_node->layout_area;
On 2016/10/21 21:06:58, glebl wrote:
> const NGLayoutOpportunity& parent_opportunity 

Done.

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

https://codereview.chromium.org/2438313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.h:18:
typedef const NGConstraintSpace NGLayoutOpportunity;
On 2016/10/21 21:06:58, glebl wrote:
> change
> typedef const NGConstraintSpace NGLayoutOpportunity;
> to 
> typedef NGLogicalRect NGLayoutOpportunity;
> 
> and then use it below like
> const NGLayoutOpportunity* Next()

Done.

https://codereview.chromium.org/2438313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.h:19:
typedef Vector<NGLogicalRect> NGLayoutOpportunities;
On 2016/10/21 21:06:58, glebl wrote:
> typedef Vector<const NGLayoutOpportunity> NGLayoutOpportunities

Done.

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

https://codereview.chromium.org/2438313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_tree_node.h:44:
NGLogicalRect layout_area;
On 2016/10/21 21:06:59, glebl wrote:
> On 2016/10/21 20:24:11, ikilpatrick wrote:
> > I renamed this area, as if it has an exclusion it really isn't a layout
> > opportunity? More than happy to rename back :)
> 
> how about this
> 
> struct NGLayoutOpportunityTreeNode(const NGLayoutOpportunity* opportunity) :
> layout_area(opportunity) {}
> 
> NGLayoutOpportunity* Opportunity() {
>   if (IsLeafNode()) return layout_area_.get();
>   return nullptr;
> }
> 
> private:
>   std::unique_ptr<NGLayoutOpportunity> layout_area_;

Renamed back to opportunity for now.

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

https://codereview.chromium.org/2438313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_units.cc:27: return *this ==
NGLogicalRect();
On 2016/10/21 21:06:59, glebl wrote:
> .nit We will need to change that later as it allocates an empty object every
> time we need to check NGLogicalRect for emptiness. I think we can either use
> static constexpr NGLogicalRect kEmptyNGLogicalRect = NGLogicalRect(); or
> std::is_empty if it's supported. Feel free to leave it as is now or add a
TODO.

added TODO

[commit-bot: I haz the power, 2016-10-21 21:45:41]

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

[Gleb Lanbin, 2016-10-21 22:13:01]

lgtm

https://codereview.chromium.org/2438313003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc
(right):

https://codereview.chromium.org/2438313003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc:48: return
!opportunity.IsEmpty() ? opportunity.ToString() : String("(empty)");
.nit please try to use positive booleans statement.

opportunity.IsEmpty() ? String("(empty)") : opportunity.ToString();

[commit-bot: I haz the power, 2016-10-22 01:56:04]

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

[commit-bot: I haz the power, 2016-10-22 01:56:04]

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

[ikilpatrick, 2016-10-23 18:58:38]

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

[commit-bot: I haz the power, 2016-10-23 18:58:45]

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, 2016-10-23 20:28:48]

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

[commit-bot: I haz the power, 2016-10-23 20:28:49]

Dry run: This issue passed the CQ dry run.

[ikilpatrick, 2016-10-24 16:09:20]

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

[commit-bot: I haz the power, 2016-10-24 16:09:35]

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

[Gleb Lanbin, 2016-10-24 16:45:44]

https://codereview.chromium.org/2438313003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2438313003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:31: const
NGLogicalRect& opportunity,
use NGLayoutOpportunity and below.

[ikilpatrick, 2016-10-24 17:09:19]

thanks :)

https://codereview.chromium.org/2438313003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc
(right):

https://codereview.chromium.org/2438313003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc:48: return
!opportunity.IsEmpty() ? opportunity.ToString() : String("(empty)");
On 2016/10/21 22:13:01, glebl wrote:
> .nit please try to use positive booleans statement.
> 
> opportunity.IsEmpty() ? String("(empty)") : opportunity.ToString();

Done.

https://codereview.chromium.org/2438313003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2438313003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:31: const
NGLogicalRect& opportunity,
On 2016/10/24 16:45:44, glebl wrote:
> use NGLayoutOpportunity and below.

Done.

[ikilpatrick, 2016-10-24 17:09:29]

The CQ bit was checked by ikilpatrick@chromium.org

[ikilpatrick, 2016-10-24 17:09:29]

The patchset sent to the CQ was uploaded after l-g-t-m from glebl@chromium.org
Link to the patchset: https://codereview.chromium.org/2438313003/#ps120001
(title: "address comments.")

[commit-bot: I haz the power, 2016-10-24 17:09:56]

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

[cbiesinger, 2016-10-24 17:21:54]

Hmm, don't we need the derived constraint space as the space to pass to the
child's layout function, so that the width is correct? We don't do that right
now but we should, no?

[ikilpatrick, 2016-10-24 18:09:13]

On 2016/10/24 17:21:54, cbiesinger wrote:
> Hmm, don't we need the derived constraint space as the space to pass to the
> child's layout function, so that the width is correct? We don't do that right
> now but we should, no?

Yeah the realization here is that returning a constraint space from the
opportunity iterator was overload at the moment.

For the text case, we want to just pass the available rect to the text fragment
generator (tm) and then make the decision to use that fragment or not.

For the float case at the moment we are just using it for positioning, so the
constraint space wasn't needed.

I'm going to send out a patch for the NGConstraintSpaceBuilder soon, then
migrate everything to that instead all of the constructors.

[ikilpatrick, 2016-10-24 18:55:17]

The CQ bit was unchecked by ikilpatrick@chromium.org

[ikilpatrick, 2016-10-24 18:55:37]

The CQ bit was checked by ikilpatrick@chromium.org

[commit-bot: I haz the power, 2016-10-24 18:56:17]

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

[Gleb Lanbin, 2016-10-24 23:16:15]

The CQ bit was unchecked by glebl@chromium.org

[Gleb Lanbin, 2016-10-24 23:16:20]

The CQ bit was checked by glebl@chromium.org

[commit-bot: I haz the power, 2016-10-24 23:17:22]

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, 2016-10-25 01:04:20]

Description was changed from

==========
[LayoutNG] Remove derived constraint spaces from opportunity iterator.

Creating derived constraint spaces from the layout opportunity iterator wasn't
being used for the float case, and isn't needed for the text case either.
Removing!

BUG=635619
==========

to

==========
[LayoutNG] Remove derived constraint spaces from opportunity iterator.

Creating derived constraint spaces from the layout opportunity iterator wasn't
being used for the float case, and isn't needed for the text case either.
Removing!

BUG=635619
==========

[commit-bot: I haz the power, 2016-10-25 01:04:21]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-25 01:06:08]

Description was changed from

==========
[LayoutNG] Remove derived constraint spaces from opportunity iterator.

Creating derived constraint spaces from the layout opportunity iterator wasn't
being used for the float case, and isn't needed for the text case either.
Removing!

BUG=635619
==========

to

==========
[LayoutNG] Remove derived constraint spaces from opportunity iterator.

Creating derived constraint spaces from the layout opportunity iterator wasn't
being used for the float case, and isn't needed for the text case either.
Removing!

BUG=635619

Committed: https://crrev.com/64a8a9fadd701d4049974d569fbea4b99ac49ea0
Cr-Commit-Position: refs/heads/master@{#427201}
==========

[commit-bot: I haz the power, 2016-10-25 01:06:10]

Patchset 7 (id:??) landed as
https://crrev.com/64a8a9fadd701d4049974d569fbea4b99ac49ea0
Cr-Commit-Position: refs/heads/master@{#427201}