Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2380933004:
Update layout documentation re: coordinate systems and writing mode. (Closed)
|
Created:
4 years, 2 months ago by wkorman Modified:
4 years, 2 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, kojii, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, Xianzhu, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate layout documentation re: coordinate systems and writing mode.
BUG=634143
Committed: https://crrev.com/01cc28757550adcf3eb436bab40ad67f251530c5
Cr-Commit-Position: refs/heads/master@{#422298}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Restore class header comment. #
Total comments: 4
Patch Set 3 : Add relpos example. #Patch Set 4 : Sync to head. #
Messages
Total messages: 19 (7 generated)
wkorman@chromium.org changed reviewers: + chrishtr@chromium.org, eae@chromium.org
First pass at enhancing geometry/writing-mode Layout documentation, feedback welcome. I'm not crazy about drawing ASCII art so while I've kept the one diagram we have, I've linked to a couple of demos and specs rather than adding more. I considered elaborating on the phrasing wangxianzhu@ uses in http://crbug.com/650417#c3 re: "local physical coordinate space" and "flipped physical coordinate space", but I think those are just more specific ways to refer to things within particular cases, and this document is higher level. I also didn't want to deconstruct the implementation of the cited methods too much. The code can be that level of documentation. I was leery of referencing methods explicitly due to risk they're renamed and docs become stale, but trading off that with the benefit of having some examples, I included core primitive-type methods, and a few feature-specific versions.
https://codereview.chromium.org/2380933004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h (right): https://codereview.chromium.org/2380933004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h:90: // Source/core/layout/README.md ### Coordinate Spaces. Thank you for updating the README file, please leave a summary of the coordinate system description in here (as a comment) though as everyone who looks at this code will immediately see that. The overhead of looking at a raw markdown file or finding a tool to translate it into HTML is much too high for it to replace a well throughout inline description.
https://codereview.chromium.org/2380933004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h (right): https://codereview.chromium.org/2380933004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h:90: // Source/core/layout/README.md ### Coordinate Spaces. On 2016/09/29 23:06:08, eae wrote: > Thank you for updating the README file, please leave a summary of the coordinate > system description in here (as a comment) though as everyone who looks at this > code will immediately see that. The overhead of looking at a raw markdown file > or finding a tool to translate it into HTML is much too high for it to replace a > well throughout inline description. Done.
Thank you LGTM
https://codereview.chromium.org/2380933004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/README.md (right): https://codereview.chromium.org/2380933004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/README.md:111: across the center of a block's containing block. Please add an explicit example of writing mode with multiple nested containing blocks, that demonstrates how it's required to flip recursively across all of these containing blocks in order to generate physical pixels in screen space. https://codereview.chromium.org/2380933004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/README.md:180: * `LayoutObject::container()` returns the containing block for an element as Could you just rename these methods to match their definition? Should be an easy change.
https://codereview.chromium.org/2380933004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/README.md (right): https://codereview.chromium.org/2380933004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/README.md:111: across the center of a block's containing block. On 2016/09/30 16:47:35, chrishtr wrote: > Please add an explicit example of writing mode with multiple nested containing > blocks, that > demonstrates how it's required to flip recursively across all of these > containing blocks in > order to generate physical pixels in screen space. Done. If I've missed something with the example let's discuss, but I believe the simple case is sufficient to illustrate as we need to flip materially within both 'container' and the overall view. This case was broken in M53 and is now working in M54+. We could continue illustrating with more complex examples but they become increasingly challenging to document and explain. I linked to a public-viewable Google Drawing as I don't believe ASCII art is worth the massive time investment. https://codereview.chromium.org/2380933004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/README.md:180: * `LayoutObject::container()` returns the containing block for an element as On 2016/09/30 16:47:35, chrishtr wrote: > Could you just rename these methods to match their definition? Should be an easy > change. I'll file a bug to do separately as I'd like to keep this documentation only, and while the rename will be mechanical, there are probably a range of comments and local vars we may want to consider renaming as well. I can also imagine some bikeshedding on best names.
lgtm Awesome!
The CQ bit was checked by wkorman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/2380933004/#ps40001 (title: "Add relpos example.")
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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wkorman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/2380933004/#ps60001 (title: "Sync to head.")
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Update layout documentation re: coordinate systems and writing mode. BUG=634143 ========== to ========== Update layout documentation re: coordinate systems and writing mode. BUG=634143 Committed: https://crrev.com/01cc28757550adcf3eb436bab40ad67f251530c5 Cr-Commit-Position: refs/heads/master@{#422298} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/01cc28757550adcf3eb436bab40ad67f251530c5 Cr-Commit-Position: refs/heads/master@{#422298} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
