Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1304953005:
Add some class documentation to LayoutBlock (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 class documentation to LayoutBlock
There was no explanation about LayoutBlock acting as the
concept of containing block. While at it, documented some
design decisions around the positioned descendant map.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201566
Patch Set 1 #
Total comments: 3
Patch Set 2 : Wrap better. #
Total comments: 4
Patch Set 3 : Updated after Morten's review. #
Total comments: 3
Patch Set 4 : Updated again after Morten's comments. #Patch Set 5 : Rebaselined change after outlinemap removal. #Messages
Total messages: 26 (9 generated)
jchaffraix@chromium.org changed reviewers: + eae@chromium.org, wkorman@chromium.org
+Emil for review bonanza! +Walter to foolproof the document as someone new-ish to the code.
LGTM w/nit https://codereview.chromium.org/1304953005/diff/1/Source/core/layout/LayoutBl... File Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1304953005/diff/1/Source/core/layout/LayoutBl... Source/core/layout/LayoutBlock.cpp:91: // This map keeps track of the positioned objects associated with a containing block. Could you please try to wrap this at 80 instead of 85 columns?
lgtm https://codereview.chromium.org/1304953005/diff/1/Source/core/layout/LayoutBl... File Source/core/layout/LayoutBlock.h (right): https://codereview.chromium.org/1304953005/diff/1/Source/core/layout/LayoutBl... Source/core/layout/LayoutBlock.h:58: // http://www.w3.org/TR/CSS2/visuren.html#containing-block What's the best spec to link to in general, for my knowledge for future similar docs? The most recent, or the most finalized/official-recommendation, or the first spec that was finalized that has the definition in question? Asking because here we link to the official recommendation spec, but for similar in prior change we linked to an "editor's draft" of css3 though we were describing a css2.1 notion of intrinsic size: https://drafts.csswg.org/css-sizing-3/#intrinsic and atop this it has a link to the "latest version" which is a "working draft": http://www.w3.org/TR/css3-sizing/
On 2015/08/28 22:01:17, wkorman wrote: > lgtm > > https://codereview.chromium.org/1304953005/diff/1/Source/core/layout/LayoutBl... > File Source/core/layout/LayoutBlock.h (right): > > https://codereview.chromium.org/1304953005/diff/1/Source/core/layout/LayoutBl... > Source/core/layout/LayoutBlock.h:58: // > http://www.w3.org/TR/CSS2/visuren.html#containing-block > What's the best spec to link to in general, for my knowledge for future similar > docs? The most recent, or the most finalized/official-recommendation, or the > first spec that was finalized that has the definition in question? Latest official one assuming the feature you are referring exists in there. If not link to the editors draft or whatever the most prominent version is/
On 2015/08/28 at 22:03:21, eae wrote: > On 2015/08/28 22:01:17, wkorman wrote: > > lgtm > > > > https://codereview.chromium.org/1304953005/diff/1/Source/core/layout/LayoutBl... > > File Source/core/layout/LayoutBlock.h (right): > > > > https://codereview.chromium.org/1304953005/diff/1/Source/core/layout/LayoutBl... > > Source/core/layout/LayoutBlock.h:58: // > > http://www.w3.org/TR/CSS2/visuren.html#containing-block > > What's the best spec to link to in general, for my knowledge for future similar > > docs? The most recent, or the most finalized/official-recommendation, or the > > first spec that was finalized that has the definition in question? > > Latest official one assuming the feature you are referring exists in there. If not link to the editors draft or whatever the most prominent version is/ That's actually a good question. My linking was based on the usefulness / relevance of the specification: - The definition of intrinsic was better in the draft version, thus I skipped CSS 2.1. - For the definition of 'block level', I am not aware of a more up-to-date specification. Now in general, we use the editor's draft for implementation (Tab can confirm that) so it would make sense to link to it in our code IMO. The reason is that it's the most up-to-date with specification fixes and addition. It seems like like we're suggesting different linking strategy, Emil :)
https://codereview.chromium.org/1304953005/diff/1/Source/core/layout/LayoutBl... File Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1304953005/diff/1/Source/core/layout/LayoutBl... Source/core/layout/LayoutBlock.cpp:91: // This map keeps track of the positioned objects associated with a containing block. On 2015/08/28 at 21:02:20, eae wrote: > Could you please try to wrap this at 80 instead of 85 columns? I wrapped the following text at 85 characters instead as I didn't see the inconsistency between the headers and the text.
mstensho@opera.com changed reviewers: + mstensho@opera.com
https://codereview.chromium.org/1304953005/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1304953005/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutBlock.cpp:93: // This map is cleared at the beginning of layout and populated during layout. The map It is populated during layout, but it's not cleared at the beginning of layout. How would we be able to add everything back then, without doing a full subtree layout? <div id="a" style="position:relative;"> <div id="b" style="width:100px;"> <div id="extra"> <div id="c" style="position:absolute;"> So #a has a map that contains #c. Now imagine that we change the "width" property of #a, which will trigger a relayout. If we cleared the map of positioned descendants of #a when starting layout of #a, we'd not add #c back into the map, since we may skip layout of #b's children, since #b doesn't change, having a fixed width. Note: To demonstrate my point, we need #extra because of the layoutChildren flag in the layoutFoo() methods. It will be set when laying out the the children of #a, so that we'll actually force layout out #b (quite unnecessarily so, as far as I can tell), and walk #b's children, but not necessarily lay them out. https://codereview.chromium.org/1304953005/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutBlock.h (right): https://codereview.chromium.org/1304953005/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutBlock.h:57: // that is a containing block. Close enough, I guess, but this isn't true for positioned inlines (LayoutInline), which also establish a containing block for absolutely positioned descendants.
https://codereview.chromium.org/1304953005/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1304953005/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutBlock.cpp:93: // This map is cleared at the beginning of layout and populated during layout. The map On 2015/08/31 at 12:00:45, mstensho wrote: > It is populated during layout, but it's not cleared at the beginning of layout. How would we be able to add everything back then, without doing a full subtree layout? > > <div id="a" style="position:relative;"> > <div id="b" style="width:100px;"> > <div id="extra"> > <div id="c" style="position:absolute;"> > > So #a has a map that contains #c. > > Now imagine that we change the "width" property of #a, which will trigger a relayout. If we cleared the map of positioned descendants of #a when starting layout of #a, we'd not add #c back into the map, since we may skip layout of #b's children, since #b doesn't change, having a fixed width. > > Note: To demonstrate my point, we need #extra because of the layoutChildren flag in the layoutFoo() methods. It will be set when laying out the the children of #a, so that we'll actually force layout out #b (quite unnecessarily so, as far as I can tell), and walk #b's children, but not necessarily lay them out. Indeed, my bad for not checking thoroughly. I have updated this line to be (emphasis mine): // This map is populated during layout. It is kept across layouts to handle sub-tree // layouts. [...] // it's easier to just *update* it for every layout. https://codereview.chromium.org/1304953005/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutBlock.h (right): https://codereview.chromium.org/1304953005/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutBlock.h:57: // that is a containing block. On 2015/08/31 at 12:00:45, mstensho wrote: > Close enough, I guess, but this isn't true for positioned inlines (LayoutInline), which also establish a containing block for absolutely positioned descendants. CSS makes me sad. They call positioned inlines a containing block as you rightly point out (see section 10.1.4) but it's not even a block. Added the following to clarify: // CSS is inconsistent and allows inline elements (LayoutInline) to be // containing blocks, even though they are not blocks. Our // implementation is as confused with inlines. See e.g. // LayoutObject::containingBlock() vs LayoutObject::container().
https://codereview.chromium.org/1304953005/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1304953005/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutBlock.cpp:94: // layouts. The map could be invalidated during style change but keeping track of You mean "handle that we skip unchanged subtrees during layout, in such a way that we're able to lay out deeply nested out-of-flow descendants if their containing block got laid out"? https://codereview.chromium.org/1304953005/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutBlock.h (right): https://codereview.chromium.org/1304953005/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutBlock.h:62: // CSS is inconsistent and allows inline elements (LayoutInline) to be A just description of the madness. :)
https://codereview.chromium.org/1304953005/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1304953005/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutBlock.cpp:94: // layouts. The map could be invalidated during style change but keeping track of On 2015/08/31 at 19:28:52, mstensho wrote: > You mean "handle that we skip unchanged subtrees during layout, in such a way that we're able to lay out deeply nested out-of-flow descendants if their containing block got laid out"? Me gusta! Changed for your less hand-wavy explanation.
lgtm
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, eae@chromium.org Link to the patchset: https://codereview.chromium.org/1304953005/#ps60001 (title: "Updated again after Morten's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1304953005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1304953005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by jchaffraix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com, eae@chromium.org, wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/1304953005/#ps80001 (title: "Rebaselined change after outlinemap removal.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1304953005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1304953005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jchaffraix@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1304953005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1304953005/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201566 |
