Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1307653004:
Add some better documentation to LayoutObject (Closed)
|
Created:
5 years, 3 months ago by Julien - ping for review Modified:
5 years, 3 months ago CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/remotes/origin/master Project:
blink Visibility:
Public. |
DescriptionAdd some better documentation to LayoutObject
The current documentation was short and didn't really
explain much about the object. The expanded description
introduces more concepts and talks about the different
layouts in more details.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201368
Patch Set 1 #
Total comments: 22
Patch Set 2 : Addressed the review comments. #
Total comments: 2
Patch Set 3 : Updated after nit. #Patch Set 4 : Fixed compilation after moving some boolean fields. #
Messages
Total messages: 19 (7 generated)
jchaffraix@chromium.org changed reviewers: + pdr@chromium.org, skobes@chromium.org, wkorman@chromium.org
+Steve / Walter for header documentation bonanza. +Philip to check if we should add something for SVG (or if what I wrote clashes with SVG).
Thanks for doing this :) https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:121: // Some LayoutObjects don't have an associated Node and are called "anonymous" (see the constructor Also mention that some Nodes don't have an associated LayoutObject (e.g. display:none). https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:130: // Layout is the process of sizing and positioning Nodes on the page. In Blink, layouts always start Shouldn't that be "sizing and positioning LayoutObjects"? https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:134: // Due to the hight cost of layout, a lot of effort is done to avoid doing full layouts of nodes. typo: "high cost" https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:1382: // Intrinsic means that they depend purely on the content, not on the style This seems wrong, some styles certainly affect the intrinsic size (e.g. font-size). Maybe this should say "depend purely on the content, not on the space available from the container"?
<3 <3 <3 <3 i love this so much. SVG parts look great. https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:119: // It may be useful to add a blurb about the layout tree being rooted at a LayoutView which roughly corresponds to the Document node. https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:1356: // layout (but this object doesn't). Due to the nature of CSS, relaying out a child can cause Nit: "relaying" was a little confusing to me. Maybe: Laying out a child can cause the parent to resize (e.g., if 'height' is auto).
Awesome! LGTM pending replies to other feedback. https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:1373: // Setting this flag only recompute the overflow information. recomputes https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:1378: // bleed into its containing block's so we have to recompute it in some case. cases
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org
https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:1342: // This is the default layout but it is expensive as it recomputes everything. It won't, necessarily, re-layout children if they aren't marked thought, correct? https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:1367: // The implementation of this layout is in Simplified LayoutBlock::simplifiedNormalFlowLayout. nit: Is that Simplified supposed to be there?
Updated the change after the awesome. Steve, I did some changes that you would want to double-check before I land. https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutBox.h File Source/core/layout/LayoutBox.h (right): https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutBo... Source/core/layout/LayoutBox.h:852: // CSS 2.1 calls this width the 'preferred minimum width'. Added minimum content width to this that I missed. Also broke the text to have a tighter paragraph. https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutBo... Source/core/layout/LayoutBox.h:857: // CSS 2.1 calls this width the 'preferred width'. Added "maximum cell width" to this that I missed. https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:119: // On 2015/08/27 at 04:09:48, pdr wrote: > It may be useful to add a blurb about the layout tree being rooted at a LayoutView which roughly corresponds to the Document node. Added: // The root of the LayoutObject tree is the LayoutView, which is the LayoutObject associated // with the Document. https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:121: // Some LayoutObjects don't have an associated Node and are called "anonymous" (see the constructor On 2015/08/27 at 00:55:03, skobes wrote: > Also mention that some Nodes don't have an associated LayoutObject (e.g. display:none). Added: // Also some Node don't have an associated LayoutObjects e.g. if display: none is set. For more // detail, see LayoutObject::createObject that creates the right LayoutObject based on the style. https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:130: // Layout is the process of sizing and positioning Nodes on the page. In Blink, layouts always start On 2015/08/27 at 00:55:03, skobes wrote: > Shouldn't that be "sizing and positioning LayoutObjects"? Conceptually either phrasing works and I don't feel strongly about either. I thought about layout in terms of what the author writes (html, which is translated into the DOM). The Layout tree is just our internal structure to handle laying out and is an implementation detail from this perspective. https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:134: // Due to the hight cost of layout, a lot of effort is done to avoid doing full layouts of nodes. On 2015/08/27 at 00:55:03, skobes wrote: > typo: "high cost" Corrected. https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:1342: // This is the default layout but it is expensive as it recomputes everything. On 2015/08/27 at 14:09:24, dsinclair wrote: > It won't, necessarily, re-layout children if they aren't marked thought, correct? That's not correct. I added a comment to LayoutObject::layout, which should answer your question: // By default, layout only lays out the children that are marked for layout. // In some cases, layout has to force laying out more children. An example is // when the width of the LayoutObject changes as this impacts children with // 'width' set to auto. https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:1356: // layout (but this object doesn't). Due to the nature of CSS, relaying out a child can cause On 2015/08/27 at 04:09:48, pdr wrote: > Nit: "relaying" was a little confusing to me. > > Maybe: Laying out a child can cause the parent to resize (e.g., if 'height' is auto). Applied your suggestion. https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:1367: // The implementation of this layout is in Simplified LayoutBlock::simplifiedNormalFlowLayout. On 2015/08/27 at 14:09:24, dsinclair wrote: > nit: Is that Simplified supposed to be there? Hell no! Removed. https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:1373: // Setting this flag only recompute the overflow information. On 2015/08/27 at 05:02:30, wkorman wrote: > recomputes Done. https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:1378: // bleed into its containing block's so we have to recompute it in some case. On 2015/08/27 at 05:02:30, wkorman wrote: > cases Done. https://codereview.chromium.org/1307653004/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:1382: // Intrinsic means that they depend purely on the content, not on the style On 2015/08/27 at 00:55:03, skobes wrote: > This seems wrong, some styles certainly affect the intrinsic size (e.g. font-size). Maybe this should say "depend purely on the content, not on the space available from the container"? You're right that it's not accurate. Only referring to the available space is too restrictive IMO as it seems to oppose it to the normal width that includes it (but also the layout algorithm selected with 'display'). CSS uses the following definition but it seems hand-wavy too: "Intrinsic sizing determines sizes based on the contents of an element, without regard for its context." I improved the original text by changing it to: // Intrinsic sizes depend mostly on the content and a limited set of style // properties (e.g. any font-related property for text, 'min-width'/'max-width'). // // Those widths are used to determine the final layout logical width, which // depends on the layout algorithm used and the available logical width.
lgtm https://codereview.chromium.org/1307653004/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1307653004/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutObject.h:1400: // m_maxPreferredLogicalWidth). They are lazily computed during layout. Nit: sometimes preferred logical widths are computed outside of layout, e.g. WebView::contentsPreferredMinimumSize().
https://codereview.chromium.org/1307653004/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1307653004/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutObject.h:1400: // m_maxPreferredLogicalWidth). They are lazily computed during layout. On 2015/08/27 at 21:19:04, skobes wrote: > Nit: sometimes preferred logical widths are computed outside of layout, e.g. WebView::contentsPreferredMinimumSize(). Updated. Thanks!
The CQ bit was checked by jchaffraix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org, skobes@chromium.org Link to the patchset: https://codereview.chromium.org/1307653004/#ps40001 (title: "Updated after nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1307653004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1307653004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jchaffraix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org, wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/1307653004/#ps60001 (title: "Fixed compilation after moving some boolean fields.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1307653004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1307653004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201368 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
