Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1327483003:
Add documentation to 2 DeprecatedPaintLayer objects (Closed)
|
Created:
5 years, 3 months ago by Julien - ping for review Modified:
5 years, 3 months ago CC:
blink-reviews, blink-reviews-paint_chromium.org, dshwang, slimming-paint-reviews_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdd documentation to 2 DeprecatedPaintLayer objects
Added class level comments to DeprecatedPaintLayerClipper
and DeprecatedPaintLayerStackingNode.
While touching DeprecatedPaintLayerStackingNode, added a comment
about NormalFlowChildren.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201805
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201815
Patch Set 1 #
Total comments: 16
Patch Set 2 : Rebaselined after all the review comments. #Patch Set 3 : Fixed the ASCII art to prevent multi-line comments. #
Messages
Total messages: 17 (4 generated)
jchaffraix@chromium.org changed reviewers: + eae@chromium.org, mstensho@opera.com, wkorman@chromium.org
Usual suspect for more class-level documentation!
https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... File Source/core/paint/DeprecatedPaintLayerClipper.h (right): https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerClipper.h:130: // DeprecatedPaintLayerClipper is responsible for caching clip rects. Worth adding a note for any class with 'Deprecated' in the name re: why it is considered deprecated and, if we have a sense, what we expect to eventually take its place? Would apply to all three classes in this change. If in some cases we've just renamed the classes so they're consistent with the DeprecatedPaintLayer naming then IMO still worth noting that, since otherwise a new eng seeing a class named like this will absolutely think they should avoid using the class and instead look for something else. https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerClipper.h:172: // clip #fixed. This is the reason why we compute the painting clip rects during The way this is written up makes it sound as if, if it weren't for this functionality requirement, we wouldn't cache these. But I'm expecting we'd want to cache them anyway so that paint phase has them available and doesn't need to recalculate them unnecessarily when we potentially have to repaint an area multiple times. True or false?
Nice! https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... File Source/core/paint/DeprecatedPaintLayerStackingNode.h (right): https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerStackingNode.h:77: // This class's purpose is to cache the z-order lists for painting and They're used to represent and not just cache the stacking contexts, right?
https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... File Source/core/paint/DeprecatedPaintLayerClipper.h (right): https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerClipper.h:130: // DeprecatedPaintLayerClipper is responsible for caching clip rects. Calculating and caching, perhaps? https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... File Source/core/paint/DeprecatedPaintLayerStackingNode.h (right): https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerStackingNode.h:68: // context. See CSS 2.1 appendix e for the actual algorithm Might want to capitalize that "e". https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerStackingNode.h:74: // this class to be a node in this tree but there are potential issues with "objects of this class to be nodes in this tree"? https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerStackingNode.h:80: // To implement any zorder list iterations, use "z-order" https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... File Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.h (right): https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.h:43: // are not treated as stacking contexts and thus we need to walk them I don't think "stacking contexts" is the right term here. What's the grown-up word to use for "positioned object and/or stacking context"? "Stacking node"? <div style="position:absolute;"> would be a stacking node, but not a stacking context. <div style="overflow:hidden;"> would be neither. <div style="opacity:0.5;"> would be a stacking node AND a stacking context, likewise for <div style="position:absolute; z-index:7;">.
https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... File Source/core/paint/DeprecatedPaintLayerClipper.h (right): https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerClipper.h:130: // DeprecatedPaintLayerClipper is responsible for caching clip rects. On 2015/09/01 at 17:58:20, wkorman wrote: > Worth adding a note for any class with 'Deprecated' in the name re: why it is considered deprecated and, if we have a sense, what we expect to eventually take its place? Would apply to all three classes in this change. I have added, that should clarify the status of the classes: // This class is NOT DEPRECATED, DeprecatedPaintLayer is and we match its // naming. Also modified DeprecatedPaintLayer to explain the rationale for deprecation: // The class is DEPRECATED, which means that we would like to remove it. The // reason for removal is that it has been a dumping ground for features for too // long and is the wrong level of abstraction, bearing no correspondence to any // CSS concept. Its associated objects and some of its feature need to be // migrated to LayoutObject (or the appropriate sub-class). > If in some cases we've just renamed the classes so they're consistent with the DeprecatedPaintLayer naming then IMO still worth noting that, since otherwise a new eng seeing a class named like this will absolutely think they should avoid using the class and instead look for something else. Makes sense! On 2015/09/01 at 18:59:06, mstensho wrote: > Calculating and caching, perhaps? Updated. https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerClipper.h:172: // clip #fixed. This is the reason why we compute the painting clip rects during On 2015/09/01 at 17:58:20, wkorman wrote: > The way this is written up makes it sound as if, if it weren't for this functionality requirement, we wouldn't cache these. But I'm expecting we'd want to cache them anyway so that paint phase has them available and doesn't need to recalculate them unnecessarily when we potentially have to repaint an area multiple times. True or false? I don't really have a definite answer to your question without data :-) The aim of this paragraph is to highlight the 2 different walks (layout vs paint tree), which motivates the need for caching. If we could do the computation during a paint tree walk, we may not need caching (by recomputing them on the fly) but it turns out that it's really hard to do that. As a counter argument to the multiple recomputation argument, we switched the normal flow list to be generated on the fly, potentially several times per paint and we didn't see any performance degradation (https://codereview.chromium.org/1188363002). This was motivated by a code simplification and less memory use. https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... File Source/core/paint/DeprecatedPaintLayerStackingNode.h (right): https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerStackingNode.h:68: // context. See CSS 2.1 appendix e for the actual algorithm On 2015/09/01 at 18:59:06, mstensho wrote: > Might want to capitalize that "e". D-O-N-E! https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerStackingNode.h:74: // this class to be a node in this tree but there are potential issues with On 2015/09/01 at 18:59:06, mstensho wrote: > "objects of this class to be nodes in this tree"? sgtm. https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerStackingNode.h:77: // This class's purpose is to cache the z-order lists for painting and On 2015/09/01 at 18:08:59, eae wrote: > They're used to represent and not just cache the stacking contexts, right? Good catch. Amended the description of the class to be better aligned with that: // This class's purpose is to represent a node in the stacking context tree // (aka paint tree). It currently caches the z-order lists for painting and // hit-testing. https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerStackingNode.h:80: // To implement any zorder list iterations, use On 2015/09/01 at 18:59:06, mstensho wrote: > "z-order" changed. https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... File Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.h (right): https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.h:43: // are not treated as stacking contexts and thus we need to walk them On 2015/09/01 at 18:59:06, mstensho wrote: > I don't think "stacking contexts" is the right term here. What's the grown-up word to use for "positioned object and/or stacking context"? "Stacking node"? <div style="position:absolute;"> would be a stacking node, but not a stacking context. <div style="overflow:hidden;"> would be neither. <div style="opacity:0.5;"> would be a stacking node AND a stacking context, likewise for <div style="position:absolute; z-index:7;">. The CSS word for stacking context + positioned is "treated as stacking context" (see the infamous appendix E). It does include floats too in CSS lingo but that's because we implement the painting algorithm in a different way.
lgtm https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... File Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.h (right): https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.h:43: // are not treated as stacking contexts and thus we need to walk them On 2015/09/03 14:25:54, Julien Chaffraix - PST wrote: > On 2015/09/01 at 18:59:06, mstensho wrote: > > I don't think "stacking contexts" is the right term here. What's the grown-up > word to use for "positioned object and/or stacking context"? "Stacking node"? > <div style="position:absolute;"> would be a stacking node, but not a stacking > context. <div style="overflow:hidden;"> would be neither. <div > style="opacity:0.5;"> would be a stacking node AND a stacking context, likewise > for <div style="position:absolute; z-index:7;">. > > The CSS word for stacking context + positioned is "treated as stacking context" > (see the infamous appendix E). It does include floats too in CSS lingo but > that's because we implement the painting algorithm in a different way. OK. Lacking a better word for a true stacking context (one that orders positioned descendants and true stacking context descendants), I can live with the current wording. I think of a (true) stacking context as something with applicable z-index, opacity or transform (anything else? Rhetorical question to my lazy self, who doesn't bother to check for other things). A stacking context paints its background, foreground and floats atomically (so that no other elements on the outside can interleave), and controls the paint order of descendant (true) stacking contexts AND positioned descendants. The "treated as stacking context" thing is only about painting background, foreground and floats atomically.
On 2015/09/04 at 13:22:32, mstensho wrote: > lgtm > > https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... > File Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.h (right): > > https://codereview.chromium.org/1327483003/diff/1/Source/core/paint/Deprecate... > Source/core/paint/DeprecatedPaintLayerStackingNodeIterator.h:43: // are not treated as stacking contexts and thus we need to walk them > On 2015/09/03 14:25:54, Julien Chaffraix - PST wrote: > > On 2015/09/01 at 18:59:06, mstensho wrote: > > > I don't think "stacking contexts" is the right term here. What's the grown-up > > word to use for "positioned object and/or stacking context"? "Stacking node"? > > <div style="position:absolute;"> would be a stacking node, but not a stacking > > context. <div style="overflow:hidden;"> would be neither. <div > > style="opacity:0.5;"> would be a stacking node AND a stacking context, likewise > > for <div style="position:absolute; z-index:7;">. > > > > The CSS word for stacking context + positioned is "treated as stacking context" > > (see the infamous appendix E). It does include floats too in CSS lingo but > > that's because we implement the painting algorithm in a different way. > > OK. Lacking a better word for a true stacking context (one that orders positioned descendants and true stacking context descendants), I can live with the current wording. > > I think of a (true) stacking context as something with applicable z-index, opacity or transform (anything else? Rhetorical question to my lazy self, who doesn't bother to check for other things). A stacking context paints its background, foreground and floats atomically (so that no other elements on the outside can interleave), and controls the paint order of descendant (true) stacking contexts AND positioned descendants. Actually a (true) stacking context is what CSS calls "a stacking context" and what we call "stacking context" in the code too (baring bad wording). > The "treated as stacking context" thing is only about painting background, foreground and floats atomically. Your definition is correct but we bundled both stacking context and "treated as stacking context" in the code (see ComputedStyle::isTreatedAsStackingContextForPainting()). The fact that we need to talk about this and seem to have a slightly divergent definition from CSS seems like I should add more details about that or clarify our current use.
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/1327483003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1327483003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201805
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1312203004/ by robertocn@chromium.org. The reason for reverting is: Speculative revert: Failed build job. I don't know why a comment would cause a breakage but the build output shows: In file included from ../../third_party/WebKit/Source/core/paint/DeprecatedPaintLayer.h:50:0, from ../../third_party/WebKit/Source/core/layout/svg/LayoutSVGModelObject.cpp:39: ../../third_party/WebKit/Source/core/paint/DeprecatedPaintLayerClipper.h:135:1:error: multi-line comment [-Werror=comment] // / \ ^.
Message was sent while issue was closed.
On 2015/09/04 at 17:47:50, robertocn wrote: > A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1312203004/ by robertocn@chromium.org. > > The reason for reverting is: Speculative revert: Failed build job. > > I don't know why a comment would cause a breakage but the build output shows: > > In file included from ../../third_party/WebKit/Source/core/paint/DeprecatedPaintLayer.h:50:0, > from ../../third_party/WebKit/Source/core/layout/svg/LayoutSVGModelObject.cpp:39: > ../../third_party/WebKit/Source/core/paint/DeprecatedPaintLayerClipper.h:135:1:error: multi-line comment [-Werror=comment] > // / \ > ^. Apparently Android doesn't like my ASCII art. Fixed and tested on a local build.
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 Link to the patchset: https://codereview.chromium.org/1327483003/#ps40001 (title: "Fixed the ASCII art to prevent multi-line comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1327483003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1327483003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201815 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
