Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2886453005:
Account for border/padding in NG Inline layout algorithm. (Closed)
|
Created:
3 years, 7 months ago by Gleb Lanbin Modified:
3 years, 7 months ago CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, cbiesinger, chromium-reviews, dgrogan+ng_chromium.org, eae+blinkwatch, glebl+reviews_chromium.org, jchaffraix+rendering, leviw+renderwatch, ojan+watch_chromium.org, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAccount for border/padding in NG Inline layout algorithm.
BUG=635619
TESTS=79 tests
Review-Url: https://codereview.chromium.org/2886453005
Cr-Commit-Position: refs/heads/master@{#472323}
Committed: https://chromium.googlesource.com/chromium/src/+/59bb4d543e23a248553affa3dc9de1e01a1ce228
Patch Set 1 #
Total comments: 6
Patch Set 2 : fix comments #Patch Set 3 : git rebase-update #Patch Set 4 : Skip some tests becase of crbug.com/713891 #
Messages
Total messages: 34 (23 generated)
glebl@chromium.org changed reviewers: + ikilpatrick@chromium.org, kojii@chromium.org
The CQ bit was checked by glebl@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...
lgtm https://codereview.chromium.org/2886453005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc (right): https://codereview.chromium.org/2886453005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc:134: border_and_padding_ = ComputeBorders(ConstraintSpace(), Style()) + does it make sense for this to be in ctor? Initialize is sortof just for the Items() ? not sure.
thanks for the prompt lgtm. https://codereview.chromium.org/2886453005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc (right): https://codereview.chromium.org/2886453005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc:134: border_and_padding_ = ComputeBorders(ConstraintSpace(), Style()) + On 2017/05/15 22:53:41, ikilpatrick wrote: > does it make sense for this to be in ctor? Initialize is sortof just for the > Items() ? not sure. Koji, what do you think? Should I move this to the constructor?
lgtm https://codereview.chromium.org/2886453005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc (right): https://codereview.chromium.org/2886453005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc:471: NGLogicalOffset opp_offset = What is "opp"?
thanks. https://codereview.chromium.org/2886453005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc (right): https://codereview.chromium.org/2886453005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc:471: NGLogicalOffset opp_offset = On 2017/05/15 22:57:42, kojii wrote: > What is "opp"? opportunity. Let me rename it to opportunity_offset.
https://codereview.chromium.org/2886453005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc (right): https://codereview.chromium.org/2886453005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc:134: border_and_padding_ = ComputeBorders(ConstraintSpace(), Style()) + On 2017/05/15 22:57:09, Gleb Lanbin wrote: > On 2017/05/15 22:53:41, ikilpatrick wrote: > > does it make sense for this to be in ctor? Initialize is sortof just for the > > Items() ? not sure. > > Koji, what do you think? Should I move this to the constructor? moved https://codereview.chromium.org/2886453005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc:471: NGLogicalOffset opp_offset = On 2017/05/15 22:59:12, Gleb Lanbin wrote: > On 2017/05/15 22:57:42, kojii wrote: > > What is "opp"? > > opportunity. Let me rename it to opportunity_offset. renamed
The CQ bit was checked by glebl@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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 ========== Account for border/padding in NG Inline layout algorithm. BUG=635619 TESTS=79 tests ========== to ========== Account for border/padding in NG Inline layout algorithm. BUG=635619 TESTS=79 tests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ==========
The CQ bit was checked by glebl@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 glebl@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_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by glebl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org, kojii@chromium.org Link to the patchset: https://codereview.chromium.org/2886453005/#ps60001 (title: "Skip some tests becase of crbug.com/713891")
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_layout_tests_layout_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Account for border/padding in NG Inline layout algorithm. BUG=635619 TESTS=79 tests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ========== to ========== Account for border/padding in NG Inline layout algorithm. BUG=635619 TESTS=79 tests ==========
The CQ bit was unchecked by glebl@chromium.org
The CQ bit was checked by glebl@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": 60001, "attempt_start_ts": 1494994817809240,
"parent_rev": "de633202221100c5bc812309bdff65f7faa6b0d3", "commit_rev":
"59bb4d543e23a248553affa3dc9de1e01a1ce228"}
Message was sent while issue was closed.
Description was changed from ========== Account for border/padding in NG Inline layout algorithm. BUG=635619 TESTS=79 tests ========== to ========== Account for border/padding in NG Inline layout algorithm. BUG=635619 TESTS=79 tests Review-Url: https://codereview.chromium.org/2886453005 Cr-Commit-Position: refs/heads/master@{#472323} Committed: https://chromium.googlesource.com/chromium/src/+/59bb4d543e23a248553affa3dc9d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/59bb4d543e23a248553affa3dc9d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
