Bug 86022 – Refactor layer-related logic out of RenderBoxModelObject

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Florin Malita]

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 |
   +----------------------+   +----------------------+

[Florin Malita]

http://bugs.webkit.org/show_bug.cgi?id=23113 explains how this relates to SVG ForeignObject layers.

[Florin Malita]

Created attachment 141038 [details]
Patch

First pass.

[Eric Seidel (no email)]

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]

(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 on attachment 141038 [details]
Patch

Attachment 141038 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12644867

[Build Bot]

Comment on attachment 141038 [details]
Patch

Attachment 141038 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12663132

[Simon Fraser (smfr)]

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)]

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]

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]

(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]

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]

Created attachment 149971 [details]
Patch

[Simon Fraser (smfr)]

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]

(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)]

RenderTransformableObject? RenderBaseObject?

[Florin Malita]

(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]

Created attachment 151471 [details]
Patch

[Julien Chaffraix]

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]

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]

Created attachment 152337 [details]
Patch

[Build Bot]

Comment on attachment 152337 [details]
Patch

Attachment 152337 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13239242

[Florin Malita]

Created attachment 152439 [details]
Patch

Hopefully fix the Win build.

[Julien Chaffraix]

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 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 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]

(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]

(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)]

(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]

Created attachment 153116 [details]
Patch

Reverted RenderBoxModelObject::style{Did,Will}Change refactoring.

[Julien Chaffraix]

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]

(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)]

(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]

(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]

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]

Created attachment 160168 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 160168 [details]
Patch

Attachment 160168 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13569653

[Early Warning System Bot]

Comment on attachment 160168 [details]
Patch

Attachment 160168 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13562746

[Florin Malita]

Created attachment 160171 [details]
Patch

Build fix

[Build Bot]

Comment on attachment 160171 [details]
Patch

Attachment 160171 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13567740

[Florin Malita]

Created attachment 160195 [details]
Patch

Mac build fix

[Dave Hyatt]

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]

> 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]

(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]

(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]

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]

Created attachment 166057 [details]
Patch

Updated class name: RenderLayerModelObject

[Nikolas Zimmermann]

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]

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]

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)]

RenderLayerModelObject is OK with me.

[Philip Rogers]

(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 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]

Created attachment 166537 [details]
Patch

Updated per comments.

[Dave Hyatt]

Comment on attachment 166537 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 166537 [details]
Patch

Clearing flags on attachment: 166537

Committed r130081: <http://trac.webkit.org/changeset/130081>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Florin Malita]

Thanks everyone for reviewing this!