Use RenderElement instead of RenderObject in many places
Created attachment 212934 [details] Patch
Created attachment 212935 [details] Patch
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.
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 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 on attachment 212935 [details] Patch Attachment 212935 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/2709091
Comment on attachment 212935 [details] Patch Attachment 212935 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2711075
Working on a new patch now.
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.
Created attachment 212940 [details] Patch
Comment on attachment 212940 [details] Patch r=me
Committed r156622: <http://trac.webkit.org/changeset/156622>
(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
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 ...
This broke the Windows build. http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/70962
Fixes landed in - http://trac.webkit.org/changeset/156655 - http://trac.webkit.org/changeset/156667