Templatize NGInlineItemsBuilder to take a OffsetMappingBuilder parameter

Templatize NGInlineItemsBuilder to take a OffsetMappingBuilder parameter

This patch templatizes NGInlineItemsBuilder with an additional parameter
OffsetMappingBuilder, but does not do anything with the new parameter, and
hence, does not introduce any new behavior yet.

A follow-up patch (*) will pass in a real offset mapping builder, and make
NGInlineItemsBuilder also construct the whitespace-collapsed offset mapping
with the passed-in builder.

Design doc of offset mapping: https://goo.gl/CJbxky

* crrev.com/2943573002

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

Review-Url: https://codereview.chromium.org/2960673004
Cr-Commit-Position: refs/heads/master@{#483145}
Committed: https://chromium.googlesource.com/chromium/src/+/1bc92de1145d6d9afe97605bd39808b547bd1ae8

[Xiaocheng, 2017-06-28 02:49:56]

Description was changed from

==========
Templatize NGInlineItemsBuilder to take a OffsetMappingBuilder parameter

BUG=699017
==========

to

==========
Templatize NGInlineItemsBuilder to take a OffsetMappingBuilder parameter

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

[Xiaocheng, 2017-06-28 02:56:54]

Description was changed from

==========
Templatize NGInlineItemsBuilder to take a OffsetMappingBuilder parameter

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

to

==========
Templatize NGInlineItemsBuilder to take a OffsetMappingBuilder parameter

This patch templatizes NGInlineItemsBuilder with an additional parameter
OffsetMappingBuilder, but does not introduce any new behavior yet.

A follow-up patch will pass in a real offset mapping builder, and make
NGInlineItemsBuilder also construct the whitespace-collapsed offset mapping with
the passed-in builder.

Design doc of offset mapping: https://goo.gl/CJbxky

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

[Xiaocheng, 2017-06-28 02:57:40]

xiaochengh@chromium.org changed reviewers:
+ kojii@chromium.org, yoichio@chromium.org, yosin@chromium.org

[Xiaocheng, 2017-06-28 02:57:41]

PTAL.

This is a preparation patch for crrev.com/2943573002, since with the new design,
the nullptr check and branching causes ~2% performance regression.

[Xiaocheng, 2017-06-28 03:34:16]

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

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

Dry run: CQ is trying da patch.

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

[yosin_UTC9, 2017-06-28 03:37:11]

https://codereview.chromium.org/2960673004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.h
(right):

https://codereview.chromium.org/2960673004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.h:93:
OffsetMappingBuilder mapping_builder_;
Could you upload the patch which using |mapping_builder_|?
It seems this patch doesn't have use sites of |mapping_builder_|.

[Xiaocheng, 2017-06-28 03:44:42]

https://codereview.chromium.org/2960673004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.h
(right):

https://codereview.chromium.org/2960673004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.h:93:
OffsetMappingBuilder mapping_builder_;
On 2017/06/28 at 03:37:11, yosin_UTC9 wrote:
> Could you upload the patch which using |mapping_builder_|?
> It seems this patch doesn't have use sites of |mapping_builder_|.

This is just a preparation patch that templatizes NGInlineItemsBuilder, but does
not do anything with the template parameter.

Use sites of the mapping builder are introduced in
https://codereview.chromium.org/2943573002/.

Maybe I should even remove |mapping_builder_| in this patch, and put it in the
follow-up one?

[yosin_UTC9, 2017-06-28 04:06:27]

On 2017/06/28 at 03:44:42, xiaochengh wrote:
>
https://codereview.chromium.org/2960673004/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.h
(right):
> 
>
https://codereview.chromium.org/2960673004/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/ng/inline/ng_inline_items_builder.h:93:
OffsetMappingBuilder mapping_builder_;
> On 2017/06/28 at 03:37:11, yosin_UTC9 wrote:
> > Could you upload the patch which using |mapping_builder_|?
> > It seems this patch doesn't have use sites of |mapping_builder_|.
> 
> This is just a preparation patch that templatizes NGInlineItemsBuilder, but
does not do anything with the template parameter.
> 
> Use sites of the mapping builder are introduced in
https://codereview.chromium.org/2943573002/.
Thanks, I saw that.
Please include the link of use sites patch in description to demonstrate how
template parameter used.

> Maybe I should even remove |mapping_builder_| in this patch, and put it in the
follow-up one?
Yes, please. It is confusing because there are no use sites in this patch.

[commit-bot: I haz the power, 2017-06-28 04:18:01]

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

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

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

[Xiaocheng, 2017-06-28 05:29:11]

Description was changed from

==========
Templatize NGInlineItemsBuilder to take a OffsetMappingBuilder parameter

This patch templatizes NGInlineItemsBuilder with an additional parameter
OffsetMappingBuilder, but does not introduce any new behavior yet.

A follow-up patch will pass in a real offset mapping builder, and make
NGInlineItemsBuilder also construct the whitespace-collapsed offset mapping with
the passed-in builder.

Design doc of offset mapping: https://goo.gl/CJbxky

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

to

==========
Templatize NGInlineItemsBuilder to take a OffsetMappingBuilder parameter

This patch templatizes NGInlineItemsBuilder with an additional parameter
OffsetMappingBuilder, but does not do anything with the new parameter, and
hence, does not introduce any new behavior yet.

A follow-up patch (*) will pass in a real offset mapping builder, and make
NGInlineItemsBuilder also construct the whitespace-collapsed offset mapping
with the passed-in builder.

Design doc of offset mapping: https://goo.gl/CJbxky

* crrev.com/2943573002

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

[Xiaocheng, 2017-06-28 05:30:00]

Updated with |mapping_builder_| moved to the follow-up patch.

PTAL.

[Xiaocheng, 2017-06-28 05:30:05]

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

[commit-bot: I haz the power, 2017-06-28 05:30:14]

Dry run: CQ is trying da patch.

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

[kojii, 2017-06-28 06:15:53]

Ah...this doesn't call any functions of the template?

Can you include the real one, or empty one, and include calls to them?

Or you can write the code without templating, then template later. I'd like to
see how we call it. 2% perf regression is fine for now, we'll tackle perf in Q4.

[Xiaocheng, 2017-06-28 07:03:44]

On 2017/06/28 at 06:15:53, kojii wrote:
> Ah...this doesn't call any functions of the template?
> 
> Can you include the real one, or empty one, and include calls to them?
> 
> Or you can write the code without templating, then template later. I'd like to
see how we call it. 2% perf regression is fine for now, we'll tackle perf in Q4.

This patch is intentionally made to do nothing with the template parameter.

Use sites are in the follow-up patch: crrev.com/2943573002

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

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

[commit-bot: I haz the power, 2017-06-28 07:27:12]

Dry run: This issue passed the CQ dry run.

[kojii, 2017-06-28 07:45:29]

lgtm w/nit

https://codereview.chromium.org/2960673004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/BUILD.gn (right):

https://codereview.chromium.org/2960673004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/BUILD.gn:328:
"ng/api/empty_offset_mapping_builder.h",
Please put this into "inline" directory.

[Xiaocheng, 2017-06-28 18:11:47]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-06-28 18:11:48]

Thanks for reviewing!

Committing with EmptyOffsetMappingBuilder moved to ng/inline/

[Xiaocheng, 2017-06-28 18:11:49]

The patchset sent to the CQ was uploaded after l-g-t-m from kojii@chromium.org
Link to the patchset: https://codereview.chromium.org/2960673004/#ps100001
(title: "Move EmptyOffsetMappingBuilder to ng/inline/")

[commit-bot: I haz the power, 2017-06-28 18:12:17]

CQ is trying da patch.

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

[Xiaocheng, 2017-06-28 19:05:12]

The CQ bit was unchecked by xiaochengh@chromium.org

[Xiaocheng, 2017-06-28 19:47:09]

The CQ bit was checked by xiaochengh@chromium.org

[commit-bot: I haz the power, 2017-06-28 19:47:42]

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-28 21:25:41]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498679229745070,
"parent_rev": "83eda110a4bac97a1053c6735ed22c635994312f", "commit_rev":
"1bc92de1145d6d9afe97605bd39808b547bd1ae8"}

[commit-bot: I haz the power, 2017-06-28 21:25:55]

Description was changed from

==========
Templatize NGInlineItemsBuilder to take a OffsetMappingBuilder parameter

This patch templatizes NGInlineItemsBuilder with an additional parameter
OffsetMappingBuilder, but does not do anything with the new parameter, and
hence, does not introduce any new behavior yet.

A follow-up patch (*) will pass in a real offset mapping builder, and make
NGInlineItemsBuilder also construct the whitespace-collapsed offset mapping
with the passed-in builder.

Design doc of offset mapping: https://goo.gl/CJbxky

* crrev.com/2943573002

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

to

==========
Templatize NGInlineItemsBuilder to take a OffsetMappingBuilder parameter

This patch templatizes NGInlineItemsBuilder with an additional parameter
OffsetMappingBuilder, but does not do anything with the new parameter, and
hence, does not introduce any new behavior yet.

A follow-up patch (*) will pass in a real offset mapping builder, and make
NGInlineItemsBuilder also construct the whitespace-collapsed offset mapping
with the passed-in builder.

Design doc of offset mapping: https://goo.gl/CJbxky

* crrev.com/2943573002

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

Review-Url: https://codereview.chromium.org/2960673004
Cr-Commit-Position: refs/heads/master@{#483145}
Committed:
https://chromium.googlesource.com/chromium/src/+/1bc92de1145d6d9afe97605bd398...
==========

[commit-bot: I haz the power, 2017-06-28 21:25:57]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/1bc92de1145d6d9afe97605bd398...