Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2755733003:
Implement relative utilities (Closed)
|
Created:
3 years, 9 months ago by atotic Modified:
3 years, 9 months ago CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, 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. |
DescriptionImplement relative utility routines
These utilities will be used to compute relative offset in layoutng.
BUG=635619
Review-Url: https://codereview.chromium.org/2755733003
Cr-Commit-Position: refs/heads/master@{#458590}
Committed: https://chromium.googlesource.com/chromium/src/+/1af111915f36842841682091b8c6882cae410831
Patch Set 1 #
Total comments: 8
Patch Set 2 : CR fixes #
Total comments: 8
Patch Set 3 : CR fixes #Patch Set 4 : rebase #
Messages
Total messages: 26 (11 generated)
Description was changed from ========== Implement relative utils BUG=635619 ========== to ========== Implement relative utility routines These utilities will be used to compute relative offset in layoutng. BUG=635619 ==========
atotic@chromium.org changed reviewers: + ikilpatrick@chromium.org
https://codereview.chromium.org/2755733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_relative_utils.cc (right): https://codereview.chromium.org/2755733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_relative_utils.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2755733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_relative_utils.cc:27: left = valueForLength(child_style.left(), container_size.inline_size); would it be easier to just convert the container_size to physical and not have the above if (is_horizontal) ? https://codereview.chromium.org/2755733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_relative_utils.h (right): https://codereview.chromium.org/2755733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_relative_utils.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2755733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_relative_utils_test.cc (right): https://codereview.chromium.org/2755733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_relative_utils_test.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017
ptal https://codereview.chromium.org/2755733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_relative_utils.cc (right): https://codereview.chromium.org/2755733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_relative_utils.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/16 at 19:08:36, ikilpatrick wrote: > 2017 done https://codereview.chromium.org/2755733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_relative_utils.cc:27: left = valueForLength(child_style.left(), container_size.inline_size); On 2017/03/16 at 19:08:36, ikilpatrick wrote: > would it be easier to just convert the container_size to physical and not have the above if (is_horizontal) ? done. Thanks. https://codereview.chromium.org/2755733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_relative_utils.h (right): https://codereview.chromium.org/2755733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_relative_utils.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/16 at 19:08:36, ikilpatrick wrote: > 2017 done https://codereview.chromium.org/2755733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_relative_utils_test.cc (right): https://codereview.chromium.org/2755733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_relative_utils_test.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/16 at 19:08:36, ikilpatrick wrote: > 2017 done
Would it be easier to take a ConstraintSpace instead of writing mode and length?
On 2017/03/20 at 19:17:48, cbiesinger wrote: > Would it be easier to take a ConstraintSpace instead of writing mode and length? No. In my full patch, this code is getting called from ng_fragment_builder, which does not have a constraint space.
On 2017/03/20 at 19:21:25, atotic wrote: > On 2017/03/20 at 19:17:48, cbiesinger wrote: > > Would it be easier to take a ConstraintSpace instead of writing mode and length? > > No. In my full patch, this code is getting called from ng_fragment_builder, which > does not have a constraint space. ptal? 24hrs
https://codereview.chromium.org/2755733003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_relative_utils.cc (right): https://codereview.chromium.org/2755733003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_relative_utils.cc:15: // Returns child's relative position wrt containing fragment. "Returns the child's relative position wrt the containing fragment." https://codereview.chromium.org/2755733003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_relative_utils.h (right): https://codereview.chromium.org/2755733003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_relative_utils.h:19: // Return relative position as defined by style. "Returns the relative position offset as defined by style." https://codereview.chromium.org/2755733003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_relative_utils_test.cc (right): https://codereview.chromium.org/2755733003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_relative_utils_test.cc:14: //#define NGAuto LayoutUnit(-1) ? https://codereview.chromium.org/2755733003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_relative_utils_test.cc:59: const LayoutUnit NGRelativeUtilsTest::kZero{0}; these are all in the anon namespace? they can probably just be: static const LayoutUnit kTop(7); at the top - i.e. not in the class ctor? no need to initialize them here?
ptal https://codereview.chromium.org/2755733003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_relative_utils.cc (right): https://codereview.chromium.org/2755733003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_relative_utils.cc:15: // Returns child's relative position wrt containing fragment. On 2017/03/21 at 17:51:17, ikilpatrick wrote: > "Returns the child's relative position wrt the containing fragment." done. https://codereview.chromium.org/2755733003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_relative_utils.h (right): https://codereview.chromium.org/2755733003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_relative_utils.h:19: // Return relative position as defined by style. On 2017/03/21 at 17:51:17, ikilpatrick wrote: > "Returns the relative position offset as defined by style." done https://codereview.chromium.org/2755733003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_relative_utils_test.cc (right): https://codereview.chromium.org/2755733003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_relative_utils_test.cc:14: //#define NGAuto LayoutUnit(-1) On 2017/03/21 at 17:51:17, ikilpatrick wrote: > ? gone https://codereview.chromium.org/2755733003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_relative_utils_test.cc:59: const LayoutUnit NGRelativeUtilsTest::kZero{0}; On 2017/03/21 at 17:51:17, ikilpatrick wrote: > these are all in the anon namespace? they can probably just be: > > static const LayoutUnit kTop(7); > > at the top - i.e. not in the class ctor? no need to initialize them here? done I was worried that this might be prohibited by coding guidelines, but it turns out that LayoutUnit is a PODType.
lgtm
The CQ bit was checked by atotic@chromium.org
The CQ bit was unchecked by atotic@chromium.org
atotic@chromium.org changed reviewers: + eae@chromium.org
On 2017/03/21 at 19:46:24, ikilpatrick wrote: > lgtm @eae, can you lgtm BUILD.gn files?
RS LGTM
The CQ bit was checked by atotic@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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 atotic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/2755733003/#ps60001 (title: "rebase")
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": 1490129186239670,
"parent_rev": "23d735af2e1eeb5af3b0cc919f57189c4217a89e", "commit_rev":
"1af111915f36842841682091b8c6882cae410831"}
Message was sent while issue was closed.
Description was changed from ========== Implement relative utility routines These utilities will be used to compute relative offset in layoutng. BUG=635619 ========== to ========== Implement relative utility routines These utilities will be used to compute relative offset in layoutng. BUG=635619 Review-Url: https://codereview.chromium.org/2755733003 Cr-Commit-Position: refs/heads/master@{#458590} Committed: https://chromium.googlesource.com/chromium/src/+/1af111915f36842841682091b8c6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/1af111915f36842841682091b8c6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
