Bug 122078 – Use RenderElement instead of RenderObject in many places

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Darin Adler]

Use RenderElement instead of RenderObject in many places

[Darin Adler]

Created attachment 212934 [details]
Patch

[Darin Adler]

Created attachment 212935 [details]
Patch

[Andreas Kling]

Comment on attachment 212935 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212935&action=review

I would say r+ but I feel insecure about the HTMLFormElement::rendererIsNeeded change.

> Source/WebCore/css/CSSFilterImageValue.cpp:137
> +    auto end = clients().end();
> +    for (auto it = clients().begin(); it != end; ++it)

Lame scoping of "end" here.

> Source/WebCore/html/HTMLFormElement.cpp:103
> -    ContainerNode* node = parentNode();
> -    RenderObject* parentRenderer = node->renderer();
> +    Element* parent = parentElement();
> +    RenderElement* parentRenderer = parent->renderer();

Is this change correct?
HTMLFormElement could be a direct child of Document, in which case parentElement() would be null, no?

> Source/WebCore/html/HTMLPlugInElement.cpp:90
> +    return renderer && (renderer->isEmbeddedObject() || renderer->isWidget());

RenderEmbeddedObject inherits from RenderWidget.
It's sufficient to check for renderer->isWidget() here.

> Source/WebCore/html/HTMLPlugInElement.cpp:229
> +    const PluginViewBase* plugin = pluginWidget() && pluginWidget()->isPluginViewBase() ? static_cast<const PluginViewBase*>(pluginWidget()) : nullptr;

pluginWidget() is nontrivial, we should use a local for it.

> Source/WebCore/html/HTMLSelectElement.cpp:682
> -    if (RenderObject* renderer = this->renderer())
> +    if (RenderElement* renderer = this->renderer())

auto

> Source/WebCore/html/HTMLTableCellElement.cpp:171
> -    RenderObject* cellRenderer = renderer();
> +    RenderElement* cellRenderer = renderer();

auto

> Source/WebCore/html/shadow/MeterShadowElement.cpp:75
> -    RenderObject* render = meterElement()->renderer();
> +    auto render = meterElement()->renderer();

I'm starting to really like this.
Using auto ensures that we automatically use tighter types if a covariant override is added in the future.

> Source/WebCore/loader/ImageLoader.cpp:321
> -    RenderObject* renderer = m_element->renderer();
> +    RenderElement* renderer = m_element->renderer();

auto

> Source/WebCore/page/FrameView.cpp:989
> -    RenderObject* frameOwnerRenderer = frame().ownerRenderer();
> +    RenderElement* frameOwnerRenderer = frame().ownerRenderer();

auto!
Frame::ownerRenderer() is actually a RenderWidget* so we are missing out on tightness here.

> Source/WebCore/page/FrameView.cpp:1946
> +    auto end = m_viewportConstrainedObjects->end();
> +    for (auto it = m_viewportConstrainedObjects->begin(); it != end; ++it)

Lame scoping of "end" here.

> Source/WebCore/page/animation/KeyframeAnimation.h:43
> +    static PassRefPtr<KeyframeAnimation> create(const Animation& animation, RenderElement* renderer, int index, CompositeAnimation* compositeAnimation, RenderStyle* unanimatedStyle)

More RefPtr.

> Source/WebCore/rendering/FilterEffectRenderer.h:113
> +    PassRefPtr<FilterEffect> buildReferenceFilter(RenderElement*, PassRefPtr<FilterEffect> previousEffect, ReferenceFilterOperation*);

More RefPtr.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:798
> -            RenderObject* clientForBackgroundImage = backgroundObject ? backgroundObject : this;
> +            RenderElement* clientForBackgroundImage = backgroundObject ? backgroundObject : this;

auto

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1783
> -    RenderObject* renderer = &layer->renderer();
> +    RenderElement* renderer = &layer->renderer();

auto
layer->renderer() is a RenderLayerModelObject here.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1813
> -    RenderObject* renderer = &layer->renderer();
> +    RenderElement* renderer = &layer->renderer();

Ditto.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1861
> -    RenderObject* renderer = &layer->renderer();
> +    RenderElement* renderer = &layer->renderer();

Ditto.

> Source/WebCore/rendering/RenderLayerCompositor.h:370
> +    bool requiresCompositingForAnimation(RenderElement*) const;
> +    bool requiresCompositingForTransform(RenderElement*) const;
> +    bool requiresCompositingForVideo(RenderElement*) const;
> +    bool requiresCompositingForCanvas(RenderElement*) const;
> +    bool requiresCompositingForPlugin(RenderElement*) const;
> +    bool requiresCompositingForFrame(RenderElement*) const;
> +    bool requiresCompositingForFilters(RenderElement*) const;
> +    bool requiresCompositingForBlending(RenderElement*) const;

I think all of these should really take const RenderLayerModelObject& instead of RenderElement*.

> Source/WebCore/rendering/RenderObject.cpp:-1988
> -    animation().cancelAnimations(this);

Nice.

[Andreas Kling]

Re: "More RefPtr", I started making comments about how we should return RefPtr instead of PassRefPtr, but then it got out of hand so I removed them. But not all of them, it seems :|

[Build Bot]

Comment on attachment 212935 [details]
Patch

Attachment 212935 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2705100

[Build Bot]

Comment on attachment 212935 [details]
Patch

Attachment 212935 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/2709091

[Build Bot]

Comment on attachment 212935 [details]
Patch

Attachment 212935 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2711075

[Darin Adler]

Working on a new patch now.

[Darin Adler]

Comment on attachment 212935 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212935&action=review

>> Source/WebCore/html/HTMLFormElement.cpp:103
>> +    RenderElement* parentRenderer = parent->renderer();
> 
> Is this change correct?
> HTMLFormElement could be a direct child of Document, in which case parentElement() would be null, no?

I thought that an HTMLFormElement could not be a direct child of Document, but then I read Document::childTypeAllowed, and it seems I was wrong. I’ll do this a different way.

[Darin Adler]

Created attachment 212940 [details]
Patch

[Andreas Kling]

Comment on attachment 212940 [details]
Patch

r=me

[Darin Adler]

Committed r156622: <http://trac.webkit.org/changeset/156622>

[Csaba Osztrogonác]

(In reply to comment #12)
> Committed r156622: <http://trac.webkit.org/changeset/156622>

It broke the Apple Windows build:

    1>..\..\win\WebFrame.cpp(1231): error C2664: 'WebCore::AnimationController::pauseAnimationAtTime' : cannot convert parameter 1 from 'WebCore::RenderObject *' to 'WebCore::RenderElement *'
                 Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast
     1>..\..\win\WebFrame.cpp(1250): error C2664: 'WebCore::AnimationController::pauseTransitionAtTime' : cannot convert parameter 1 from 'WebCore::RenderObject *' to 'WebCore::RenderElement *'
                 Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast

[Csaba Osztrogonác]

Unfortunately Windows EWS is very very sick and it seems nobody is really
interested in fixing it. :(

I noticed it a month before: https://lists.webkit.org/pipermail/webkit-dev/2013-September/025374.html

... but nothing happened ...

I prepared a workaround for it 3 days before: https://bugs.webkit.org/show_bug.cgi?id=122013

... but it is still waiting for review ...

Otherwise switching off buggy bots would be much more simpler task
than trying to land a workaround to opt-out buggy machines ...

[Timothy Hatcher]

This broke the Windows build.

http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/70962

[Csaba Osztrogonác]

Fixes landed in
 - http://trac.webkit.org/changeset/156655
 - http://trac.webkit.org/changeset/156667