Bug 122078

Summary: Use RenderElement instead of RenderObject in many places
Product: WebKit Reporter: Darin Adler <darin>
Component: Layout and RenderingAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, kling, ossy, rniwa, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 122106    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch kling: review+

Description Darin Adler 2013-09-29 14:48:21 PDT
Use RenderElement instead of RenderObject in many places
Comment 1 Darin Adler 2013-09-29 15:08:12 PDT
Created attachment 212934 [details]
Patch
Comment 2 Darin Adler 2013-09-29 15:10:53 PDT
Created attachment 212935 [details]
Patch
Comment 3 Andreas Kling 2013-09-29 15:40:12 PDT
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.
Comment 4 Andreas Kling 2013-09-29 15:41:10 PDT
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 :|
Comment 5 Build Bot 2013-09-29 15:43:05 PDT
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
Comment 6 Build Bot 2013-09-29 15:52:58 PDT
Comment on attachment 212935 [details]
Patch

Attachment 212935 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/2709091
Comment 7 Build Bot 2013-09-29 16:00:23 PDT
Comment on attachment 212935 [details]
Patch

Attachment 212935 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2711075
Comment 8 Darin Adler 2013-09-29 16:59:10 PDT
Working on a new patch now.
Comment 9 Darin Adler 2013-09-29 17:02:51 PDT
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.
Comment 10 Darin Adler 2013-09-29 18:37:40 PDT
Created attachment 212940 [details]
Patch
Comment 11 Andreas Kling 2013-09-29 18:55:21 PDT
Comment on attachment 212940 [details]
Patch

r=me
Comment 12 Darin Adler 2013-09-29 19:39:35 PDT
Committed r156622: <http://trac.webkit.org/changeset/156622>
Comment 13 Csaba Osztrogonác 2013-09-30 00:07:00 PDT
(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
Comment 14 Csaba Osztrogonác 2013-09-30 00:27:07 PDT
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 ...
Comment 15 Timothy Hatcher 2013-09-30 08:39:20 PDT
This broke the Windows build.

http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/70962
Comment 16 Csaba Osztrogonác 2013-09-30 12:09:23 PDT
Fixes landed in
 - http://trac.webkit.org/changeset/156655
 - http://trac.webkit.org/changeset/156667