Make NGFragmentBuilder to keep track on text children.

Make NGFragmentBuilder to keep track on text children.

This patch adds methods to access text children and their offsets from
NGFragmentBuilder, so they can be accessed from NGLineBuilder.

Also we want NGLineBuilder to have the class variable container_builder_
so we can add positioned floats directly to the fragment builder
bypassing any additional lists in between.

BUG=635619
Review-Url: https://codereview.chromium.org/2753743003
Cr-Commit-Position: refs/heads/master@{#457633}
Committed: https://chromium.googlesource.com/chromium/src/+/06f0e62cf0feb57ba69bb6f31d09e17d8a2cc073

[Gleb Lanbin, 2017-03-15 22:26:34]

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

[kojii, 2017-03-16 06:45:48]

Thanks for coming up this.

I'm happy to make children in FragmentBuilder mutable, that helps LineBuilder
not to keep a separate vector as this CL demonstrates. Don't know why we didn't
allow this at first place, so Ian might be able to say if this is good?

Assuming it's ok, why do we want to have a separate Vector only for
NGPhysicalTextFragment? Can't we put them into |children_| and check/cast type
as needed? I wish us to have less different code paths between box and text
where possible.

For |container_builder_|, we're currently a bit back and forth on this. We
originally had this, but then found that abspos doesn't work, so removed this
and moved to use parent's builder (NGLineBuilder::containing_block_builder_).
But this prevents unifying two different Layout() function signatures for block
and inline, so we're trying to eliminate it, but abspos problem needs to be
fixed first. See https://codereview.chromium.org/2750213002

Stabilizing how to handle the anonymous block for inline is likely to take a bit
longer, and we may eventually want to remove them. I'm good to have it in
NGLineBuilder until we have more solid picture, especially if it can help floats
to move forward.

Ian, WDYT?

[ikilpatrick, 2017-03-16 16:28:00]

On 2017/03/16 06:45:48, kojii wrote:
> Thanks for coming up this.
> 
> I'm happy to make children in FragmentBuilder mutable, that helps LineBuilder
> not to keep a separate vector as this CL demonstrates. Don't know why we
didn't
> allow this at first place, so Ian might be able to say if this is good?
> 
> Assuming it's ok, why do we want to have a separate Vector only for
> NGPhysicalTextFragment? Can't we put them into |children_| and check/cast type
> as needed? I wish us to have less different code paths between box and text
> where possible.
> 
> For |container_builder_|, we're currently a bit back and forth on this. We
> originally had this, but then found that abspos doesn't work, so removed this
> and moved to use parent's builder (NGLineBuilder::containing_block_builder_).
> But this prevents unifying two different Layout() function signatures for
block
> and inline, so we're trying to eliminate it, but abspos problem needs to be
> fixed first. See https://codereview.chromium.org/2750213002
> 
> Stabilizing how to handle the anonymous block for inline is likely to take a
bit
> longer, and we may eventually want to remove them. I'm good to have it in
> NGLineBuilder until we have more solid picture, especially if it can help
floats
> to move forward.
> 
> Ian, WDYT?

Yeah this lgtm. We are going to need a anon-fragment for either all of the
inlines up to a fragmentation point, or for each line. The abs-pos we need to
fix anyway the current solution has side-effects.

NGLineBuilder should really be NGInlineLayoutAlgorithm :).

[Gleb Lanbin, 2017-03-16 16:29:03]

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

[commit-bot: I haz the power, 2017-03-16 16:29:44]

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

[kojii, 2017-03-16 17:02:48]

On 2017/03/16 at 16:28:00, ikilpatrick wrote:
> NGLineBuilder should really be NGInlineLayoutAlgorithm :).

Yup, it's in my queue. Sorry not as fast as I would like me to be ;)

[commit-bot: I haz the power, 2017-03-16 18:07:52]

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

[commit-bot: I haz the power, 2017-03-16 18:07:53]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ikilpatrick, 2017-03-16 21:14:17]

ok, lgtm

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

https://codereview.chromium.org/2753743003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:413:
box_fragment->Children()[fragment_index];
this gan just be a raw ptr? no transfer on ownership?

[Gleb Lanbin, 2017-03-16 21:20:19]

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

https://codereview.chromium.org/2753743003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_line_builder.cc:413:
box_fragment->Children()[fragment_index];
On 2017/03/16 21:14:16, ikilpatrick wrote:
> this gan just be a raw ptr? no transfer on ownership?

Done.

[Gleb Lanbin, 2017-03-16 21:20:25]

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

[commit-bot: I haz the power, 2017-03-16 21:21:17]

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-03-17 00:16:38]

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

[commit-bot: I haz the power, 2017-03-17 00:16:39]

Dry run: This issue passed the CQ dry run.

[Gleb Lanbin, 2017-03-17 00:26:34]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2017-03-17 00:26:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
ikilpatrick@chromium.org
Link to the patchset: https://codereview.chromium.org/2753743003/#ps40001
(title: "get a raw ptr of NGPhysicalFragment instead of RefPtr")

[commit-bot: I haz the power, 2017-03-17 00:27:09]

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-17 00:49:27]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489710394188530,
"parent_rev": "8f0264a9eb0620b67b746a44b5856eea61245f77", "commit_rev":
"06f0e62cf0feb57ba69bb6f31d09e17d8a2cc073"}

[commit-bot: I haz the power, 2017-03-17 00:50:07]

Description was changed from

==========
Make NGFragmentBuilder to keep track on text children.

This patch adds methods to access text children and their offsets from
NGFragmentBuilder, so they can be accessed from NGLineBuilder.

Also we want NGLineBuilder to have the class variable container_builder_
so we can add positioned floats directly to the fragment builder
bypassing any additional lists in between.

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

to

==========
Make NGFragmentBuilder to keep track on text children.

This patch adds methods to access text children and their offsets from
NGFragmentBuilder, so they can be accessed from NGLineBuilder.

Also we want NGLineBuilder to have the class variable container_builder_
so we can add positioned floats directly to the fragment builder
bypassing any additional lists in between.

BUG=635619

Review-Url: https://codereview.chromium.org/2753743003
Cr-Commit-Position: refs/heads/master@{#457633}
Committed:
https://chromium.googlesource.com/chromium/src/+/06f0e62cf0feb57ba69bb6f31d09...
==========

[commit-bot: I haz the power, 2017-03-17 00:50:09]

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