Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(3)

Issue 2733133002: Combine 2 exclusions in Layout Opportunity Tree if they shadow each other (Closed)

Created:
3 years, 9 months ago by Gleb Lanbin
Modified:
3 years, 9 months ago
Reviewers:
cbiesinger, ikilpatrick
CC:
chromium-reviews, 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
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Combine 2 exclusions in Layout Opportunity Tree if they shadow each other This patch introduces the logic that tries to combine 2 exclusions if possible. We can combine 2 exclusions if - they are adjoining to each other and have the same exclusion type - the new exclusion shadows the old one. That's because it's not allowed to position anything in the shadowed area. Example: <div id="SS" style="float: left; height: 10px; width: 10px"></div> <div id="BB" style="float: left; height: 20px; width: 20px"></div> +----------------+ |SSBB |**BB We combine SS and BB exclusions including the shadowed area (**). BUG=635619 TESTS=TwoRightExclusionsShadowEachOther, TwoLeftExclusionsShadowEachOther Review-Url: https://codereview.chromium.org/2733133002 Cr-Commit-Position: refs/heads/master@{#455444} Committed: https://chromium.googlesource.com/chromium/src/+/5ccd9ec6998eee48bc73e7d8a992c4cccdb31899

Patch Set 1 #

Total comments: 1

Patch Set 2 : add NGExclusion::MaybeCombineWith #

Total comments: 1

Patch Set 3 : fix comments #

Patch Set 4 : git rebase-update #

Patch Set 5 : delete unreachable return statement #

Messages

Total messages: 31 (21 generated)
Gleb Lanbin
3 years, 9 months ago (2017-03-07 01:19:54 UTC) #4
cbiesinger
lgtm https://codereview.chromium.org/2733133002/diff/1/third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc File third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc (right): https://codereview.chromium.org/2733133002/diff/1/third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc#newcode203 third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc:203: bool MaybeCombineExclusions(const NGExclusion& in_exclusion, I had a bit ...
3 years, 9 months ago (2017-03-07 19:10:42 UTC) #6
Gleb Lanbin
On 2017/03/07 19:10:42, cbiesinger wrote: > lgtm > > https://codereview.chromium.org/2733133002/diff/1/third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc > File third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc > (right): ...
3 years, 9 months ago (2017-03-07 20:53:47 UTC) #7
cbiesinger
That's much better, thank you! lgtm
3 years, 9 months ago (2017-03-07 20:56:37 UTC) #8
cbiesinger
https://codereview.chromium.org/2733133002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_exclusion.h File third_party/WebKit/Source/core/layout/ng/ng_exclusion.h (right): https://codereview.chromium.org/2733133002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_exclusion.h#newcode37 third_party/WebKit/Source/core/layout/ng/ng_exclusion.h:37: // Combines the current exclusion with {@code other} exclusion. ...
3 years, 9 months ago (2017-03-07 20:56:51 UTC) #9
Gleb Lanbin
On 2017/03/07 20:56:51, cbiesinger wrote: > https://codereview.chromium.org/2733133002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_exclusion.h > File third_party/WebKit/Source/core/layout/ng/ng_exclusion.h (right): > > https://codereview.chromium.org/2733133002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_exclusion.h#newcode37 > ...
3 years, 9 months ago (2017-03-07 21:34:57 UTC) #11
cbiesinger
lgtm
3 years, 9 months ago (2017-03-07 21:42:13 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2733133002/100001
3 years, 9 months ago (2017-03-08 13:52:27 UTC) #27
commit-bot: I haz the power
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/5ccd9ec6998eee48bc73e7d8a992c4cccdb31899
3 years, 9 months ago (2017-03-08 13:57:25 UTC) #30
agrieve
3 years, 9 months ago (2017-03-08 21:09:25 UTC) #31
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/2732223007/ by agrieve@chromium.org.

The reason for reverting is: Broke downstream bot (see bug).

Powered by Google App Engine
This is Rietveld 408576698