RESOLVED FIXED 86022
Refactor layer-related logic out of RenderBoxModelObject
https://bugs.webkit.org/show_bug.cgi?id=86022
Summary Refactor layer-related logic out of RenderBoxModelObject
Florin Malita
Reported 2012-05-09 15:22:16 PDT
The intent is to extract layer-related functionality into a dedicated class that can be used to support non-RenderBoxModelObject layers. This is the first phase (and a prerequisite) for adding SVG ForeignObject layers support. The plan: (currently) +--------------+ | RenderObject | +--------------+ ^ | +-----------+-------------+ | | +-------------+ +-----------+----------+ +----------+-----------+ | RenderLayer |*----| RenderBoxModelObject | | RenderSVGModelObject | +-------------+ +----------------------+ +----------------------+ (this refactoring) +--------------+ | RenderObject | +--------------+ ^ | +----------+-----------+ | | +-------------+ +------+-------+ +----------+-----------+ | RenderLayer |*----| (LayerStuff) | | RenderSVGModelObject | +-------------+ +--------------+ +----------------------+ ^ | +----------------------+ | RenderBoxModelObject | +----------------------+ (eventually) +--------------+ | RenderObject | +--------------+ ^ | +-------------+ +--------------+ | RenderLayer |*----| (LayerStuff) | +-------------+ +--------------+ ^ | +-----------+-------------+ | | +-----------+----------+ +----------+-----------+ | RenderBoxModelObject | | RenderSVGModelObject | +----------------------+ +----------------------+
Attachments
Patch (114.42 KB, patch)
2012-05-09 16:03 PDT, Florin Malita
no flags
Patch (129.88 KB, patch)
2012-06-28 10:47 PDT, Florin Malita
no flags
Patch (127.55 KB, patch)
2012-07-10 09:48 PDT, Florin Malita
no flags
Patch (138.99 KB, patch)
2012-07-13 14:41 PDT, Florin Malita
no flags
Patch (139.58 KB, patch)
2012-07-14 17:29 PDT, Florin Malita
no flags
Patch (132.48 KB, patch)
2012-07-18 15:49 PDT, Florin Malita
no flags
Patch (145.34 KB, patch)
2012-08-23 08:23 PDT, Florin Malita
no flags
Patch (146.11 KB, patch)
2012-08-23 08:43 PDT, Florin Malita
no flags
Patch (146.15 KB, patch)
2012-08-23 10:56 PDT, Florin Malita
no flags
Patch (146.84 KB, patch)
2012-09-27 13:41 PDT, Florin Malita
no flags
Patch (147.12 KB, patch)
2012-10-01 14:01 PDT, Florin Malita
no flags
Florin Malita
Comment 1 2012-05-09 15:29:53 PDT
http://bugs.webkit.org/show_bug.cgi?id=23113 explains how this relates to SVG ForeignObject layers.
Florin Malita
Comment 2 2012-05-09 16:03:02 PDT
Created attachment 141038 [details] Patch First pass.
Eric Seidel (no email)
Comment 3 2012-05-09 16:06:37 PDT
RenderLayer has a whole bunch of CSS-specific stuff. I'm surprised we'd want SVG objects to have a RenderLayer as is.
Florin Malita
Comment 4 2012-05-09 16:17:20 PDT
(In reply to comment #3) > RenderLayer has a whole bunch of CSS-specific stuff. I'm surprised we'd want SVG objects to have a RenderLayer as is. Refactoring RenderLayer should remain an option - I haven't gotten far enough down that path.
Build Bot
Comment 5 2012-05-09 16:22:24 PDT
Build Bot
Comment 6 2012-05-09 16:37:55 PDT
Simon Fraser (smfr)
Comment 7 2012-05-09 16:41:26 PDT
Comment on attachment 141038 [details] Patch I don't think RenderLayerBase is a good name for something that RenderBoxModelObject inherits from; that name implies that it's a base class of RenderLayer. The name should be RenderSomethingBox or RenderBoxSomething. RenderBoxModelObject was actually created for this very purpose a while back, so perhaps you should investigate why RenderSVGModelObject can't inherit from RenderBoxModelObject.
Eric Seidel (no email)
Comment 8 2012-05-09 17:00:39 PDT
RenderSVGModelObject was created by me (on a 16hr plane flight to sydney) to unify SVG object logic into a single baseclass. Previously all SVG renders had been moved off of RenderBlock (where they started) as the SVG layout model is quite distinct from the CSS box model. SVG ignores non-SVG objects inside its tree: http://www.w3.org/TR/SVG/extend.html#ForeignNamespaces similarly, SVG objects which exist outside of an <svg> root, are ignored by CSS, as svg elements require an <svg> element to establish a viewport for them: http://www.w3.org/TR/SVG/coords.html It's possible that "being" is not the right model for logic inclusion here. Unfortunately Simon and I didn't get to chat at the contributor meeting about what troubles with foreignObject participating in the layer tree were. <foreignObject> is a RenderBlock and thus could have a layer. It's unclear to me if there are still troubles there or not. I do not believe that RenderSVGBoxModelObjects want a RenderLayer as RenderLayer is currently designed. They may very well want a GraphicsLayer, but they can get that w/o having a RenderLayer. I view RenderLayer as largely a rare-data object for RenderBoxModelObject. I understand it does more than that, but it's largely tied to being an object which participates in the CSS box model, which all SVG renders (with the exception of SVGRenderRoot) do not.
Julien Chaffraix
Comment 9 2012-05-09 17:14:31 PDT
More comments, Dirk Schulze mentioned that SVG is only interested in the "stacking context" part of RenderLayer IIRC. I am looking into splitting RenderLayer which could help your effort. I am unsure about the current direction: splitting the code into a new class is good but I don't think the new class should be a RenderObject. Having a holder for layers would be much better. I may be wrong but if you want something akin to RenderLayer (to attach GraphicsLayer or share code with CSS) in SVG, it looks like the "RenderLayer" object should be attached to RenderObject, not some artificial new object inserted in the middle. All CSS classes are RenderBoxModelObjects so that's not a memory penalty, only SVG and MathML would see an increase in objects' size, which was to be expected for SVG anyway. It would also remove all of the down-casts that we have in RenderObject.
Florin Malita
Comment 10 2012-05-09 18:32:43 PDT
(In reply to comment #7) > (From update of attachment 141038 [details]) > I don't think RenderLayerBase is a good name for something that RenderBoxModelObject inherits from; that name implies that it's a base class of RenderLayer. Agreed. I forgot to mention that the class name is tentative at best and I'm looking for suggestions. > The name should be RenderSomethingBox or RenderBoxSomething. RenderBoxModelObject was actually created for this very purpose a while back, so perhaps you should investigate why RenderSVGModelObject can't inherit from RenderBoxModelObject. I think the short answer is that the CSS box model is not applicable to SVG objects, and most of the stuff in RenderBoxModelObject is simply not needed. So it seems reasonable to extract the few layer-related bits that are needed in a common base class. In this light, a Render*Box* name may not be the best choice either. (In reply to comment #8) > Unfortunately Simon and I didn't get to chat at the contributor meeting about what troubles with foreignObject participating in the layer tree were. <foreignObject> is a RenderBlock and thus could have a layer. It's unclear to me if there are still troubles there or not. Two issues that I'm aware of: * preserving the painting context across layers - SVG elements accumulate state (transforms, filters, masks, etc) as the tree is painted; this context is currently lost when switching between layers (so for example painting a FO with a layer will ignore any filters on its ancestors). * stacking order - SVG elements stack in document order and a direct FO layer approach (simply giving the FO its own layer) would pretty much break that. To properly support FO layers using the current stacking logic, we would have to create a synthetic pre-FO layer, the actual FO layer and another synthetic post-FO layer. The related discussion is on this thread: http://bugs.webkit.org/show_bug.cgi?id=23113 This patch is a step towards addressing the second problem: supporting the ability to create pre & post FO layers for arbitrary SVG elements. Without it, ForeignObject layers would break the SVG stacking order. > I do not believe that RenderSVGBoxModelObjects want a RenderLayer as RenderLayer is currently designed. That may very well be the case. But whatever RenderSVGBoxModelObjects wants (and gets) should be able to stack and intermingle with regular RenderLayers, because ForeignObject content can always insert those into the hierarchy. (In reply to comment #9) > More comments, Dirk Schulze mentioned that SVG is only interested in the "stacking context" part of RenderLayer IIRC. I am looking into splitting RenderLayer which could help your effort. Great - if we can isolate the stacking logic and only use that subset for SVG-related layers then that should address some of Eric's concerns. > I may be wrong but if you want something akin to RenderLayer (to attach GraphicsLayer or share code with CSS) in SVG, it looks like the "RenderLayer" object should be attached to RenderObject, not some artificial new object inserted in the middle. All CSS classes are RenderBoxModelObjects so that's not a memory penalty, only SVG and MathML would see an increase in objects' size, which was to be expected for SVG anyway. It would also remove all of the down-casts that we have in RenderObject. I've actually considered that, and decided to try what I deemed to be a less controversial approach first :) But I like the idea and would definitely give it a shot if others are on board.
Florin Malita
Comment 11 2012-06-28 09:47:33 PDT
I looked at pushing the layer RenderBoxModelObject bits into RenderObject, but that would add unneeded overhead to RenderText - so it's probably better to keep it as a separate class. I'm still hoping for inspiration to strike as far as naming goes (RenderLayerObject, RenderObjectWithLayer, RenderPleaseSomeoneComeUpWithAGoodNameObject ;), but I'll put up an update to test some platform build fixes.
Florin Malita
Comment 12 2012-06-28 10:47:09 PDT
Simon Fraser (smfr)
Comment 13 2012-06-28 10:56:38 PDT
Comment on attachment 149971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149971&action=review > Source/WebCore/rendering/RenderLayerObject.h:29 > +class RenderLayerObject : public RenderObject { I don't really like the class name. I'd prefer to pick a name that reflects what's common about RenderBoxModelObject and RenderSVG stuff in terms of behavior, not in terms of implementation. I'd also like to reduce the ties between RenderObjects and RenderLayers, not strengthen them, conceptually.
Florin Malita
Comment 14 2012-07-09 14:26:31 PDT
(In reply to comment #13) > I don't really like the class name. I'd prefer to pick a name that reflects what's common about RenderBoxModelObject and RenderSVG stuff in terms of behavior, not in terms of implementation. I'd also like to reduce the ties between RenderObjects and RenderLayers, not strengthen them, conceptually. Here are some alternatives that we (jchaffraix, schenney and myself) have come up with, in order of preference: * RenderModelObject - a RenderObject with some model applied to it (box model, svg model, ...) * RenderAugmentableObject - a RenderObject that can be possibly modified/augmented via various operations: positioning, transforming, stacking, filters, clipping, etc. * RenderOperationsTarget - same idea as above Let me know what you think would work best. And by all means, it doesn't have to be on this shortlist :)
Simon Fraser (smfr)
Comment 15 2012-07-09 14:27:56 PDT
RenderTransformableObject? RenderBaseObject?
Florin Malita
Comment 16 2012-07-09 14:43:18 PDT
(In reply to comment #15) > RenderTransformableObject? RenderBaseObject? Both sound good to me (and were on our long list in some form). I'll put an updated patch up.
Florin Malita
Comment 17 2012-07-10 09:48:16 PDT
Julien Chaffraix
Comment 18 2012-07-12 16:05:20 PDT
Comment on attachment 151471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151471&action=review All in all, it's a fine patch but I would like to see another round. The next round will very likely get an r+ from me so if you want to nitpick, now is the moment. > Source/WebCore/ChangeLog:12 > + I would like an explanation for what qualifies for your massive s/RenderBoxModelObject/RenderBaseObject/. Maybe better would be function-level comments about why groups of functions were touched. > Source/WebCore/rendering/RenderBaseObject.cpp:33 > +}; Stray ';' > Source/WebCore/rendering/RenderBaseObject.cpp:69 > + // This must be done before we destroy the RenderObject. > + if (m_layer) > + m_layer->clearClipRects(); I don't think this is needed. > Source/WebCore/rendering/RenderBaseObject.h:29 > +class RenderBaseObject : public RenderObject { Sincerely this name doesn't add much. RenderBaseObject as in? There is a <base> element in HTML which doesn't have a renderer but such a naming would make me think it does. I would rather go with the other alternative (RenderTransformableObject) so that everyone is happy :) > Source/WebCore/rendering/RenderBaseObject.h:40 > + virtual bool requiresLayer() const { return false; } Nit: It would be better for now if this is pure virtual. Once SVG starts inheriting from this class, it could help us catch bad use. > Source/WebCore/rendering/RenderBaseObject.h:48 > + virtual bool isLayerBase() const OVERRIDE { return true; } I find |isLayerBase| to be difficult to read and understand at call sites. Mostly because it's a very generic naming and using the full object name would be better: isRenderBaseObject(). Arguably, canHaveALayer() could be better as it matches what we are interested in (but we lose the consistency with the other is* flags and we could object that we are tying the name to RenderLayer). > Source/WebCore/rendering/RenderBox.cpp:3764 > + RenderLayer* layer = curr->hasLayer() && curr->isBox() ? toRenderBaseObject(curr)->layer() : 0; toRenderBox here. > Source/WebCore/rendering/RenderBoxModelObject.cpp:402 > if (requiresLayer()) { Some of this should go to the new class (at least the creation / deletion). It's possible we want to defer that until SVG starts using it too as you would have a better picture. > Source/WebCore/rendering/RenderBoxModelObject.h:81 > virtual bool requiresLayer() const { return isRoot() || isOutOfFlowPositioned() || isRelPositioned() || isTransparent() || hasTransform() || hasHiddenBackface() || hasMask() || hasReflection() || hasFilter() || style()->specifiesColumns(); } OVERRIDE. > Source/WebCore/rendering/svg/RenderSVGModelObject.h:56 > + virtual LayoutRect clippedOverflowRectForRepaint(RenderBaseObject* repaintContainer) const; > + virtual void computeFloatRectForRepaint(RenderBaseObject* repaintContainer, FloatRect&, bool fixed = false) const; > + virtual LayoutRect outlineBoundsForRepaint(RenderBaseObject* repaintContainer, LayoutPoint*) const; Let's annotate all the functions touched that are overriding something with OVERRIDE while at it.
Florin Malita
Comment 19 2012-07-13 14:36:50 PDT
Thanks for the review Julien, updating the patch per your comments. (In reply to comment #18) > > Source/WebCore/rendering/RenderBaseObject.h:48 > > + virtual bool isLayerBase() const OVERRIDE { return true; } > > I find |isLayerBase| to be difficult to read and understand at call sites. Mostly because it's a very generic naming and using the full object name would be better: isRenderBaseObject(). Yup, that's a leftover from when the class name was RenderLayerBase, and my global rename script didn't catch it :) The intent is to match the class name, so it should be isTransformableObject() with the next version. > > Source/WebCore/rendering/RenderBoxModelObject.cpp:402 > > if (requiresLayer()) { > > Some of this should go to the new class (at least the creation / deletion). It's possible we want to defer that until SVG starts using it too as you would have a better picture. Yes, that makes sense. I'm taking a stab at simply moving the styleWillChange & styleDidChange stuff into RenderTransformableObject for now.
Florin Malita
Comment 20 2012-07-13 14:41:50 PDT
Build Bot
Comment 21 2012-07-13 15:22:18 PDT
Florin Malita
Comment 22 2012-07-14 17:29:20 PDT
Created attachment 152439 [details] Patch Hopefully fix the Win build.
Julien Chaffraix
Comment 23 2012-07-17 14:41:21 PDT
Comment on attachment 152439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152439&action=review > Source/WebCore/ChangeLog:11 > + of adding non-RenderBoxModelObject layer supprt. typo: support > Source/WebCore/rendering/RenderBoxModelObject.cpp:345 > + RenderTransformableObject::styleDidChange(diff, oldStyle); This changes a bit the order in which we do the previous operations but I think it's safe. > Source/WebCore/rendering/RenderLayer.cpp:1095 > + RenderObject* renderer = curr->renderer(); RenderTransformableObject no?
Simon Fraser (smfr)
Comment 24 2012-07-17 14:59:00 PDT
Comment on attachment 152439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152439&action=review >> Source/WebCore/rendering/RenderBoxModelObject.cpp:345 >> + RenderTransformableObject::styleDidChange(diff, oldStyle); > > This changes a bit the order in which we do the previous operations but I think it's safe. styleDidChange should always call super *first*. Is there a reason do to it in this order?
Julien Chaffraix
Comment 25 2012-07-17 15:13:24 PDT
Comment on attachment 152439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152439&action=review >>> Source/WebCore/rendering/RenderBoxModelObject.cpp:345 >>> + RenderTransformableObject::styleDidChange(diff, oldStyle); >> >> This changes a bit the order in which we do the previous operations but I think it's safe. > > styleDidChange should always call super *first*. Is there a reason do to it in this order? Good catch, I don't believe there is any dependencies between the operations so we should call styleDidChange first as you pointed out. Maybe Florin has more insights though.
Florin Malita
Comment 26 2012-07-18 11:46:31 PDT
(In reply to comment #24) > (From update of attachment 152439 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152439&action=review > > >> Source/WebCore/rendering/RenderBoxModelObject.cpp:345 > >> + RenderTransformableObject::styleDidChange(diff, oldStyle); > > > > This changes a bit the order in which we do the previous operations but I think it's safe. > > styleDidChange should always call super *first*. Is there a reason do to it in this order? Yes, it seems that updateBoxModelInfoFromStyle() needs to happen before the layer creation stuff (now in RenderTransformableObject::styleDidChange). On the first try I was calling super first and some tests were crashing. We could preserve the super-first convention with another virtual call from RenderTransformableObject::styleDidChange (updateModelFromStyle?) - is introducing an additional virtual call on this code path acceptable?
Florin Malita
Comment 27 2012-07-18 11:50:57 PDT
(In reply to comment #26) > We could preserve the super-first convention with another virtual call from RenderTransformableObject::styleDidChange (updateModelFromStyle?) - is introducing an additional virtual call on this code path acceptable? Or I could revert the style{Did,Will}Change refactoring for now, and clean it up in a separate patch.
Simon Fraser (smfr)
Comment 28 2012-07-18 11:59:02 PDT
(In reply to comment #26) > Yes, it seems that updateBoxModelInfoFromStyle() needs to happen before the layer creation stuff (now in RenderTransformableObject::styleDidChange). On the first try I was calling super first and some tests were crashing. That suggests that there's some base class dependency on derived class behavior, which is bad.
Florin Malita
Comment 29 2012-07-18 15:49:03 PDT
Created attachment 153116 [details] Patch Reverted RenderBoxModelObject::style{Did,Will}Change refactoring.
Julien Chaffraix
Comment 30 2012-07-19 15:29:40 PDT
Comment on attachment 153116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153116&action=review > Source/WebCore/rendering/RenderBoxModelObject.cpp:402 > + // FIXME: this should be handled by RenderTransformableObject::styleDidChange(), but it appears Nit: s/this/This/
Julien Chaffraix
Comment 31 2012-07-19 15:33:05 PDT
(In reply to comment #28) > (In reply to comment #26) > > Yes, it seems that updateBoxModelInfoFromStyle() needs to happen before the layer creation stuff (now in RenderTransformableObject::styleDidChange). On the first try I was calling super first and some tests were crashing. > > That suggests that there's some base class dependency on derived class behavior, which is bad. I was the one pushing for the style{Did|Will}Change part but this uncovered some underlying bug. Florin, don't forget to file a follow-up bug about this bad dependency.
Simon Fraser (smfr)
Comment 32 2012-07-19 15:34:54 PDT
(In reply to comment #31) > (In reply to comment #28) > > (In reply to comment #26) > > > Yes, it seems that updateBoxModelInfoFromStyle() needs to happen before the layer creation stuff (now in RenderTransformableObject::styleDidChange). On the first try I was calling super first and some tests were crashing. > > > > That suggests that there's some base class dependency on derived class behavior, which is bad. > > I was the one pushing for the style{Did|Will}Change part but this uncovered some underlying bug. Florin, don't forget to file a follow-up bug about this bad dependency. I'd prefer that it be understaood before this patch lands. Maybe this refactor isn't actually the right approach?
Florin Malita
Comment 33 2012-07-19 15:44:21 PDT
(In reply to comment #32) > (In reply to comment #31) > > (In reply to comment #28) > > > (In reply to comment #26) > > > > Yes, it seems that updateBoxModelInfoFromStyle() needs to happen before the layer creation stuff (now in RenderTransformableObject::styleDidChange). On the first try I was calling super first and some tests were crashing. > > > > > > That suggests that there's some base class dependency on derived class behavior, which is bad. > > > > I was the one pushing for the style{Did|Will}Change part but this uncovered some underlying bug. Florin, don't forget to file a follow-up bug about this bad dependency. > > I'd prefer that it be understaood before this patch lands. Maybe this refactor isn't actually the right approach? Are you referring specifically to the styleDidChange refactor, or the whole RenderTransformable patch? The latest version does not touch styleDidChange anymore (to avoid any functional changes in what is already a fairly large refactoring).
Florin Malita
Comment 34 2012-08-23 08:22:01 PDT
Sorry about the delay, I finally got around to revisiting this. UpdateBoxModelInfoFromStyle() needs to happen before handling RenderLayer creation/destruction because the latter depends on various bits being up to date. StyleWillChange() clears all the flags and (a possibly overwritten) updateBoxModelInfoFromStyle() is responsible for updating them. If that doesn't happen, the layer related logic will be looking at incorrect flags and likely making the wrong decision (for example not creating a layer because requiresLayer() returns false as hasTransform() has not been set yet). I propose that we promote updateBoxModelInfoFromStyle() to a RenderTransformableObject virtual with a more generic name and call it from RenderTransformableObject::styleDidChange() instead of RenderBoxModelObject::styleDidChange(). That way we enforce the ordering at the right level and don't depend on derived class behavior. I'll post an updated patch.
Florin Malita
Comment 35 2012-08-23 08:23:38 PDT
Gyuyoung Kim
Comment 36 2012-08-23 08:35:21 PDT
Early Warning System Bot
Comment 37 2012-08-23 08:41:47 PDT
Florin Malita
Comment 38 2012-08-23 08:43:00 PDT
Created attachment 160171 [details] Patch Build fix
Build Bot
Comment 39 2012-08-23 10:18:21 PDT
Florin Malita
Comment 40 2012-08-23 10:56:26 PDT
Created attachment 160195 [details] Patch Mac build fix
Dave Hyatt
Comment 41 2012-08-23 11:08:26 PDT
Comment on attachment 160195 [details] Patch I don't really mind the approach taken here, but RenderTransformableObject is just a terrible terrible name. You picked one aspect of layers (that they support transforms) and named the entire class after that one feature. That makes no sense. It seems like what you really mean is that this is an object that is capable of having a RenderLayer backing it. I think your name should simply reflect "capable of having a RenderLayer" and that it should not be using one specific feature of RenderLayers (that they are transformable). I'd suggest RenderLayerModelObject, since what you really mean is a RenderObject that participates in the RenderLayer way of doing things. Also, I'm not too happy with the generic rename to "updateFromStyle", but I don't really have a better suggestion.
Julien Chaffraix
Comment 42 2012-08-23 11:25:32 PDT
> I'd suggest RenderLayerModelObject, since what you really mean is a RenderObject that participates in the RenderLayer way of doing things. Dave, the consensus was that we shouldn't tie the new name to RenderLayer as it's an implementation detail that could change. See the first few patches that followed this naming and comment #14 with some alternate names.
Florin Malita
Comment 43 2012-08-23 11:53:26 PDT
(In reply to comment #41) > (From update of attachment 160195 [details]) > I don't really mind the approach taken here, but RenderTransformableObject is just a terrible terrible name. You picked one aspect of layers (that they support transforms) and named the entire class after that one feature. That makes no sense. "Transformable" was intended to have a more general meaning here (not necessarily restricted to CSS/SVG transforms), but I can see how overloading the term may be confusing. We went through several naming ideas, and honestly I'm ready to call it RenderYellowChicken if that would get everyone onboard :) Admittedly, RenderLayerModelObject is a good choice - except people have expressed reservations about reflecting a strong RenderLayer connection in the name. > Also, I'm not too happy with the generic rename to "updateFromStyle", but I don't really have a better suggestion. My only other idea here was updateModelFromStyle - definitely open to suggestions.
Dave Hyatt
Comment 44 2012-08-23 11:58:22 PDT
(In reply to comment #42) > > I'd suggest RenderLayerModelObject, since what you really mean is a RenderObject that participates in the RenderLayer way of doing things. > > Dave, the consensus was that we shouldn't tie the new name to RenderLayer as it's an implementation detail that could change. See the first few patches that followed this naming and comment #14 with some alternate names. I don't really agree with that consensus. If and when the object isn't about having a layer, then revisit the naming, but right now that's exactly what it is, so why obscure that fact? Like, are you really planning on making the object not use RenderLayers and/or not be about connecting to a RenderLayer? Isn't that the whole point here?
Florin Malita
Comment 45 2012-09-10 19:01:29 PDT
Simon suggested on IRC that we should also consider a composition-based approach. After poking around a bit, I still think inheritance works better in this instance: we already use composition to associate a RenderLayer with a renderer so the "has a" part is handled with the right abstraction; but it gets tricky as many methods need to handle layer-backed renderers uniformly (they basically operate on "something that might have a layer"). The only elegant non-inheritance solution I see at this point is to push the layer bits (m_layer, layer(), hasSelfPaintingLayer(), ensureLayer(), destroyLayer(), etc.) all to way up into RenderObject - but that would add an unneeded pointer to RenderText which I believe was deemed unacceptable. Other approaches seem to add even more overhead in both code duplication and/or layer-tracking memory needed. I don't think it's worth paying this price just to avoid extending the inheritance chain, but maybe I'm missing something. Thoughts? Also, Simon, what do you think of Dave's naming proposal (RenderLayerModelObject)?
Florin Malita
Comment 46 2012-09-27 13:41:32 PDT
Created attachment 166057 [details] Patch Updated class name: RenderLayerModelObject
Nikolas Zimmermann
Comment 47 2012-10-01 01:01:59 PDT
Comment on attachment 166057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166057&action=review Nice patch Florin. I'm tempted to r+ as-is, but I also have no strong opinion regarding the naming. The others deserve a chance to comment as well (Dave? Simon?), so I'll leave r? for now :-) > Source/WebCore/ChangeLog:11 > + of adding non-RenderBoxModelObject layer supprt. typo: support.
Dirk Schulze
Comment 48 2012-10-01 07:13:50 PDT
I am not absolutely happy with the name, since it looks like there is a inheritance hierarchies between the new class and RenderLayer. However, it is kind of sad that the naming is blocking this bug. I would like to move forward. The class can be renamed later anyway.
Philip Rogers
Comment 49 2012-10-01 10:06:23 PDT
This naming issue is derailing an otherwise good patch and is beginning to block other work building on this :/ It looks like there's some consensus on the current name--Simon, do you mind weighing in?
Simon Fraser (smfr)
Comment 50 2012-10-01 12:58:49 PDT
RenderLayerModelObject is OK with me.
Philip Rogers
Comment 51 2012-10-01 13:04:55 PDT
(In reply to comment #50) > RenderLayerModelObject is OK with me. Woohoo, we have a quorum! I think the name was the only remaining issue. Dave or Niko, can you do the final review?
Dirk Schulze
Comment 52 2012-10-01 13:22:49 PDT
Comment on attachment 166057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166057&action=review The patch looks good to me. Some snippets but nothing that could block a r+. Just giving dhyatt time to comment. > Source/WebCore/GNUmakefile.list.am:4998 > + Source/WebCore/rendering/RenderLayerModelObject.cpp \ > + Source/WebCore/rendering/RenderLayerModelObject.h \ indentation wrong. > Source/WebCore/rendering/RenderLayerModelObject.h:63 > + static bool s_wasFloating; > + static bool s_hadLayer; > + static bool s_hadTransform; > + static bool s_layerWasSelfPainting; You did not remove them from the header in RenderBoxModelObject.
Florin Malita
Comment 53 2012-10-01 14:01:14 PDT
Created attachment 166537 [details] Patch Updated per comments.
Dave Hyatt
Comment 54 2012-10-01 14:06:53 PDT
Comment on attachment 166537 [details] Patch r=me
WebKit Review Bot
Comment 55 2012-10-01 15:04:45 PDT
Comment on attachment 166537 [details] Patch Clearing flags on attachment: 166537 Committed r130081: <http://trac.webkit.org/changeset/130081>
WebKit Review Bot
Comment 56 2012-10-01 15:04:54 PDT
All reviewed patches have been landed. Closing bug.
Florin Malita
Comment 57 2012-10-01 15:12:09 PDT
Thanks everyone for reviewing this!
Note You need to log in before you can comment on or make changes to this bug.