Search the entire subtree when looking for the end of an inline continuation chain.

Search the entire subtree when looking for the end of an inline continuation chain.

Inlines may be nested, so we may not find the last inline in the chain as a
direct child of the anonymous blocks. We need to search the entire subtree.
Don't do this with anonymous blocks that wrap block children (the block-level
DOM children of the inline-level objects), though. We're not going to find
anything interesting there.

This fix is speculative; the original bug report didn't come with a test case.

BUG=662767
Review-Url: https://codereview.chromium.org/2738503004
Cr-Commit-Position: refs/heads/master@{#455420}
Committed: https://chromium.googlesource.com/chromium/src/+/e72c8c06b956706b54648589f807086d17340831

[mstensho (USE GERRIT), 2017-03-07 13:18:05]

The CQ bit was checked by mstensho@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 13:18:22]

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

[mstensho (USE GERRIT), 2017-03-07 15:08:41]

mstensho@opera.com changed reviewers:
+ eae@chromium.org, robhogan@gmail.com

[commit-bot: I haz the power, 2017-03-07 15:42:13]

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

[commit-bot: I haz the power, 2017-03-07 15:42:14]

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

[rhogan, 2017-03-07 19:01:50]

https://codereview.chromium.org/2738503004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/2738503004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1151: walker =
walker->nextInPreOrder(curr)) {
Nested inside for (LayoutObject* curr = currBlock->firstChild(); curr; curr =
curr->nextSibling()) it seems like this might lead us to visit
curr->nextSibling() more than once. Or are we always certain to hit our target
inside this nested loop?

[mstensho (USE GERRIT), 2017-03-07 19:45:13]

https://codereview.chromium.org/2738503004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/2738503004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1151: walker =
walker->nextInPreOrder(curr)) {
On 2017/03/07 19:01:50, rhogan wrote:
> Nested inside for (LayoutObject* curr = currBlock->firstChild(); curr; curr =
> curr->nextSibling()) it seems like this might lead us to visit
> curr->nextSibling() more than once.

nextInPreOrder(curr) forces us to stay within |curr| here. The method will
return nullptr instead of walking to |curr|'s sibling, when it's done with the
subtree.

> Or are we always certain to hit our target inside this nested loop?

No. Come to think of it, we could probably skip this subtree walk when
processing the first object returned from inlineElementContinuation(). Since we
have established that we're in a continuation chain, the end shouldn't be found
at the first object. But I'm not sure if I dare to do this. :)

[rhogan, 2017-03-07 20:00:31]

lgtm

https://codereview.chromium.org/2738503004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/2738503004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1151: walker =
walker->nextInPreOrder(curr)) {
On 2017/03/07 at 19:45:13, mstensho wrote:
> On 2017/03/07 19:01:50, rhogan wrote:
> > Nested inside for (LayoutObject* curr = currBlock->firstChild(); curr; curr
=
> > curr->nextSibling()) it seems like this might lead us to visit
> > curr->nextSibling() more than once.
> 
> nextInPreOrder(curr) forces us to stay within |curr| here. The method will
return nullptr instead of walking to |curr|'s sibling, when it's done with the
subtree.
> 

Oh right enough, the nested loop will always bail unless curr has a
slowFirstChild.

> > Or are we always certain to hit our target inside this nested loop?
> 
> No. Come to think of it, we could probably skip this subtree walk when
processing the first object returned from inlineElementContinuation(). Since we
have established that we're in a continuation chain, the end shouldn't be found
at the first object. But I'm not sure if I dare to do this. :)
:)

lgtm

[eae, 2017-03-08 01:19:47]

LGTM

[mstensho (USE GERRIT), 2017-03-08 08:49:19]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2017-03-08 08:49:38]

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-08 09:45:20]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1488962959040180, "parent_rev":
"43ae92a14fee8271f1c878b2615ec5728d07cd48", "commit_rev":
"e72c8c06b956706b54648589f807086d17340831"}

[commit-bot: I haz the power, 2017-03-08 09:46:00]

Description was changed from

==========
Search the entire subtree when looking for the end of an inline continuation
chain.

Inlines may be nested, so we may not find the last inline in the chain as a
direct child of the anonymous blocks. We need to search the entire subtree.
Don't do this with anonymous blocks that wrap block children (the block-level
DOM children of the inline-level objects), though. We're not going to find
anything interesting there.

This fix is speculative; the original bug report didn't come with a test case.

BUG=662767
==========

to

==========
Search the entire subtree when looking for the end of an inline continuation
chain.

Inlines may be nested, so we may not find the last inline in the chain as a
direct child of the anonymous blocks. We need to search the entire subtree.
Don't do this with anonymous blocks that wrap block children (the block-level
DOM children of the inline-level objects), though. We're not going to find
anything interesting there.

This fix is speculative; the original bug report didn't come with a test case.

BUG=662767

Review-Url: https://codereview.chromium.org/2738503004
Cr-Commit-Position: refs/heads/master@{#455420}
Committed:
https://chromium.googlesource.com/chromium/src/+/e72c8c06b956706b54648589f807...
==========

[commit-bot: I haz the power, 2017-03-08 09:46:01]

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