Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2406843003:
[LayoutNG] Initial README.md file. (Closed)
|
Created:
4 years, 2 months ago by ikilpatrick Modified:
4 years, 2 months ago CC:
chromium-reviews, cbiesinger, ojan+watch_chromium.org, szager+layoutwatch_chromium.org, glebl+reviews_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] Initial README.md file.
Contains a very basic overview of the NGLayoutAlgorithm :).
BUG=635619
Committed: https://crrev.com/010709c620f2d8412ffd4cd11c545a074b01ea72
Cr-Commit-Position: refs/heads/master@{#424880}
Patch Set 1 #
Total comments: 14
Patch Set 2 : . #
Total comments: 2
Patch Set 3 : . #Messages
Total messages: 16 (5 generated)
ikilpatrick@chromium.org changed reviewers: + cbiesinger@chromium.org, eae@chromium.org, glebl@chromium.org
Wanted to do lots of small checkpoints, so others could jump in :).
usually google3 reviews with MD files include a link for preview. Should we follow the same practice here? Here is an example of similar chromium patch that has a preview link https://codereview.chromium.org/2218053003/#msg8 https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/README.md (right): https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/README.md:3: This directory contains the implementation of blink's new layout engine use Blink's instead of blink's? https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/README.md:20: - The [NGBox](ng_box.h) which we are currently performing layout for - the should "- the following" start from a new line? https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/README.md:42: TODO(layout-ng): Document with lots of pretty pictures. TODO should be followed by username, bug ID or email. layout-ng is not associated with anything. https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/README.md:48: ### Contstraint Spaces ### typo. Contstraint -> Constraint .nit should we drop "The" in titles to keep it simple? https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/README.md:57: ### Margin Collapsing ### it's named Collapsing Margins in https://www.w3.org/TR/CSS2/box.html#collapsing-margins
Perhaps we should reconsider and host these docs on chromium.org instead? That would also allow us to use images which is nice. https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/README.md (right): https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/README.md:6: This README can be viewed in formatted form [here](https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/core/layout/ng/README.md). I know I suggested it but I'd be very reluctant to include a link that relies on the current tooling and repository format. Given that these things have changes at least four times in the last two years I doubt this url will last for more than a year. Sigh. https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/README.md:8: The original design document can be seen [here](https://docs.google.com/document/d/1uxbDh4uONFQOiGuiumlJBLGgO4KDWB8ZEkp7Rd47fw4/edit). This URL doesn't work.
Not sure how that other patch did that for example? https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/README.md (right): https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/README.md:3: This directory contains the implementation of blink's new layout engine On 2016/10/10 23:07:55, glebl wrote: > use Blink's instead of blink's? Done. https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/README.md:6: This README can be viewed in formatted form [here](https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/core/layout/ng/README.md). On 2016/10/10 23:17:05, eae wrote: > I know I suggested it but I'd be very reluctant to include a link that relies on > the current tooling and repository format. Given that these things have changes > at least four times in the last two years I doubt this url will last for more > than a year. > > Sigh. :( Yeah, I like the bug you filed w/ infra. This really needs to be exposed by a maintained site. https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/README.md:8: The original design document can be seen [here](https://docs.google.com/document/d/1uxbDh4uONFQOiGuiumlJBLGgO4KDWB8ZEkp7Rd47fw4/edit). On 2016/10/10 23:17:05, eae wrote: > This URL doesn't work. did you copy the space in the review tool? https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/README.md:20: - The [NGBox](ng_box.h) which we are currently performing layout for - the On 2016/10/10 23:07:55, glebl wrote: > should "- the following" start from a new line? Acknowledged. Changed to sentence. https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/README.md:42: TODO(layout-ng): Document with lots of pretty pictures. On 2016/10/10 23:07:55, glebl wrote: > TODO should be followed by username, bug ID or email. layout-ng is not > associated with anything. Done. https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/README.md:48: ### Contstraint Spaces ### On 2016/10/10 23:07:55, glebl wrote: > typo. Contstraint -> Constraint > > .nit should we drop "The" in titles to keep it simple? > Done. https://codereview.chromium.org/2406843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/README.md:57: ### Margin Collapsing ### On 2016/10/10 23:07:55, glebl wrote: > it's named Collapsing Margins in > https://www.w3.org/TR/CSS2/box.html#collapsing-margins Done.
lgtm
lgtm https://codereview.chromium.org/2406843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/README.md (right): https://codereview.chromium.org/2406843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/README.md:48: ### Contstraint Spaces ### Constraint
https://codereview.chromium.org/2406843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/README.md (right): https://codereview.chromium.org/2406843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/README.md:48: ### Contstraint Spaces ### On 2016/10/12 19:59:02, cbiesinger wrote: > Constraint Done, thanks :)
The CQ bit was checked by ikilpatrick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, glebl@chromium.org Link to the patchset: https://codereview.chromium.org/2406843003/#ps40001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [LayoutNG] Initial README.md file. Contains a very basic overview of the NGLayoutAlgorithm :). BUG=635619 ========== to ========== [LayoutNG] Initial README.md file. Contains a very basic overview of the NGLayoutAlgorithm :). BUG=635619 Committed: https://crrev.com/010709c620f2d8412ffd4cd11c545a074b01ea72 Cr-Commit-Position: refs/heads/master@{#424880} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/010709c620f2d8412ffd4cd11c545a074b01ea72 Cr-Commit-Position: refs/heads/master@{#424880}
Message was sent while issue was closed.
Patchset #4 (id:60001) has been deleted |
