|
|
Created:
3 years, 6 months ago by ikilpatrick Modified:
3 years, 5 months ago CC:
chromium-reviews, cbiesinger, ojan+watch_chromium.org, szager+layoutwatch_chromium.org, glebl+reviews_chromium.org, dgrogan+ng_chromium.org, atotic+reviews_chromium.org, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, zoltan1, jchaffraix+rendering, blink-reviews, lchoi+reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[LayoutNG] Float behavior simplification before larger patch.
This is just an intermediate patch which simplifies some of the float
logic.
As we won't be "re-parenting" floats away from their actual parents
anymore a lot of the complex logic around the offsets becomes simpler.
Specifically we won't need the "paint-offset" calculation anymore.
This is just an initial patch to reduce the behaviour, it has some
regressions, but these should be fixed with the new code.
BUG=635619
Review-Url: https://codereview.chromium.org/2945213004
Cr-Commit-Position: refs/heads/master@{#483826}
Committed: https://chromium.googlesource.com/chromium/src/+/e65e2d1acb3c3bb037f0183bdd3db4cf35da45c2
Patch Set 1 #Patch Set 2 : ... #Patch Set 3 : reduce++ #Patch Set 4 : more simplifying #Patch Set 5 : rawr. #Patch Set 6 : falalala. #Patch Set 7 : testrun #Patch Set 8 : try? #Patch Set 9 : testrun #Patch Set 10 : testrun #Patch Set 11 : [LayoutNG] Float behavior simplification before larger patch. #
Total comments: 2
Patch Set 12 : update test expectations. #
Messages
Total messages: 48 (42 generated)
Description was changed from ========== testrun BUG= ========== to ========== testrun BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ==========
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_layout_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_layout_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_layout_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== testrun BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ========== to ========== This is just an intermediate patch which simplifies some of the float logic. As we won't be "re-parenting" floats away from their actual parents anymore a lot of the complex logic around the offsets becomes simpler. Specifically we won't need the "paint-offset" calculation anymore. This is just an initial patch to reduce the behaviour, it has some regressions, but these should be fixed with the new code. BUG=635619 ==========
ikilpatrick@chromium.org changed reviewers: + cbiesinger@chromium.org, eae@chromium.org, kojii@chromium.org
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I like it, thanks for working on this! https://codereview.chromium.org/2945213004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2945213004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:108: container_builder->MutableUnpositionedFloats().clear(); Exposing the mutable list like this is a little weird. How about having the UnpositionedFloats method use move semantics to transfer ownership of the list? That way this code would take ownership of the unpositioned floats and the clear method will no longer be needed.
https://codereview.chromium.org/2945213004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2945213004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:108: container_builder->MutableUnpositionedFloats().clear(); On 2017/06/29 22:21:18, eae wrote: > Exposing the mutable list like this is a little weird. How about having the > UnpositionedFloats method use move semantics to transfer ownership of the list? > > That way this code would take ownership of the unpositioned floats and the clear > method will no longer be needed. So this will actually go away in the larger patch, as both the block&inline layout algorithms will have their own list of unpositioned_floats now... removing this now would be a larger change :).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== This is just an intermediate patch which simplifies some of the float logic. As we won't be "re-parenting" floats away from their actual parents anymore a lot of the complex logic around the offsets becomes simpler. Specifically we won't need the "paint-offset" calculation anymore. This is just an initial patch to reduce the behaviour, it has some regressions, but these should be fixed with the new code. BUG=635619 ========== to ========== This is just an intermediate patch which simplifies some of the float logic. As we won't be "re-parenting" floats away from their actual parents anymore a lot of the complex logic around the offsets becomes simpler. Specifically we won't need the "paint-offset" calculation anymore. This is just an initial patch to reduce the behaviour, it has some regressions, but these should be fixed with the new code. BUG=635619 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ==========
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/29 22:34:42, ikilpatrick wrote: > https://codereview.chromium.org/2945213004/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc > (right): > > https://codereview.chromium.org/2945213004/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:108: > container_builder->MutableUnpositionedFloats().clear(); > On 2017/06/29 22:21:18, eae wrote: > > Exposing the mutable list like this is a little weird. How about having the > > UnpositionedFloats method use move semantics to transfer ownership of the > list? > > > > That way this code would take ownership of the unpositioned floats and the > clear > > method will no longer be needed. > > So this will actually go away in the larger patch, as both the block&inline > layout algorithms will have their own list of unpositioned_floats now... > removing this now would be a larger change :). Ah, yay! LGTM
Description was changed from ========== This is just an intermediate patch which simplifies some of the float logic. As we won't be "re-parenting" floats away from their actual parents anymore a lot of the complex logic around the offsets becomes simpler. Specifically we won't need the "paint-offset" calculation anymore. This is just an initial patch to reduce the behaviour, it has some regressions, but these should be fixed with the new code. BUG=635619 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ========== to ========== This is just an intermediate patch which simplifies some of the float logic. As we won't be "re-parenting" floats away from their actual parents anymore a lot of the complex logic around the offsets becomes simpler. Specifically we won't need the "paint-offset" calculation anymore. This is just an initial patch to reduce the behaviour, it has some regressions, but these should be fixed with the new code. BUG=635619 ==========
Description was changed from ========== This is just an intermediate patch which simplifies some of the float logic. As we won't be "re-parenting" floats away from their actual parents anymore a lot of the complex logic around the offsets becomes simpler. Specifically we won't need the "paint-offset" calculation anymore. This is just an initial patch to reduce the behaviour, it has some regressions, but these should be fixed with the new code. BUG=635619 ========== to ========== [LayoutNG] Float behavior simplification before larger patch. This is just an intermediate patch which simplifies some of the float logic. As we won't be "re-parenting" floats away from their actual parents anymore a lot of the complex logic around the offsets becomes simpler. Specifically we won't need the "paint-offset" calculation anymore. This is just an initial patch to reduce the behaviour, it has some regressions, but these should be fixed with the new code. BUG=635619 ==========
The CQ bit was unchecked by ikilpatrick@chromium.org
The CQ bit was checked by ikilpatrick@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 220001, "attempt_start_ts": 1498855967049500, "parent_rev": "99936ad737f80a465b479d6bc0abacd22767deaf", "commit_rev": "e65e2d1acb3c3bb037f0183bdd3db4cf35da45c2"}
Message was sent while issue was closed.
Description was changed from ========== [LayoutNG] Float behavior simplification before larger patch. This is just an intermediate patch which simplifies some of the float logic. As we won't be "re-parenting" floats away from their actual parents anymore a lot of the complex logic around the offsets becomes simpler. Specifically we won't need the "paint-offset" calculation anymore. This is just an initial patch to reduce the behaviour, it has some regressions, but these should be fixed with the new code. BUG=635619 ========== to ========== [LayoutNG] Float behavior simplification before larger patch. This is just an intermediate patch which simplifies some of the float logic. As we won't be "re-parenting" floats away from their actual parents anymore a lot of the complex logic around the offsets becomes simpler. Specifically we won't need the "paint-offset" calculation anymore. This is just an initial patch to reduce the behaviour, it has some regressions, but these should be fixed with the new code. BUG=635619 Review-Url: https://codereview.chromium.org/2945213004 Cr-Commit-Position: refs/heads/master@{#483826} Committed: https://chromium.googlesource.com/chromium/src/+/e65e2d1acb3c3bb037f0183bdd3d... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/e65e2d1acb3c3bb037f0183bdd3d... |