Replace collectLayerFragments() with FragmentainerIterator.

Replace collectLayerFragments() with FragmentainerIterator.

No functional changes intended.

The way collectLayerFragments() and PaintLayer::collectFragments() cooperated
to populate the PaintLayerFragments was rather messy. collectLayerFragments()
set two members, and then, later on, PaintLayer::collectFragments() would set
the remaining members, translate and clip. With this change, the layout code no
longer uses PaintLayerFragments.

It should also be mentioned that collectLayerFragments() was a bad name,
because this functionality shouldn't be strictly for layers. We're soon going
to need it for getClientRects() as well.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/bf0d65b763505b57663425ae3499d766e55ba395
Cr-Commit-Position: refs/heads/master@{#417271}

[mstensho (USE GERRIT), 2016-09-06 12:15:35]

Description was changed from

==========
Replace collectLayerFragments() with FragmentainerIterator.

No functional changes intended.

The way collectLayerFragments() and PaintLayer::collectFragments() cooperated
to populate the PaintLayerFragments was rather messy. collectLayerFragments()
set two members, and then, later on, PaintLayer::collectFragments() would set
the remaining members, translate and clip. With this change, the layout code no
longer uses PaintLayerFragments.

It should also be mentioned that collectLayerFragments() was a bad name,
because this functionality shouldn't be strictly for layers. We're soon going
to need it for getClientRects() as well.

Change the reference point of the clip rect passed from PaintLayer. Instead of
being relative to the flow thread, it's now relative to the multicol container.
This happens to be in line with the comment there, so now we're actually doing
what we say we're doing. This also means that we no longer have to convert it
afterwards to become relative to the multicol container in
FragmentainerIterator, which is what we had to do in now-gone
collectLayerFragments().
==========

to

==========
Replace collectLayerFragments() with FragmentainerIterator.

No functional changes intended.

The way collectLayerFragments() and PaintLayer::collectFragments() cooperated
to populate the PaintLayerFragments was rather messy. collectLayerFragments()
set two members, and then, later on, PaintLayer::collectFragments() would set
the remaining members, translate and clip. With this change, the layout code no
longer uses PaintLayerFragments.

It should also be mentioned that collectLayerFragments() was a bad name,
because this functionality shouldn't be strictly for layers. We're soon going
to need it for getClientRects() as well.

Change the reference point of the clip rect passed from PaintLayer. Instead of
being relative to the flow thread, it's now relative to the multicol container.
This happens to be in line with the comment there, so now we're actually doing
what we say we're doing. This also means that we no longer have to convert it
afterwards to become relative to the multicol container in
FragmentainerIterator, which is what we had to do in now-gone
collectLayerFragments().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[mstensho (USE GERRIT), 2016-09-06 12:15:50]

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

[commit-bot: I haz the power, 2016-09-06 12:16:04]

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-09-06 13:58:52]

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

[commit-bot: I haz the power, 2016-09-06 13:58:55]

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

[mstensho (USE GERRIT), 2016-09-07 07:58:16]

Description was changed from

==========
Replace collectLayerFragments() with FragmentainerIterator.

No functional changes intended.

The way collectLayerFragments() and PaintLayer::collectFragments() cooperated
to populate the PaintLayerFragments was rather messy. collectLayerFragments()
set two members, and then, later on, PaintLayer::collectFragments() would set
the remaining members, translate and clip. With this change, the layout code no
longer uses PaintLayerFragments.

It should also be mentioned that collectLayerFragments() was a bad name,
because this functionality shouldn't be strictly for layers. We're soon going
to need it for getClientRects() as well.

Change the reference point of the clip rect passed from PaintLayer. Instead of
being relative to the flow thread, it's now relative to the multicol container.
This happens to be in line with the comment there, so now we're actually doing
what we say we're doing. This also means that we no longer have to convert it
afterwards to become relative to the multicol container in
FragmentainerIterator, which is what we had to do in now-gone
collectLayerFragments().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace collectLayerFragments() with FragmentainerIterator.

No functional changes intended.

The way collectLayerFragments() and PaintLayer::collectFragments() cooperated
to populate the PaintLayerFragments was rather messy. collectLayerFragments()
set two members, and then, later on, PaintLayer::collectFragments() would set
the remaining members, translate and clip. With this change, the layout code no
longer uses PaintLayerFragments.

It should also be mentioned that collectLayerFragments() was a bad name,
because this functionality shouldn't be strictly for layers. We're soon going
to need it for getClientRects() as well.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[mstensho (USE GERRIT), 2016-09-07 08:07:40]

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

[commit-bot: I haz the power, 2016-09-07 08:07:56]

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-09-07 09:43:17]

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

[commit-bot: I haz the power, 2016-09-07 09:43:18]

Dry run: This issue passed the CQ dry run.

[mstensho (USE GERRIT), 2016-09-07 09:49:28]

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

[mstensho (USE GERRIT), 2016-09-07 09:49:28]

This one is ready for review. My initial attempt had
https://codereview.chromium.org/2310233002/ as part of this CL, but it turned
out that this actually affected behavior (by fixing a bug), so it had to be
landed separately first.

[eae, 2016-09-08 08:15:03]

LGTM w/nit

https://codereview.chromium.org/2314763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/FragmentainerIterator.cpp (right):

https://codereview.chromium.org/2314763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/FragmentainerIterator.cpp:57: if
(m_currentFragmentainerIndex > m_endFragmentainerIndex) {
How about reversing this logic instead of increasing the index beyond the end?

if (m_currentFragmentainerIndex < m_endFragmentainerIndex) {
  m_currentFragmentainerIndex++;
} else {
  moveToNextFragmentainerGroup();
  if (atEnd())
    return;
}

https://codereview.chromium.org/2314763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2314763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:1581: // TODO(mstensho):
Don't add empty fragments. We've always done that in some cases, but it
This comment seems to suggest that we shouldn't add empty fragments AND that we
should be filtering them. 

You can either have your empty cake or not-eat it. :)

[mstensho (USE GERRIT), 2016-09-08 08:59:51]

https://codereview.chromium.org/2314763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/FragmentainerIterator.cpp (right):

https://codereview.chromium.org/2314763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/FragmentainerIterator.cpp:57: if
(m_currentFragmentainerIndex > m_endFragmentainerIndex) {
On 2016/09/08 08:15:03, eae wrote:
> How about reversing this logic instead of increasing the index beyond the end?
> 
> if (m_currentFragmentainerIndex < m_endFragmentainerIndex) {
>   m_currentFragmentainerIndex++;
> } else {
>   moveToNextFragmentainerGroup();
>   if (atEnd())
>     return;
> }

Done. Very nice! I like!

https://codereview.chromium.org/2314763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2314763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:1581: // TODO(mstensho):
Don't add empty fragments. We've always done that in some cases, but it
On 2016/09/08 08:15:03, eae wrote:
> This comment seems to suggest that we shouldn't add empty fragments AND that
we
> should be filtering them. 
> 
> You can either have your empty cake or not-eat it. :)

Done.

[mstensho (USE GERRIT), 2016-09-08 09:00:19]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2016-09-08 09:00:20]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2314763002/#ps40001
(title: "code review.")

[commit-bot: I haz the power, 2016-09-08 09:00:37]

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-09-08 09:07:17]

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

[commit-bot: I haz the power, 2016-09-08 09:07:18]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[eae, 2016-09-08 09:17:21]

Thanks, LGTM++

[mstensho (USE GERRIT), 2016-09-08 09:33:29]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2016-09-08 09:33:45]

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-09-08 10:26:12]

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

[commit-bot: I haz the power, 2016-09-08 10:26:12]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[mstensho (USE GERRIT), 2016-09-08 11:08:23]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2016-09-08 11:08:44]

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-09-08 13:25:08]

Description was changed from

==========
Replace collectLayerFragments() with FragmentainerIterator.

No functional changes intended.

The way collectLayerFragments() and PaintLayer::collectFragments() cooperated
to populate the PaintLayerFragments was rather messy. collectLayerFragments()
set two members, and then, later on, PaintLayer::collectFragments() would set
the remaining members, translate and clip. With this change, the layout code no
longer uses PaintLayerFragments.

It should also be mentioned that collectLayerFragments() was a bad name,
because this functionality shouldn't be strictly for layers. We're soon going
to need it for getClientRects() as well.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace collectLayerFragments() with FragmentainerIterator.

No functional changes intended.

The way collectLayerFragments() and PaintLayer::collectFragments() cooperated
to populate the PaintLayerFragments was rather messy. collectLayerFragments()
set two members, and then, later on, PaintLayer::collectFragments() would set
the remaining members, translate and clip. With this change, the layout code no
longer uses PaintLayerFragments.

It should also be mentioned that collectLayerFragments() was a bad name,
because this functionality shouldn't be strictly for layers. We're soon going
to need it for getClientRects() as well.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-09-08 13:25:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-08 13:28:30]

Description was changed from

==========
Replace collectLayerFragments() with FragmentainerIterator.

No functional changes intended.

The way collectLayerFragments() and PaintLayer::collectFragments() cooperated
to populate the PaintLayerFragments was rather messy. collectLayerFragments()
set two members, and then, later on, PaintLayer::collectFragments() would set
the remaining members, translate and clip. With this change, the layout code no
longer uses PaintLayerFragments.

It should also be mentioned that collectLayerFragments() was a bad name,
because this functionality shouldn't be strictly for layers. We're soon going
to need it for getClientRects() as well.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace collectLayerFragments() with FragmentainerIterator.

No functional changes intended.

The way collectLayerFragments() and PaintLayer::collectFragments() cooperated
to populate the PaintLayerFragments was rather messy. collectLayerFragments()
set two members, and then, later on, PaintLayer::collectFragments() would set
the remaining members, translate and clip. With this change, the layout code no
longer uses PaintLayerFragments.

It should also be mentioned that collectLayerFragments() was a bad name,
because this functionality shouldn't be strictly for layers. We're soon going
to need it for getClientRects() as well.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/bf0d65b763505b57663425ae3499d766e55ba395
Cr-Commit-Position: refs/heads/master@{#417271}
==========

[commit-bot: I haz the power, 2016-09-08 13:28:31]

Patchset 3 (id:??) landed as
https://crrev.com/bf0d65b763505b57663425ae3499d766e55ba395
Cr-Commit-Position: refs/heads/master@{#417271}