Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2298273002:
Initial exclusion aware layout opportunities implementation (Closed)
|
Created:
4 years, 3 months ago by eae Modified:
4 years, 3 months ago CC:
chromium-reviews, 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. |
DescriptionInitial exclusion aware layout opportunities implementation
Implement exclusion aware layout opportunities algorithm. At the moment
it only handles opportunities for a fixed starting point, sorted by the
width of the opportunities, widest first. Next step is to recompute for
the next exclusion once all current opportunities have been exhausted.
BUG=591099
R=ikilpatrick@chromium.org
Committed: https://crrev.com/ac9d3f8a067654f9059e6ab00051be9102ff471e
Cr-Commit-Position: refs/heads/master@{#417562}
Patch Set 1 : Initial exclusion aware layout opportunities implementation #
Total comments: 17
Patch Set 2 : address reviewer comments #
Total comments: 10
Patch Set 3 : Address reviewer comments #
Messages
Total messages: 41 (31 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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: 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...)
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: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by eae@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Description was changed from ========== WIP layout opp ========== to ========== Initial exclusion aware layout opportunities implementation Implement exclusion aware layout opportunities algorithm. At the moment it only handles opportunities for a fixed starting point, sorted by the width of the opportunities, widest first. Next step is to recompute for the next exclusion once all current opportunities have been exhausted. BUG=591099 R=ikilpatrick@chromium.org ==========
eae@chromium.org changed reviewers: + ikilpatrick@chromium.org
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
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.
https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:181: filtered_exclusions_.append(item); I'm assuming: filtered_exclusions_(constaint_space_->Phy()->Exclusions()) doesn't work b/c it's a const reference? https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:196: if (exclusion.Right() <= left || exclusion.Bottom() <= top) { I think we'll eventually want to do this in logical b/c writing modes. https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:211: LayoutUnit NGLayoutOpportunityIterator::heightForOpportunity( I'd comment this with: For the given 2d range (opportunity); will return a height which makes it bounded by the lowest exclusion in the filtered exclusion list https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:216: LayoutUnit minBottom = bottom; lowestBottom? https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:243: if (clear_ != NGClearNone && for_inline_or_bfc_) DCHECK? hmm... not sure. https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:247: if (opportunityHeight && right > left) explain / rename variables for this magic. https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:250: for (const NGExclusion& exclusion : filtered_exclusions_) { add comment here. https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:271: NGConstraintSpace* NGLayoutOpportunityIterator::Next() { i'd place this right below the ctor, so people know which fn to read next; but up to you. https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:154: unsigned current_exclusion_; current_exclusion_idx_ ? up to you.
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...
Thanks for the thorough review; please take another look. https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:181: filtered_exclusions_.append(item); On 2016/09/06 23:09:40, ikilpatrick wrote: > I'm assuming: > filtered_exclusions_(constaint_space_->Phy()->Exclusions()) doesn't work b/c > it's a const reference? Correct https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:196: if (exclusion.Right() <= left || exclusion.Bottom() <= top) { On 2016/09/06 23:09:40, ikilpatrick wrote: > I think we'll eventually want to do this in logical b/c writing modes. Yeah, I think so too. At least for the inline case/bfc case. https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:211: LayoutUnit NGLayoutOpportunityIterator::heightForOpportunity( On 2016/09/06 23:09:40, ikilpatrick wrote: > I'd comment this with: > > For the given 2d range (opportunity); will return a height which makes it > bounded by the lowest exclusion in the filtered exclusion list Good one, done! https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:216: LayoutUnit minBottom = bottom; On 2016/09/06 23:09:40, ikilpatrick wrote: > lowestBottom? Done. https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:243: if (clear_ != NGClearNone && for_inline_or_bfc_) On 2016/09/06 23:09:40, ikilpatrick wrote: > DCHECK? hmm... not sure. This is all very temporary... https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:250: for (const NGExclusion& exclusion : filtered_exclusions_) { On 2016/09/06 23:09:40, ikilpatrick wrote: > add comment here. Done. https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:271: NGConstraintSpace* NGLayoutOpportunityIterator::Next() { On 2016/09/06 23:09:40, ikilpatrick wrote: > i'd place this right below the ctor, so people know which fn to read next; but > up to you. Good idea. https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2298273002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:154: unsigned current_exclusion_; On 2016/09/06 23:09:40, ikilpatrick wrote: > current_exclusion_idx_ ? up to you. I like that, changed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ikilpatrick@chromium.org changed reviewers: + glebl@chromium.org
Please take another look when you get a chance.
lgtm, sorry for the delay i was swamped yesterday; I think gleb also has some comments about comments :) https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:277: // bounded by the lowest exclusion in the filtered exclusion list. I've just realized that s/lowest/highest right? (everywhere) https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:277: // bounded by the lowest exclusion in the filtered exclusion list. Maybe For the given 2D range (opportunity), this will return a height which makes it bounded by the highest exclusion in the filtered exclusion list within the range. Returns 0-height for an invalid opportunity, (which has zero area).
lgtm I have a couple of minor comments regarding the code style. I will gladly fix them by myself if you want. https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:168: static bool ascendingTopCompare(const NGExclusion& a, const NGExclusion& b) { make it inline or move it to the anonymous namespace. https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:168: static bool ascendingTopCompare(const NGExclusion& a, const NGExclusion& b) { all functions should start with a capital letter https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:180: for (auto item : constraint_space_->PhysicalSpace()->Exclusions()) const auto& ? https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:186: // TODO: Set based on offset once that has been moved to NGConstraintSpace. TODO should be followed with a bug number or ldap/group email address. https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:212: if (!current_opportunities_.size() && it's advised to use isEmpty instead of size() to check vector for emptiness. https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:233: static bool descendingWidthCompare(const NGConstraintSpace* a, inline or move to anonymous namespace https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:292: void NGLayoutOpportunityIterator::addLayoutOpportunity(LayoutUnit left, should we use const references for left, top, right and bottom ? https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (left): https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:124: NGLayoutOpportunityIterator(NGConstraintSpace* space, - can we add some comments to this constructor and its parameters, especially when we use abbreviations Example: // Default constructor. // @param space ... // @param is_for_inline_or_bfc Whether NGLayoutOpportunityIterator(NGConstraintSpace* space, ... - Should we prefix all boolean variable with is_ ? i.e. for_inline_or_bfc_ => is_for_inline_or_bfc, same for clear_ ? - since constraint_space_ is not a Persistent object anymore, we probably can pass it by const reference ?
On 2016/09/08 20:44:20, glebl wrote: > lgtm > I have a couple of minor comments regarding the code style. I will gladly fix > them by myself if you want. > > https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): > > https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:168: static bool > ascendingTopCompare(const NGExclusion& a, const NGExclusion& b) { > make it inline or move it to the anonymous namespace. Done > > https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:168: static bool > ascendingTopCompare(const NGExclusion& a, const NGExclusion& b) { > all functions should start with a capital letter > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Done > https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:180: for (auto > item : constraint_space_->PhysicalSpace()->Exclusions()) > const auto& ? Done > https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:186: // TODO: > Set based on offset once that has been moved to NGConstraintSpace. > TODO should be followed with a bug number or ldap/group email address. > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Done > https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:212: if > (!current_opportunities_.size() && > it's advised to use isEmpty instead of size() to check vector for emptiness. Done > https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:233: static bool > descendingWidthCompare(const NGConstraintSpace* a, > inline or move to anonymous namespace > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done > https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:292: void > NGLayoutOpportunityIterator::addLayoutOpportunity(LayoutUnit left, > should we use const references for left, top, right and bottom ? We did a lot of testing when we added LayoutUnits a few years ago and passing by value is significantly faster. > https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (left): > > https://codereview.chromium.org/2298273002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:124: > NGLayoutOpportunityIterator(NGConstraintSpace* space, > - can we add some comments to this constructor and its parameters, especially > when we use abbreviations > Example: > // Default constructor. > // @param space ... > // @param is_for_inline_or_bfc Whether > NGLayoutOpportunityIterator(NGConstraintSpace* space, > ... > > - Should we prefix all boolean variable with is_ ? i.e. for_inline_or_bfc_ => > is_for_inline_or_bfc, same for clear_ ? Yes, we should. The parameters are about to change though, will add updated comments in a follow up change.
Thanks for the detailed review both of you!
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, glebl@chromium.org Link to the patchset: https://codereview.chromium.org/2298273002/#ps120001 (title: "Address reviewer comments")
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.
Description was changed from ========== Initial exclusion aware layout opportunities implementation Implement exclusion aware layout opportunities algorithm. At the moment it only handles opportunities for a fixed starting point, sorted by the width of the opportunities, widest first. Next step is to recompute for the next exclusion once all current opportunities have been exhausted. BUG=591099 R=ikilpatrick@chromium.org ========== to ========== Initial exclusion aware layout opportunities implementation Implement exclusion aware layout opportunities algorithm. At the moment it only handles opportunities for a fixed starting point, sorted by the width of the opportunities, widest first. Next step is to recompute for the next exclusion once all current opportunities have been exhausted. BUG=591099 R=ikilpatrick@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Initial exclusion aware layout opportunities implementation Implement exclusion aware layout opportunities algorithm. At the moment it only handles opportunities for a fixed starting point, sorted by the width of the opportunities, widest first. Next step is to recompute for the next exclusion once all current opportunities have been exhausted. BUG=591099 R=ikilpatrick@chromium.org ========== to ========== Initial exclusion aware layout opportunities implementation Implement exclusion aware layout opportunities algorithm. At the moment it only handles opportunities for a fixed starting point, sorted by the width of the opportunities, widest first. Next step is to recompute for the next exclusion once all current opportunities have been exhausted. BUG=591099 R=ikilpatrick@chromium.org Committed: https://crrev.com/ac9d3f8a067654f9059e6ab00051be9102ff471e Cr-Commit-Position: refs/heads/master@{#417562} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ac9d3f8a067654f9059e6ab00051be9102ff471e Cr-Commit-Position: refs/heads/master@{#417562} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
