|
|
Created:
4 years, 3 months ago by eae Modified:
4 years, 3 months ago CC:
chromium-reviews, cbiesinger, ojan+watch_chromium.org, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[LayoutNG] Multi-exclusion aware layout opportunities
Update layout opportunities implementation to handle multiple exclusions
and to support moving down to the next line if no suitable opportunities
can be found on the original line or once all those have been exhausted.
Very simple and un-optimized algorithm, trading performance for clarity.
BUG=635619
R=cbiesinger@chromium.org,ikilpatrick@chromium.org
Committed: https://crrev.com/882e244a67cc80983cd3cc9eb648f2df18bcc4bf
Cr-Commit-Position: refs/heads/master@{#420025}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address reviwer comments #Patch Set 3 : Rebase w/HEAD #
Messages
Total messages: 44 (24 generated)
The CQ bit was checked by eae@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...
Please hold off reviewing this for a bit.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Ready for review
The CQ bit was checked by eae@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...
cool, I'll have a look after lunch.
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_...)
Can haz review?
On 2016/09/16 13:19:59, eae wrote: > Can haz review? The string tests make me happy :) So I expected a different set of opportunities to appear I left a couple of comments regarding that, but the algorithms look sane.
https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc (right): https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc:143: EXPECT_EQ("0,0 150x400", OpportunityToString(iterator->Next())); where is the "350,0 250x350" exclusion? I.e. directly to the right of the first exclusion? https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc:147: // have a different alignment. How does alignment come into this? (I might be missing something) https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc (right): https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc:20: currentX_(0), current_x_ ? & elsewhere.
Thanks for the review; comments inline. https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc (right): https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc:143: EXPECT_EQ("0,0 150x400", OpportunityToString(iterator->Next())); On 2016/09/16 16:47:51, ikilpatrick wrote: > where is the > "350,0 250x350" exclusion? > > I.e. directly to the right of the first exclusion? I tried to model it based on my understanding of inlines with a length-first priority. As such the current implementation will not find all possible opportunities. We'll likely have to adjust this, especially for floats. https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc:147: // have a different alignment. On 2016/09/16 16:47:51, ikilpatrick wrote: > How does alignment come into this? (I might be missing something) We don't have alignment support yet but once we do this opportunity would allow for alignment with the top edge of the exclusion. https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc (right): https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc:20: currentX_(0), On 2016/09/16 16:47:51, ikilpatrick wrote: > current_x_ ? & elsewhere. Good catch, I'll change it!
The CQ bit was checked by eae@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: This issue passed the CQ dry run.
PTAL
lgtm https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc (right): https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc:143: EXPECT_EQ("0,0 150x400", OpportunityToString(iterator->Next())); On 2016/09/16 19:07:49, eae wrote: > On 2016/09/16 16:47:51, ikilpatrick wrote: > > where is the > > "350,0 250x350" exclusion? > > > > I.e. directly to the right of the first exclusion? > > I tried to model it based on my understanding of inlines with a length-first > priority. As such the current implementation will not find all possible > opportunities. We'll likely have to adjust this, especially for floats. ok, sg. its a bit difficult as we don't really have these "middle" exclusion types yet. https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc:147: // have a different alignment. On 2016/09/16 19:07:49, eae wrote: > On 2016/09/16 16:47:51, ikilpatrick wrote: > > How does alignment come into this? (I might be missing something) > > We don't have alignment support yet but once we do this opportunity would allow > for alignment with the top edge of the exclusion. ah ok. i think that this would actually want to be a separate argument into the iterator as i'm not sure that all modes will want this; but fine for the moment.
On 2016/09/20 11:05:00, ikilpatrick wrote: > lgtm > > https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc > (right): > > https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc:143: > EXPECT_EQ("0,0 150x400", OpportunityToString(iterator->Next())); > On 2016/09/16 19:07:49, eae wrote: > > On 2016/09/16 16:47:51, ikilpatrick wrote: > > > where is the > > > "350,0 250x350" exclusion? > > > > > > I.e. directly to the right of the first exclusion? > > > > I tried to model it based on my understanding of inlines with a length-first > > priority. As such the current implementation will not find all possible > > opportunities. We'll likely have to adjust this, especially for floats. > > ok, sg. its a bit difficult as we don't really have these "middle" exclusion > types yet. > > https://codereview.chromium.org/2347663002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc:147: // > have a different alignment. > On 2016/09/16 19:07:49, eae wrote: > > On 2016/09/16 16:47:51, ikilpatrick wrote: > > > How does alignment come into this? (I might be missing something) > > > > We don't have alignment support yet but once we do this opportunity would > allow > > for alignment with the top edge of the exclusion. > > ah ok. i think that this would actually want to be a separate argument into the > iterator as i'm not sure that all modes will want this; but fine for the moment. SG. I'll do a follow up and see about adding the "missing" opportunities you expect.
The CQ bit was checked by eae@chromium.org
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
Try jobs failed on following builders: 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_...)
The CQ bit was checked by eae@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org Link to the patchset: https://codereview.chromium.org/2347663002/#ps60001 (title: "Rebase w/HEAD")
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by eae@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org Link to the patchset: https://codereview.chromium.org/2347663002/#ps80001 (title: "Rebase w/HEAD")
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
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...)
The CQ bit was checked by eae@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [LayoutNG] Multi-exclusion aware layout opportunities Update layout opportunities implementation to handle multiple exclusions and to support moving down to the next line if no suitable opportunities can be found on the original line or once all those have been exhausted. Very simple and un-optimized algorithm, trading performance for clarity. BUG=635619 R=cbiesinger@chromium.org,ikilpatrick@chromium.org ========== to ========== [LayoutNG] Multi-exclusion aware layout opportunities Update layout opportunities implementation to handle multiple exclusions and to support moving down to the next line if no suitable opportunities can be found on the original line or once all those have been exhausted. Very simple and un-optimized algorithm, trading performance for clarity. BUG=635619 R=cbiesinger@chromium.org,ikilpatrick@chromium.org Committed: https://crrev.com/882e244a67cc80983cd3cc9eb648f2df18bcc4bf Cr-Commit-Position: refs/heads/master@{#420025} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/882e244a67cc80983cd3cc9eb648f2df18bcc4bf Cr-Commit-Position: refs/heads/master@{#420025} |