Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13467028:
Add an intermediate function to generate 2 paint-order lists. (Closed)
|
Created:
7 years, 8 months ago by hartmanng Modified:
7 years, 8 months ago CC:
blink-reviews, jchaffraix+rendering Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionAdd an intermediate function to generate 2 paint-order lists.
The first represents the list before opting in to composited scrolling, and the
second one shows what the state would be if we decided to opt in. These two
lists will be necessary for our decision of whether or not to opt in, because we
have to verify that this decision will not change the paint order. This also
includes some plumbing and a layout test to ensure that the paint-order lists
generated in this function conform to the stacking-order as verified by existing
hit-testing code.
R=jchaffraix
BUG=222016
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149028
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 15
Patch Set 3 : Responding to Ian's comments #
Total comments: 12
Patch Set 4 : Shawn's review #Patch Set 5 : Removed idl interface #Patch Set 6 : fix rebase mistake #
Total comments: 11
Patch Set 7 : Addressing Julien's comments #Patch Set 8 : minor fix - remove duplicate method #
Total comments: 8
Patch Set 9 : addressing review comments #Messages
Total messages: 31 (0 generated)
Ian and Shawn, please pre-review this patch before I send it to jchaffraix and enne. Also, Ian, did you mention someone I could ask about the copyright headers for the new files?
https://codereview.chromium.org/13467028/diff/1/Source/WebCore/dom/PaintOrder... File Source/WebCore/dom/PaintOrderLists.idl (right): https://codereview.chromium.org/13467028/diff/1/Source/WebCore/dom/PaintOrder... Source/WebCore/dom/PaintOrderLists.idl:4: ] interface PaintOrderLists { Is this for extensions? Or is this for the web platform? Do we have a spec for this?
https://codereview.chromium.org/13467028/diff/1/Source/WebCore/dom/PaintOrder... File Source/WebCore/dom/PaintOrderLists.idl (right): https://codereview.chromium.org/13467028/diff/1/Source/WebCore/dom/PaintOrder... Source/WebCore/dom/PaintOrderLists.idl:4: ] interface PaintOrderLists { On 2013/04/09 07:20:34, Eric Seidel (Google) wrote: > Is this for extensions? Or is this for the web platform? Do we have a spec for > this? My intent was just to plumb this through for use in Internals in a layout test, I didn't realize this would expose it completely. Is there a way to make this available only for Internals?
I'm not an internals expert. Adding an idl file doesn't necessarily mean that it's exposed to the Web, but it's in a gray-area, where that can be confusing. CC'd dglazkov who likely has more context in this case.
In the meantime, I had a few other comments Maybe I'm misunderstanding why some parts of the patch are needed, but it seems like they can be separated, which would be a good idea. I'll continue to review after hearing back about these =) https://codereview.chromium.org/13467028/diff/1/Source/WebCore/rendering/Rend... File Source/WebCore/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/13467028/diff/1/Source/WebCore/rendering/Rend... Source/WebCore/rendering/RenderLayer.cpp:2226: updateIsNormalFlowOnly(); I'm under the impression this is a drive-by bugfix, if I'm understanding things correctly? Would it make more sense to do this as a separate much simpler patch, with an additional layout test showing why this is needed? https://codereview.chromium.org/13467028/diff/1/Source/WebCore/rendering/Rend... Source/WebCore/rendering/RenderLayer.cpp:6096: if (isStackingContainer()) Same here - it seems to me this is unrelated to the exact purpose of this patch, and can be done in a separate trivial patch? The reason for adding this is not clear, but having it in a separate patch would probably make it obvious and simple for reviewing
On 2013/04/09 20:16:07, shawnsingh wrote: > In the meantime, I had a few other comments Maybe I'm misunderstanding why some > parts of the patch are needed, but it seems like they can be separated, which > would be a good idea. I'll continue to review after hearing back about these > =) > > https://codereview.chromium.org/13467028/diff/1/Source/WebCore/rendering/Rend... > File Source/WebCore/rendering/RenderLayer.cpp (right): > > https://codereview.chromium.org/13467028/diff/1/Source/WebCore/rendering/Rend... > Source/WebCore/rendering/RenderLayer.cpp:2226: updateIsNormalFlowOnly(); > I'm under the impression this is a drive-by bugfix, if I'm understanding things > correctly? Would it make more sense to do this as a separate much simpler > patch, with an additional layout test showing why this is needed? > > https://codereview.chromium.org/13467028/diff/1/Source/WebCore/rendering/Rend... > Source/WebCore/rendering/RenderLayer.cpp:6096: if (isStackingContainer()) > Same here - it seems to me this is unrelated to the exact purpose of this patch, > and can be done in a separate trivial patch? The reason for adding this is not > clear, but having it in a separate patch would probably make it obvious and > simple for reviewing You're right, I removed it from this patch. I think Ian is actually already planning on fixing this elsewhere (crbug.com/222015)
https://codereview.chromium.org/13467028/diff/15001/LayoutTests/compositing/o... File LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html (right): https://codereview.chromium.org/13467028/diff/15001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html:2: <style type="text/css" media="screen"> Ditch the unnecessary attributes. https://codereview.chromium.org/13467028/diff/15001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html:36: <script type="text/javascript" charset="utf-8"> Ditto. https://codereview.chromium.org/13467028/diff/15001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html:171: // below, when we set webkitTransform to '', we want that to force a style recalculation. Rather than comments like 'force to promote' and 'force not to promote', perhaps we should have a short paragraph explaining what the point of all this is. Something like 'We want to compare the pre/post promotion paint order lists, so we...' https://codereview.chromium.org/13467028/diff/15001/LayoutTests/compositing/o... File LayoutTests/compositing/overflow/build-paint-order-lists.html (right): https://codereview.chromium.org/13467028/diff/15001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-lists.html:114: if (stackingOrder[i] == paintOrder[paintOrder.length - j - 1]) This seems overly cryptic. Why not bail at the first mismatch and forgo the j index? https://codereview.chromium.org/13467028/diff/15001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-lists.html:124: var container = document.getElementById('container'); Is it possible to share some of this duplicated code with the previous test? https://codereview.chromium.org/13467028/diff/15001/Source/WebCore/rendering/... File Source/WebCore/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/13467028/diff/15001/Source/WebCore/rendering/... Source/WebCore/rendering/RenderLayer.cpp:682: // instead. Let's just fix this rather than putting in a FIXME. It's definitely the right thing to do. https://codereview.chromium.org/13467028/diff/15001/Source/WebCore/rendering/... File Source/WebCore/rendering/RenderLayer.h (right): https://codereview.chromium.org/13467028/diff/15001/Source/WebCore/rendering/... Source/WebCore/rendering/RenderLayer.h:863: void collectBeforeAfterPromotionZOrderLists(RenderLayer* ancestorStackingContext, Please combine these two methods. They're hard to distinguish by their names, and looking at the .cpp, they could be merged. (And use the first, more descriptive, method name).
I've responded to all the comments, and added a few other fixes I've gotten from other reviews (using === instead of ==, etc). I also rebased this patch on https://codereview.chromium.org/13913013/, since it now relies on that for the updateIsNormalFlowOnly() fix. https://codereview.chromium.org/13467028/diff/15001/LayoutTests/compositing/o... File LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html (right): https://codereview.chromium.org/13467028/diff/15001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html:2: <style type="text/css" media="screen"> On 2013/04/16 19:17:41, vollick wrote: > Ditch the unnecessary attributes. Done. https://codereview.chromium.org/13467028/diff/15001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html:36: <script type="text/javascript" charset="utf-8"> On 2013/04/16 19:17:41, vollick wrote: > Ditto. Done. https://codereview.chromium.org/13467028/diff/15001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html:171: // below, when we set webkitTransform to '', we want that to force a style recalculation. On 2013/04/16 19:17:41, vollick wrote: > Rather than comments like 'force to promote' and 'force not to promote', perhaps > we should have a short paragraph explaining what the point of all this is. > Something like 'We want to compare the pre/post promotion paint order lists, so > we...' Done. https://codereview.chromium.org/13467028/diff/15001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html:219: if (shouldOptIn != didOptIn()) { This part must have been left in from before. The purpose of this test is to make sure the _lists_ generated are correct, it should _not_ care about the actual final opt-in decision (in particular, because this isn't actually hooked up to the paint order lists yet). Removed. https://codereview.chromium.org/13467028/diff/15001/LayoutTests/compositing/o... File LayoutTests/compositing/overflow/build-paint-order-lists.html (right): https://codereview.chromium.org/13467028/diff/15001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-lists.html:114: if (stackingOrder[i] == paintOrder[paintOrder.length - j - 1]) On 2013/04/16 19:17:41, vollick wrote: > This seems overly cryptic. Why not bail at the first mismatch and forgo the j > index? Yeah, it is a bit cryptic, but we expect the stacking order list to contain more than the paint order list sometimes because after we promote, the container's children won't appear in the stacking context's ancestor's lists anymore. They'll still be in the stacking order list. The important part is that the _order_ of the things present in the paint order list is preserved in the stacking order list. Also added as a comment in the code to explain. https://codereview.chromium.org/13467028/diff/15001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-lists.html:124: var container = document.getElementById('container'); On 2013/04/16 19:17:41, vollick wrote: > Is it possible to share some of this duplicated code with the previous test? Yeah, we can definitely move 2 or 3 of these helper functions out into a common .js file if we want. Based on https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove..., though, I got the feeling that layout tests are generally expected to be self-containing. These helper functions are mostly pretty trivial, so I thought the duplication wasn't too terrible. wdyt? https://codereview.chromium.org/13467028/diff/15001/Source/WebCore/rendering/... File Source/WebCore/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/13467028/diff/15001/Source/WebCore/rendering/... Source/WebCore/rendering/RenderLayer.cpp:682: // instead. On 2013/04/16 19:17:41, vollick wrote: > Let's just fix this rather than putting in a FIXME. It's definitely the right > thing to do. I tried to fix this, but ran into the following assert in debug mode: STDERR: ASSERTION FAILED: enclosingIntRect(rendererMappedResult) == enclosingIntRect(FloatQuad(result).boundingBox()) STDERR: ../../third_party/WebKit/Source/core/rendering/RenderGeometryMap.cpp(142) : WebCore::FloatQuad WebCore::RenderGeometryMap::mapToContainer(const WebCore::FloatRect &, const WebCore::RenderLayerModelObject *) const when running LayoutTests/compositing/overflow/build-paint-order-lists.html. Julien: any idea why this might be? https://codereview.chromium.org/13467028/diff/15001/Source/WebCore/rendering/... File Source/WebCore/rendering/RenderLayer.h (right): https://codereview.chromium.org/13467028/diff/15001/Source/WebCore/rendering/... Source/WebCore/rendering/RenderLayer.h:863: void collectBeforeAfterPromotionZOrderLists(RenderLayer* ancestorStackingContext, On 2013/04/16 19:17:41, vollick wrote: > Please combine these two methods. They're hard to distinguish by their names, > and looking at the .cpp, they could be merged. (And use the first, more > descriptive, method name). Done.
Next round of review here... Mostly nits, except for one worry I have about our liberal use of rebuildZOrderLists(). https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:760: ancestorStackingContext->rebuildZOrderLists(StopAtStackingContexts, posZOrderListAfterPromote, negZOrderListAfterPromote, this); I feel uncomfortable about using this function to perform speculative computations that theoretically should not affect the layer tree state. This function calls updateDescendantDependentFlags(), which actually does update a few stateful pieces of information -- and then it marks those values as "clean". So even before this patch, the code is precariously balanced on trusting that we have correctly (or conservatively) dirtied those flags when needed. So, to suddenly call it twice in a place where we don't even intend to change the layer tree seems prone to errors down the road... So my question is: do we have any guarantees we will have that we won't accidentally change the state of the layers and keep it changed by mistake? https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:5820: void RenderLayer::rebuildZOrderLists(CollectLayersBehavior behavior, OwnPtr<Vector<RenderLayer*> >& posZOrderList, OwnPtr<Vector<RenderLayer*> >& negZOrderList, const RenderLayer* layerToForceAsStackingContext) To be strict with the distinction between stacking context and stacking container, shouldn't this variable name be "layerToForceAsStackingContainer" ? Or am I missing something? https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:5870: void RenderLayer::collectLayers(bool includeHiddenLayers, CollectLayersBehavior behavior, OwnPtr<Vector<RenderLayer*> >& posBuffer, OwnPtr<Vector<RenderLayer*> >& negBuffer, const RenderLayer* layerToForceAsStackingContext) same question about variable name as above. https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:5879: bool isStacking = (this == layerToForceAsStackingContext) || (behavior == StopAtStackingContexts ? isStackingContext() : isStackingContainer()); Not your patch, but the existing conditions on this line of code make me uncomfortable. If we ever extend the CollectLayersBehavior enum further, there's a chance this may become wrong. I feel there's a more robust way to handle this that is reasonably compact and comfortable to read... https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayer.h (right): https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.h:840: String paintOrderListsAsText(); Should this be const? Hopefully it's possible to do that... https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.h:841: PassRefPtr<PaintOrderLists> paintOrderLists(); Also should this be const? Hopefully it's possible to do that...
https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:760: ancestorStackingContext->rebuildZOrderLists(StopAtStackingContexts, posZOrderListAfterPromote, negZOrderListAfterPromote, this); On 2013/04/18 02:22:14, shawnsingh wrote: > I feel uncomfortable about using this function to perform speculative > computations that theoretically should not affect the layer tree state. This > function calls updateDescendantDependentFlags(), which actually does update a > few stateful pieces of information -- and then it marks those values as "clean". > So even before this patch, the code is precariously balanced on trusting that > we have correctly (or conservatively) dirtied those flags when needed. So, to > suddenly call it twice in a place where we don't even intend to change the layer > tree seems prone to errors down the road... > I get your concern, but I think we're pretty safe here. updateDescendantDependentFlags() does update some state, but it only cleans what's already dirty. If something gets changed after we finish in this function, the bits should get set to dirty again, and the next time we clean we'll be at the same state we would've been anyway. I'm not a fan of relying on dirty bits too much either, but we _should_ be able to rely on them - otherwise they're really not doing their job. FWIW, Ian has a patch (https://codereview.chromium.org/13859006/) which cleans up a lot of the dirty/clean bit code and should give us a bit more confidence that these bits are being maintained properly. > So my question is: do we have any guarantees we will have that we won't > accidentally change the state of the layers and keep it changed by mistake? Basically, the way I see it, since the only state we change is to clean already-dirty values, we kind of _want_ to keep it changed, don't we? I don't see a reason we would want to intentionally keep things dirty longer, the idea is that everything should be clean by the time we need to use it regardless. https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:5820: void RenderLayer::rebuildZOrderLists(CollectLayersBehavior behavior, OwnPtr<Vector<RenderLayer*> >& posZOrderList, OwnPtr<Vector<RenderLayer*> >& negZOrderList, const RenderLayer* layerToForceAsStackingContext) On 2013/04/18 02:22:14, shawnsingh wrote: > To be strict with the distinction between stacking context and stacking > container, shouldn't this variable name be "layerToForceAsStackingContainer" ? > Or am I missing something? Yeah, you're right, good catch. https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:5870: void RenderLayer::collectLayers(bool includeHiddenLayers, CollectLayersBehavior behavior, OwnPtr<Vector<RenderLayer*> >& posBuffer, OwnPtr<Vector<RenderLayer*> >& negBuffer, const RenderLayer* layerToForceAsStackingContext) On 2013/04/18 02:22:14, shawnsingh wrote: > same question about variable name as above. Done. https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:5879: bool isStacking = (this == layerToForceAsStackingContext) || (behavior == StopAtStackingContexts ? isStackingContext() : isStackingContainer()); On 2013/04/18 02:22:14, shawnsingh wrote: > Not your patch, but the existing conditions on this line of code make me > uncomfortable. If we ever extend the CollectLayersBehavior enum further, > there's a chance this may become wrong. I feel there's a more robust way to > handle this that is reasonably compact and comfortable to read... Changed to a switch statement, does this look better? https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayer.h (right): https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.h:840: String paintOrderListsAsText(); On 2013/04/18 02:22:14, shawnsingh wrote: > Should this be const? Hopefully it's possible to do that... Unfortunately it's not possible. We need to call collectBeforeAfterPromotionZOrderLists(), which is necessarily non-const (due to temporary changes of m_needsCompositedScrolling and m_isNormalFlowOnly). https://codereview.chromium.org/13467028/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.h:841: PassRefPtr<PaintOrderLists> paintOrderLists(); On 2013/04/18 02:22:14, shawnsingh wrote: > Also should this be const? Hopefully it's possible to do that... Same as above.
In response to the dirty/clean business --> I don't see that https://codereview.chromium.org/13859006/ is addressing the particular issue i'm referring to. Am I missing something about the relationship between dirty flags that increases your confidence? What you're saying about updateDescendantDependentFlags being worth calling arbitrarily to make things clean --> relies on the assumption that none of those properties being computed will be depend on which ancestor is its stacking container. With the existing state of the code, I agree that's an assumption that we can make. But it still feels like not-ideal-design to rely on such an assumption. such an assumption could easily be overlooked when someone is modifying updateDescendantDependentFlags - it really doesn't look like an assumption that needs to exist from that code's perspective. The bigger issue is that it's needed at all while rebuilding z order lists. If everyone else wants to land this, I'm OK not blocking beacuse of this issue. The code does seem as it is, and technically the problem is not the patch's code, but the code that the patch is using. But we should definitely take some time to discuss the possibility of removing updateDescendantDependentFlags from rebuildZOrderLists, and trying to be more certain that it's being called in a principled way rather than "just in case", and especially so that this speculative z-order-list computation is limited in the scope of what properties it affects (i.e. doesn't affect any data except z order lists).
typo above - it should say "the code does seem correct as it is" =)
On 2013/04/19 06:51:31, shawnsingh wrote: > In response to the dirty/clean business --> > > I don't see that https://codereview.chromium.org/13859006/ is addressing the > particular issue i'm referring to. Am I missing something about the > relationship between dirty flags that increases your confidence? > > What you're saying about updateDescendantDependentFlags being worth calling > arbitrarily to make things clean --> relies on the assumption that none of those > properties being computed will be depend on which ancestor is its stacking > container. With the existing state of the code, I agree that's an assumption > that we can make. But it still feels like not-ideal-design to rely on such an > assumption. such an assumption could easily be overlooked when someone is > modifying updateDescendantDependentFlags - it really doesn't look like an > assumption that needs to exist from that code's perspective. The bigger issue > is that it's needed at all while rebuilding z order lists. Ah I see, I don't think I fully understood your concerns before. How about the following: we have a few bugs (https://crbug.com/233597, https://crbug.com/233595, https://crbug.com/233598) that should break this up into more well-defined phases, and add better enforcement to make sure the composited scrolling code and updateDescendantDependentFlags() don't interfere with each other's state. It's a bit much to be included in this patch, but I think the code is correct as-is, and the concern is more about future modifications. So I think if we can take on these bugs shortly, we can be confident that we won't run into these conflicts. Do you agree?
Sure, sounds good. =) this patch looks good to me, then
I would avoid adding a new interface (the IDL file) just for testing. Especially if its in core/dom, it's hard to tell whether this is a public DOM API or just something we use in Blink. You have a couple of options (all good): 1) Make your API a bit more clunky and have two calls to return before and after lists 2) Move PaintOrderLists to Source/core/testing, and thus make the purpose of the interface clear.
On 2013/04/19 21:15:16, Dimitri Glazkov wrote: > I would avoid adding a new interface (the IDL file) just for testing. Especially > if its in core/dom, it's hard to tell whether this is a public DOM API or just > something we use in Blink. You have a couple of options (all good): > > 1) Make your API a bit more clunky and have two calls to return before and after > lists > 2) Move PaintOrderLists to Source/core/testing, and thus make the purpose of the > interface clear. Done, I picked the former option, seems simpler than adding new files and interfaces just for one layout test. Thanks!
Julien, this has been pre-reviewed by Ian and Shawn, I think it's ready for your review now. Please take a look. Thanks, Ian and Shawn!
As a whole, the change looks good. I am just afraid of the code duplication. https://codereview.chromium.org/13467028/diff/57001/LayoutTests/compositing/o... File LayoutTests/compositing/overflow/build-paint-order-lists.html (right): https://codereview.chromium.org/13467028/diff/57001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-lists.html:57: function getPaintOrder(element) This function has a non-trivial behavior and it's duplicated twice which makes it super easy for one version to drift from the other one. Couldn't we share both of them in some utility function? https://codereview.chromium.org/13467028/diff/57001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/13467028/diff/57001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:707: void RenderLayer::collectBeforePromotionZOrderList( Some of the line breaking are *really* weird (like this one). The blink style doesn't use a 80 column limit so couldn't we avoid such unnatural breakings? https://codereview.chromium.org/13467028/diff/57001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:805: if (ancestorStackingContext != this && ancestorStackingContext->isStackingContext()) The 'ancestorStackingContext != this' check is artificial and could be removed if you changed how you write your loop: RenderLayer* ancestorStackingContext = parent(); // RenderView, nothing to do. if (!ancestorStackingContext) return String(); while (!ancestorStackingContext->isStackingContext()) ancestorStackingContext = ancestorStackingContext->parent(); https://codereview.chromium.org/13467028/diff/57001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:808: } This should be in its own function really as it is repeated 3 times (named e.g. ancestorStackingContext()). https://codereview.chromium.org/13467028/diff/57001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:814: collectAfterPromotionZOrderList(ancestorStackingContext, posZOrderListAfterPromote, negZOrderListAfterPromote); The code duplication makes me sad. It seems that it could be possible to write paintOrderListsAsText on top of paintOrderListBeforePromote / paintOrderListAfterPromote or at least by adding a common lower level implementation (like that would return or fills the layer Vectors).
Thanks for the comments, Julien! I believe this is ready for another review. I've uploaded a new patch, PTAL. https://codereview.chromium.org/13467028/diff/57001/LayoutTests/compositing/o... File LayoutTests/compositing/overflow/build-paint-order-lists.html (right): https://codereview.chromium.org/13467028/diff/57001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-lists.html:57: function getPaintOrder(element) On 2013/04/22 21:49:30, Julien Chaffraix wrote: > This function has a non-trivial behavior and it's duplicated twice which makes > it super easy for one version to drift from the other one. Couldn't we share > both of them in some utility function? Done. https://codereview.chromium.org/13467028/diff/57001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/13467028/diff/57001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:707: void RenderLayer::collectBeforePromotionZOrderList( On 2013/04/22 21:49:30, Julien Chaffraix wrote: > Some of the line breaking are *really* weird (like this one). The blink style > doesn't use a 80 column limit so couldn't we avoid such unnatural breakings? Done. https://codereview.chromium.org/13467028/diff/57001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:805: if (ancestorStackingContext != this && ancestorStackingContext->isStackingContext()) On 2013/04/22 21:49:30, Julien Chaffraix wrote: > The 'ancestorStackingContext != this' check is artificial and could be removed > if you changed how you write your loop: > > RenderLayer* ancestorStackingContext = parent(); > // RenderView, nothing to do. > if (!ancestorStackingContext) > return String(); > while (!ancestorStackingContext->isStackingContext()) > ancestorStackingContext = ancestorStackingContext->parent(); > Done. https://codereview.chromium.org/13467028/diff/57001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:808: } On 2013/04/22 21:49:30, Julien Chaffraix wrote: > This should be in its own function really as it is repeated 3 times (named e.g. > ancestorStackingContext()). Done. https://codereview.chromium.org/13467028/diff/57001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:814: collectAfterPromotionZOrderList(ancestorStackingContext, posZOrderListAfterPromote, negZOrderListAfterPromote); On 2013/04/22 21:49:30, Julien Chaffraix wrote: > The code duplication makes me sad. It seems that it could be possible to write > paintOrderListsAsText on top of paintOrderListBeforePromote / > paintOrderListAfterPromote or at least by adding a common lower level > implementation (like that would return or fills the layer Vectors). I'm not sure I understand your suggestion. collect[Before|After]PromotionZOrderList() already serves as the common implementation to fill the layer vectors. paintOrderList[Before|After]Promote() just converts the vector representation into a NodeList so that it can be returned through javascript. I would have liked to keep this in Internals.cpp, but unfortunately it relies on private RenderLayer functions that I'd prefer not to expose (collect[Before|After]PromotionZOrderList() and getStackingOrderElementAt()). The only duplicate code I see is calling collect[Before|After]PromotionZOrderList() and then determining the sizes of the populated vectors (which I admit isn't too pretty, since we have to check each for nullity - so I've moved that inside ollect[Before|After]PromotionZOrderList()). Were you trying to point out something larger? https://codereview.chromium.org/13467028/diff/57001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:844: PassRefPtr<NodeList> RenderLayer::paintOrderListBeforePromote() I reworked these two functions, and the ones in Internals, a little bit to try to reduce duplicate code. The solution involved adding an enum argument to the functions, but overall I think it's cleaner. Please let me know if you disagree.
LGTM. My comment was about the same setup code being copied and pasted several times. In the new code, this boilerplate code was greatly reduced so it's not that bad. https://codereview.chromium.org/13467028/diff/78001/LayoutTests/compositing/o... File LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html (right): https://codereview.chromium.org/13467028/diff/78001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html:140: write("FAIL - paint order list before promote doesn't match stacking order"); Does it make sense for these failure to not return? (ie continue testing and finally print PASS) https://codereview.chromium.org/13467028/diff/78001/LayoutTests/compositing/o... File LayoutTests/compositing/overflow/build-paint-order-lists.html (right): https://codereview.chromium.org/13467028/diff/78001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-lists.html:52: background-color: yellow; Do we *really* need all this style? Ideally we should strive to make our test case as minimal as possible. https://codereview.chromium.org/13467028/diff/78001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/13467028/diff/78001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:744: // Insert this into the posZOrderistBeforePromote directly after the typo: posZOrder*L*istBeforePromote https://codereview.chromium.org/13467028/diff/78001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayer.h (right): https://codereview.chromium.org/13467028/diff/78001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.h:828: // layerToForceAsStackingContainer parameter. Nit: You don't need to wrap at 80 columns in Blink, this would be totally fine on the previous line.
Thanks again for the review, Julien! https://codereview.chromium.org/13467028/diff/78001/LayoutTests/compositing/o... File LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html (right): https://codereview.chromium.org/13467028/diff/78001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html:140: write("FAIL - paint order list before promote doesn't match stacking order"); On 2013/04/24 15:44:01, Julien Chaffraix wrote: > Does it make sense for these failure to not return? (ie continue testing and > finally print PASS) No, you're right, it probably doesn't make sense to print PASS at the end. I'd rather we not return immediately though, it seems like we can get more information if we print out all the failures instead of stopping after the first one. I've wrapped the write("PASS...") in an if statement so that we only print PASS if we haven't already failed somewhere. https://codereview.chromium.org/13467028/diff/78001/LayoutTests/compositing/o... File LayoutTests/compositing/overflow/build-paint-order-lists.html (right): https://codereview.chromium.org/13467028/diff/78001/LayoutTests/compositing/o... LayoutTests/compositing/overflow/build-paint-order-lists.html:52: background-color: yellow; On 2013/04/24 15:44:01, Julien Chaffraix wrote: > Do we *really* need all this style? Ideally we should strive to make our test > case as minimal as possible. I've taken out what I can, unfortunately a lot of the positioning is necessary to make sure the right elements overlap. The separate background colours aren't strictly required, but taking them out makes debugging difficult. https://codereview.chromium.org/13467028/diff/78001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/13467028/diff/78001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:744: // Insert this into the posZOrderistBeforePromote directly after the On 2013/04/24 15:44:01, Julien Chaffraix wrote: > typo: posZOrder*L*istBeforePromote Done. https://codereview.chromium.org/13467028/diff/78001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayer.h (right): https://codereview.chromium.org/13467028/diff/78001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.h:828: // layerToForceAsStackingContainer parameter. On 2013/04/24 15:44:01, Julien Chaffraix wrote: > Nit: You don't need to wrap at 80 columns in Blink, this would be totally fine > on the previous line. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hartmanng@chromium.org/13467028/83001
Presubmit check for 13467028-83001 failed and returned exit status -2001. The presubmit check was hung. It took 360.3 seconds to execute and the time limit is 360.0 seconds. INFO:root:Found 10 file(s).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hartmanng@chromium.org/13467028/83001
Presubmit check for 13467028-83001 failed and returned exit status -2001. The presubmit check was hung. It took 360.0 seconds to execute and the time limit is 360.0 seconds. INFO:root:Found 10 file(s).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hartmanng@chromium.org/13467028/83001
Presubmit check for 13467028-83001 failed and returned exit status -2001. The presubmit check was hung. It took 360.0 seconds to execute and the time limit is 360.0 seconds. INFO:root:Found 10 file(s).
Message was sent while issue was closed.
Committed patchset #9 manually as r149028 (presubmit successful).
Message was sent while issue was closed.
This patch doesn't seem right. It adds 250 lines to RenderLayer all for running two layout tests. It also added a ton of comments to RenderLayer that are walking through what the test actually does which is super weird. RenderLayer.cpp isn't a unit test... It'd be nice if we could move all this code out of RenderLayer.
Message was sent while issue was closed.
On 2013/04/26 01:57:56, esprehn wrote: > This patch doesn't seem right. It adds 250 lines to RenderLayer all for running > two layout tests. It also added a ton of comments to RenderLayer that are > walking through what the test actually does which is super weird. > RenderLayer.cpp isn't a unit test... > > It'd be nice if we could move all this code out of RenderLayer. This is part of a bigger plan to fix comp-scroll opt in(next patch -- https://codereview.chromium.org/13427009/). Putting all the code in a single patch was too much, so this was done as an intermediate step. The goal isn't to support the tests, but the next patch. That said, we do hope to remove all this eventually. I'm hoping to change things so that composited-scrolling won't affect stacking or clipping and all this special casing can go away.
Message was sent while issue was closed.
This patch is also full of style violations and improper usage of types. :( |
