Refactor NGInlineNode::CollectInlines (both overloads)

Refactor NGInlineNode::CollectInlines (both overloads)

This patch does the following refactoring:

1. Remove the parameter NGInlineNode::CollectInlines(LayoutBlockFlow*) because
it is always equal to GetLayoutBlockFlow().

2. Change NGInlineNode::CollectInlines(LayoutBlockFlow*, NGInlineItemsBuilder*)
to a local function of the .cc file, because it does not access any private
member of NGInlineNode.

3. Move call sites of NGInlineItemsBuilder::EnterBlock and ExitBlock into the
above local function, so that it is easier to reuse this function to build
offset mapping.

Step 3 is a preparation for crrev.com/2963093002

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

Review-Url: https://codereview.chromium.org/2965573003
Cr-Commit-Position: refs/heads/master@{#483585}
Committed: https://chromium.googlesource.com/chromium/src/+/8cbc86dacc8e15bddeb0bae234ebb00fcc7550af

[Xiaocheng, 2017-06-29 22:27:50]

Description was changed from

==========
Refactor NGInlineNode::CollectInlines (both overloads)

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

to

==========
Refactor NGInlineNode::CollectInlines (both overloads)

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

[Xiaocheng, 2017-06-29 22:27:54]

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

[commit-bot: I haz the power, 2017-06-29 22:28:08]

Dry run: CQ is trying da patch.

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

[Xiaocheng, 2017-06-29 22:31:39]

Description was changed from

==========
Refactor NGInlineNode::CollectInlines (both overloads)

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

to

==========
Refactor NGInlineNode::CollectInlines (both overloads)

This patch does the following refactoring:

1. Remove the parameter NGInlineNode::CollectInlines(LayoutBlockFlow*) because
it is always equal to GetLayoutBlockFlow().

2. Change NGInlineNode::CollectInlines(LayoutBlockFlow*, NGInlineItemsBuilder*)
to a local function of the .cc file, because it does not access any private
member of NGInlineNode.

3. Move call sites of NGInlineItemsBuilder::EnterBlock and ExitBlock into the
above local function, so that it is easier to reuse this function to build
offset mapping.

Step 3 is a preparation for crrev.com/2963093002

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

[Xiaocheng, 2017-06-29 22:43:11]

xiaochengh@chromium.org changed reviewers:
+ eae@chromium.org

[Xiaocheng, 2017-06-29 22:43:12]

PTAL.

I'd like to make this change so that we can reuse CollectInlinesInternal for
building offset mapping.

[eae, 2017-06-29 22:54:00]

https://codereview.chromium.org/2965573003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node.cc (right):

https://codereview.chromium.org/2965573003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node.cc:195: // to the
anonymous namespace.
Please do this before landing.

https://codereview.chromium.org/2965573003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node.cc:216: static
std::pair<String, LayoutBox*> CollectInlinesInternal(
The string can easily be access from the builder which is passed in, why also
return it as a part of a pair?

Would also simplify the calling code:
MutableData().next_sibling_ = CollectInlines(block, &builder);
MutableData().text_content_ = builder.ToString();

is arguably easier to reason about and debug then:
std::tie(MutableData().text_content_, MutableData().next_sibling_) =
    CollectInlinesInternal(block, &builder);

[Xiaocheng, 2017-06-29 23:13:35]

Patchset #2 (id:20001) has been deleted

[Xiaocheng, 2017-06-29 23:16:19]

Thanks for reviewing!

https://codereview.chromium.org/2965573003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node.cc (right):

https://codereview.chromium.org/2965573003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node.cc:195: // to the
anonymous namespace.
On 2017/06/29 at 22:54:00, eae wrote:
> Please do this before landing.

The code review tool and git log are not quite friendly to code moving.

I'd like to keep the function in place so that the lines changed by this patch
are clear.

https://codereview.chromium.org/2965573003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node.cc:216: static
std::pair<String, LayoutBox*> CollectInlinesInternal(
On 2017/06/29 at 22:54:00, eae wrote:
> The string can easily be access from the builder which is passed in, why also
return it as a part of a pair?
> 
> Would also simplify the calling code:
> MutableData().next_sibling_ = CollectInlines(block, &builder);
> MutableData().text_content_ = builder.ToString();
> 
> is arguably easier to reason about and debug then:
> std::tie(MutableData().text_content_, MutableData().next_sibling_) =
>     CollectInlinesInternal(block, &builder);

Done.

[eae, 2017-06-29 23:20:12]

On 2017/06/29 23:16:19, Xiaocheng wrote:
> Thanks for reviewing!
> 
>
https://codereview.chromium.org/2965573003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node.cc
(right):
> 
>
https://codereview.chromium.org/2965573003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node.cc:195: // to
the
> anonymous namespace.
> On 2017/06/29 at 22:54:00, eae wrote:
> > Please do this before landing.
> 
> The code review tool and git log are not quite friendly to code moving.
> 
> I'd like to keep the function in place so that the lines changed by this patch
> are clear.

Makes sense, in that case please do a follow up patch to move it. Thanks!

[eae, 2017-06-29 23:20:53]

LGTM

[Xiaocheng, 2017-06-29 23:33:05]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-06-29 23:33:06]

Thanks for reviewing!

https://codereview.chromium.org/2965573003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node.cc (right):

https://codereview.chromium.org/2965573003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node.cc:195: // to the
anonymous namespace.
On 2017/06/29 at 23:16:19, Xiaocheng wrote:
> On 2017/06/29 at 22:54:00, eae wrote:
> > Please do this before landing.
> 
> The code review tool and git log are not quite friendly to code moving.
> 
> I'd like to keep the function in place so that the lines changed by this patch
are clear.

Will do.

[commit-bot: I haz the power, 2017-06-29 23:33:26]

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-30 01:46:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498779185869150,
"parent_rev": "ffcd346677e9dd28037da9619c510274d9318bfe", "commit_rev":
"8cbc86dacc8e15bddeb0bae234ebb00fcc7550af"}

[commit-bot: I haz the power, 2017-06-30 01:46:49]

Description was changed from

==========
Refactor NGInlineNode::CollectInlines (both overloads)

This patch does the following refactoring:

1. Remove the parameter NGInlineNode::CollectInlines(LayoutBlockFlow*) because
it is always equal to GetLayoutBlockFlow().

2. Change NGInlineNode::CollectInlines(LayoutBlockFlow*, NGInlineItemsBuilder*)
to a local function of the .cc file, because it does not access any private
member of NGInlineNode.

3. Move call sites of NGInlineItemsBuilder::EnterBlock and ExitBlock into the
above local function, so that it is easier to reuse this function to build
offset mapping.

Step 3 is a preparation for crrev.com/2963093002

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

to

==========
Refactor NGInlineNode::CollectInlines (both overloads)

This patch does the following refactoring:

1. Remove the parameter NGInlineNode::CollectInlines(LayoutBlockFlow*) because
it is always equal to GetLayoutBlockFlow().

2. Change NGInlineNode::CollectInlines(LayoutBlockFlow*, NGInlineItemsBuilder*)
to a local function of the .cc file, because it does not access any private
member of NGInlineNode.

3. Move call sites of NGInlineItemsBuilder::EnterBlock and ExitBlock into the
above local function, so that it is easier to reuse this function to build
offset mapping.

Step 3 is a preparation for crrev.com/2963093002

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

Review-Url: https://codereview.chromium.org/2965573003
Cr-Commit-Position: refs/heads/master@{#483585}
Committed:
https://chromium.googlesource.com/chromium/src/+/8cbc86dacc8e15bddeb0bae234eb...
==========

[commit-bot: I haz the power, 2017-06-30 01:46:51]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8cbc86dacc8e15bddeb0bae234eb...