Merge RenderWidget into single subclass, RenderPart

Merge RenderWidget into single subclass, RenderPart

The RenderWidget is an old abstraction that isn't needed any longer. We are
down to a single subclass, RenderPart. This CL folds RenderWidget into
RenderPart and updates its users.

BUG=416436
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183568

[Zeeshan Qureshi, 2014-10-03 20:57:00]

Patchset #1 (id:1) has been deleted

[Zeeshan Qureshi, 2014-10-07 01:03:31]

zeeshanq@chromium.org changed reviewers:
+ dsinclair@chromium.org, jchaffraix@chromium.org

[Zeeshan Qureshi, 2014-10-07 01:03:32]

Dan / Julien PTAL.

[dsinclair, 2014-10-07 14:15:12]

This is looking pretty good so far. Few small things below.

https://codereview.chromium.org/625073002/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:200: void updateWidgetPositions();
Should we update the names of these as they aren't really dealing with widgets
anymore.

Although, I'm not sure what a better name would be?

https://codereview.chromium.org/625073002/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:434:
WillBePersistentHeapHashSet<RefPtrWillBeMember<RenderPart> > m_widgets;
Similarily, should we rename this to something else now?

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/H...
File Source/core/rendering/HitTestResult.h (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/H...
Source/core/rendering/HitTestResult.h:150: bool m_isOverWidget; // Returns true
if we are over a widget (and not in the border/padding area of a RenderPart for
example).
Does this mean is over a RenderWidget, or is over a Widget? If it's the former,
we should rename it to be consistent with the render part rename.

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:637: // FIXME: Why does RenderPart need
this?
Can you file a bug for this and add the crbug.com/ reference here? (Feel free to
add the Project-Warden label)


This is called from RenderWidget (now RenderPart) :: destroy(). Why does render
widget, and only render widget, need to clear the node on destroy? Is something
accessing the node after we destroyed the object?

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderPart.cpp (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.cpp:45: // Reference counting is used to
prevent the widget from being
nit: s/widget/part ?

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.cpp:210: if (widget) {
if (!widget)
    return;

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.cpp:345: // if the frame bounds got changed, or
if view needs layout (possibly indicating
nit: Capitalize the first letter of a sentence. And a period at the end.

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderPart.h (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.h:56: virtual bool isWidget() const OVERRIDE
FINAL { return true; }
Is this going to be removed? Can we file a followup bug to remove these?
(Assuming this does mean is Render Widget?)

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.h:78: // TODO(zeeshanq): This was previously
RenderWidget::nodeAtPoint(), find a better name.
From the usage, it looks like nodeAtPointInternal to me, yea?

[Julien - ping for review, 2014-10-07 17:53:23]

https://codereview.chromium.org/625073002/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:200: void updateWidgetPositions();
On 2014/10/07 at 14:15:12, dsinclair wrote:
> Should we update the names of these as they aren't really dealing with widgets
anymore.
> 
> Although, I'm not sure what a better name would be?

Some of the other functions were updated with s/Widget/Part/ and I think that
would be a good direction here too.

https://codereview.chromium.org/625073002/diff/100001/Source/core/html/HTMLPl...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:231: // return a RenderEmbeddedObject,
RenderPart or even RenderPart.
Maybe a bit redundant: RenderPart or even RenderPart :-)

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderMeter.h (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderMeter.h:25: #include "core/rendering/RenderPart.h"
Do we really need this #include?

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderPart.cpp (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.cpp:48: , m_refCount(1)
I wonder if we really need this refcount as we don't really have native widgets.
That would be a nice preparatory change (less code to move).

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.cpp:262: Widget* widget = this->widget();
That should be folded in the if below:

if (Widget* widget = this->widget())
    paintContents(paintInfo, paintOffset);

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderPart.h (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.h:79: bool widgetAtPoint(const HitTestRequest&,
HitTestResult&, const HitTestLocation& locationInContainer, const LayoutPoint&
accumulatedOffset, HitTestAction);
Not a fan of this name as I would expect that API to return a Widget, not a
boolean. It matches nodeAtPoint so I would keep a closer name like
nodeAtPointOverWidget.

[Zeeshan Qureshi, 2014-10-10 19:32:39]

Thanks Julien / Dan, I've updated the references wherever it seemed reasonable.

Still going through the changes to see if there's other indirect references to
RenderPart that need to be updated.

https://codereview.chromium.org/625073002/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:200: void updateWidgetPositions();
On 2014/10/07 17:53:22, Julien Chaffraix - PST wrote:
> On 2014/10/07 at 14:15:12, dsinclair wrote:
> > Should we update the names of these as they aren't really dealing with
widgets
> anymore.
> > 
> > Although, I'm not sure what a better name would be?
> 
> Some of the other functions were updated with s/Widget/Part/ and I think that
> would be a good direction here too.

After going through the methods, I've updated the ones that were referencing
RenderPart but kept the ones that were referring to the widget inside
RenderPart.

https://codereview.chromium.org/625073002/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:434:
WillBePersistentHeapHashSet<RefPtrWillBeMember<RenderPart> > m_widgets;
On 2014/10/07 14:15:12, dsinclair wrote:
> Similarily, should we rename this to something else now?

Done. Also for RenderEmbeddeObject as it's a subclass of RenderPart.

https://codereview.chromium.org/625073002/diff/100001/Source/core/html/HTMLPl...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:231: // return a RenderEmbeddedObject,
RenderPart or even RenderPart.
On 2014/10/07 17:53:22, Julien Chaffraix - PST wrote:
> Maybe a bit redundant: RenderPart or even RenderPart :-)

Ah, yes! I remember doing this in a commit but probably lost when reverting some
breaking changes.

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/H...
File Source/core/rendering/HitTestResult.h (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/H...
Source/core/rendering/HitTestResult.h:150: bool m_isOverWidget; // Returns true
if we are over a widget (and not in the border/padding area of a RenderPart for
example).
On 2014/10/07 14:15:12, dsinclair wrote:
> Does this mean is over a RenderWidget, or is over a Widget? If it's the
former,
> we should rename it to be consistent with the render part rename.

This is referencing an actual Widget as far as I can see from the usage.

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderMeter.h (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderMeter.h:25: #include "core/rendering/RenderPart.h"
On 2014/10/07 17:53:22, Julien Chaffraix - PST wrote:
> Do we really need this #include?

Nope, good catch, thanks!

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:637: // FIXME: Why does RenderPart need
this?
On 2014/10/07 14:15:12, dsinclair wrote:
> Can you file a bug for this and add the crbug.com/ reference here? (Feel free
to
> add the Project-Warden label)
> 
> 
> This is called from RenderWidget (now RenderPart) :: destroy(). Why does
render
> widget, and only render widget, need to clear the node on destroy? Is
something
> accessing the node after we destroyed the object?

http://crbug.com/422457

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderPart.cpp (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.cpp:45: // Reference counting is used to
prevent the widget from being
On 2014/10/07 14:15:12, dsinclair wrote:
> nit: s/widget/part ?

Done. Looks like this was the only replacement needed, every other reference is
to the internal Widget*

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.cpp:48: , m_refCount(1)
On 2014/10/07 17:53:22, Julien Chaffraix - PST wrote:
> I wonder if we really need this refcount as we don't really have native
widgets.
> That would be a nice preparatory change (less code to move).

I don't know enough about how this works, maybe follow up with a later patch?

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.cpp:210: if (widget) {
On 2014/10/07 14:15:12, dsinclair wrote:
> if (!widget)
>     return;

Done.

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.cpp:262: Widget* widget = this->widget();
On 2014/10/07 17:53:22, Julien Chaffraix - PST wrote:
> That should be folded in the if below:
> 
> if (Widget* widget = this->widget())
>     paintContents(paintInfo, paintOffset);

Done.

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.cpp:345: // if the frame bounds got changed, or
if view needs layout (possibly indicating
On 2014/10/07 14:15:12, dsinclair wrote:
> nit: Capitalize the first letter of a sentence. And a period at the end.

Done.

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderPart.h (right):

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.h:56: virtual bool isWidget() const OVERRIDE
FINAL { return true; }
On 2014/10/07 14:15:12, dsinclair wrote:
> Is this going to be removed? Can we file a followup bug to remove these?
> (Assuming this does mean is Render Widget?)

Done. Since we're renaming and updating a lot more callers now, just easier to
do this here.

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.h:78: // TODO(zeeshanq): This was previously
RenderWidget::nodeAtPoint(), find a better name.
On 2014/10/07 14:15:12, dsinclair wrote:
> From the usage, it looks like nodeAtPointInternal to me, yea?

Done.

https://codereview.chromium.org/625073002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderPart.h:79: bool widgetAtPoint(const HitTestRequest&,
HitTestResult&, const HitTestLocation& locationInContainer, const LayoutPoint&
accumulatedOffset, HitTestAction);
On 2014/10/07 17:53:23, Julien Chaffraix - PST wrote:
> Not a fan of this name as I would expect that API to return a Widget, not a
> boolean. It matches nodeAtPoint so I would keep a closer name like
> nodeAtPointOverWidget.

nodeAtPointOverWidget sounds good.

[dsinclair, 2014-10-10 19:50:39]

A couple small issues, but other then those lgtm.

https://codereview.chromium.org/625073002/diff/120001/Source/core/html/HTMLEm...
File Source/core/html/HTMLEmbedElement.cpp (right):

https://codereview.chromium.org/625073002/diff/120001/Source/core/html/HTMLEm...
Source/core/html/HTMLEmbedElement.cpp:55: static inline RenderPart*
findWidgetRenderer(const Node* n)
I think this one should be findPartRenderer

https://codereview.chromium.org/625073002/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/625073002/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:636: // FIXME: Why does RenderPart need
this?
Sorry, I meant, can you stick the crbug on to the end of the FIXME. Makes it
easy in the future if you're looking at the bug to find the places in the code
by grepping for the id.

[Zeeshan Qureshi, 2014-10-10 20:24:22]

zeeshanq@chromium.org changed reviewers:
+ aelias@chromium.org

[Zeeshan Qureshi, 2014-10-10 20:24:23]

+aelias for Source/web OWNERS

https://codereview.chromium.org/625073002/diff/120001/Source/core/html/HTMLEm...
File Source/core/html/HTMLEmbedElement.cpp (right):

https://codereview.chromium.org/625073002/diff/120001/Source/core/html/HTMLEm...
Source/core/html/HTMLEmbedElement.cpp:55: static inline RenderPart*
findWidgetRenderer(const Node* n)
On 2014/10/10 19:50:39, dsinclair wrote:
> I think this one should be findPartRenderer

Done.

https://codereview.chromium.org/625073002/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/625073002/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:636: // FIXME: Why does RenderPart need
this?
On 2014/10/10 19:50:39, dsinclair wrote:
> Sorry, I meant, can you stick the crbug on to the end of the FIXME. Makes it
> easy in the future if you're looking at the bug to find the places in the code
> by grepping for the id.

Done.

[Zeeshan Qureshi, 2014-10-10 20:30:24]

Patchset #7 (id:380001) has been deleted

[aelias_OOO_until_Jul13, 2014-10-10 20:41:21]

Source/web lgtm

[Zeeshan Qureshi, 2014-10-10 20:47:26]

The CQ bit was checked by zeeshanq@chromium.org

[Zeeshan Qureshi, 2014-10-10 20:47:38]

The CQ bit was unchecked by zeeshanq@chromium.org

[dsinclair, 2014-10-10 22:16:02]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2014-10-10 22:16:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625073002/430001

[commit-bot: I haz the power, 2014-10-10 23:24:44]

Committed patchset #7 (id:430001) as 183568