Update layout documentation re: coordinate systems and writing mode. (issue 2380933004 by wkorman@chromium.org)

0 views
Skip to first unread message

wko...@chromium.org

unread,
Sep 29, 2016, 6:28:32 PM9/29/16
to e...@chromium.org, chri...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, wangx...@chromium.org, ko...@chromium.org
Reviewers: eae, chrishtr
CL: https://codereview.chromium.org/2380933004/

Message:
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.

Description:
Update layout documentation re: coordinate systems and writing mode.

BUG=634143

Affected files (+226, -100 lines):
M third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h
M third_party/WebKit/Source/core/layout/README.md


e...@chromium.org

unread,
Sep 29, 2016, 7:06:09 PM9/29/16
to wko...@chromium.org, chri...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, wangx...@chromium.org, ko...@chromium.org

https://codereview.chromium.org/2380933004/diff/1/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h
(right):

https://codereview.chromium.org/2380933004/diff/1/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h#newcode90
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/

wko...@chromium.org

unread,
Sep 29, 2016, 7:24:31 PM9/29/16
to e...@chromium.org, chri...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, wangx...@chromium.org, ko...@chromium.org

https://codereview.chromium.org/2380933004/diff/1/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h
(right):

https://codereview.chromium.org/2380933004/diff/1/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h#newcode90
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.

e...@chromium.org

unread,
Sep 30, 2016, 11:33:12 AM9/30/16
to wko...@chromium.org, chri...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, wangx...@chromium.org, ko...@chromium.org

chri...@chromium.org

unread,
Sep 30, 2016, 12:47:35 PM9/30/16
to wko...@chromium.org, e...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, ko...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, wangx...@chromium.org, zol...@webkit.org

https://codereview.chromium.org/2380933004/diff/20001/third_party/WebKit/Source/core/layout/README.md
File third_party/WebKit/Source/core/layout/README.md (right):

https://codereview.chromium.org/2380933004/diff/20001/third_party/WebKit/Source/core/layout/README.md#newcode111
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/Source/core/layout/README.md#newcode180
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/

wko...@chromium.org

unread,
Sep 30, 2016, 6:29:08 PM9/30/16
to chri...@chromium.org, e...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, ko...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, wangx...@chromium.org, zol...@webkit.org

https://codereview.chromium.org/2380933004/diff/20001/third_party/WebKit/Source/core/layout/README.md
File third_party/WebKit/Source/core/layout/README.md (right):

https://codereview.chromium.org/2380933004/diff/20001/third_party/WebKit/Source/core/layout/README.md#newcode111
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/Source/core/layout/README.md#newcode180
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.

https://codereview.chromium.org/2380933004/

chri...@chromium.org

unread,
Sep 30, 2016, 6:33:52 PM9/30/16
to wko...@chromium.org, e...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, ko...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, wangx...@chromium.org, zol...@webkit.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 6:37:05 PM9/30/16
to wko...@chromium.org, chri...@chromium.org, e...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, ko...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, wangx...@chromium.org, zol...@webkit.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 8:26:44 PM9/30/16
to wko...@chromium.org, chri...@chromium.org, e...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, ko...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, wangx...@chromium.org, zol...@webkit.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_rel_ng/builds/307823)

https://codereview.chromium.org/2380933004/

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 11:52:06 PM9/30/16
to wko...@chromium.org, chri...@chromium.org, e...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, ko...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, wangx...@chromium.org, zol...@webkit.org
CQ is trying da patch. Follow status at

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 1, 2016, 12:55:28 AM10/1/16
to wko...@chromium.org, chri...@chromium.org, e...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, ko...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, wangx...@chromium.org, zol...@webkit.org
Committed patchset #4 (id:60001)

https://codereview.chromium.org/2380933004/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 1, 2016, 12:57:22 AM10/1/16
to wko...@chromium.org, chri...@chromium.org, e...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, ko...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, wangx...@chromium.org, zol...@webkit.org
Patchset 4 (id:??) landed as
https://crrev.com/01cc28757550adcf3eb436bab40ad67f251530c5
Cr-Commit-Position: refs/heads/master@{#422298}

https://codereview.chromium.org/2380933004/
Reply all
Reply to author
Forward
0 new messages