Remove Widget's dependency upon its own inheritor aka ScrollView. This was nasty from an OO design…

Remove Widget's dependency upon its own inheritor aka ScrollView


This was nasty from an OO design perspective and also complicates
efforts to move ScrollView and Scrollbar to core/page/.

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

[atreat, 2013-10-10 22:06:26]

Remove a nasty dependency with Widget referring to its own child. Yuck!

This also makes it much easier to separate out Widget from ScrollView and
disentangle them so ScrollView can move to core/page.

[abarth-chromium, 2013-10-11 05:54:03]

I'm not the right person to review this CL.  Maybe Eric can review it?

[eseidel, 2013-10-11 06:01:32]

I'm not entirely sure I follow.

ScrollView is to ContainerNode while Widget is to Node.

It's nice for Node to not know anything about it's subclass which can contain
things.  But it's not clear that this CL is the cleanest way to do that.

Maybe I just need to read it again when I'm less tired. :)

The other factor to consider is that with out of process iframes, this whole
concept is going way.  The widget tree makes no sense if there is only one
Widget. :)  I think we should just wrap plugins in frames and put them in the
FrameTree and forget all this Widget siliness. :)

https://codereview.chromium.org/26936002/diff/1/Source/core/page/FrameView.cpp
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/26936002/diff/1/Source/core/page/FrameView.cp...
Source/core/page/FrameView.cpp:3201: if (const ScrollView* parentScrollView =
static_cast<ScrollView*>(parent())) {
toScrollView? or parentScrollView?

[eseidel, 2013-10-11 06:02:53]

Sorry the Node/ContainerNode analogy may not be very clear if you're not
familiar with the DOM.

Widget is the dumb leaf node class.  And ScrollView is the smarter Widget which
can contain children.

This whole concept mostly exists for historical reasons and really just needs to
die. :)

[abarth-chromium, 2013-10-11 06:07:24]

On 2013/10/11 06:02:53, eseidel wrote:
> This whole concept mostly exists for historical reasons and really just needs
to
> die. :)

Can we actually remove it or are there blocking issues?

[eseidel, 2013-10-11 06:12:25]

Removing widgets requires some thought.  But I do believe it's possible.

Widgets are ref-counted and plugins can hold onto them (or at least could in the
Mac port?)

Widgets used to hold scrollbars and resize corners in their tree, I believe
neither exist anymore though.

I think we should just try and move Plugins into Frames in the FrameTree and
ScrollBars into the LayerTree and then just delete Widget.  But I don't feel
like I've stared at the problem enough to write down an exact plan for someone.

[atreat, 2013-10-11 14:26:02]

Kenneth has also expressed interest in removing Widget altogether, but I 
think that is
beyond the scope of this patch and the motivation behind it.  This patch 
is a prerequisite
to moving PlatformScreen to Source/platform as it depends upon Widget 
and in turn upon
ScrollView which is encumbered by many layering violations.

I am hoping to land this patch in order to move Widget.* to 
Source/platform and then
move PlatformScreen.  I plan on moving ScrollView and Scrollbar into 
core/page.

Have a look at PlatformScreen where it relies upon ScrollView right 
now.  This patch
removes that.

On 10/11/2013 02:12 AM, eseidel@chromium.org wrote:
> Removing widgets requires some thought.  But I do believe it's possible.
>
> Widgets are ref-counted and plugins can hold onto them (or at least 
> could in the
> Mac port?)
>
> Widgets used to hold scrollbars and resize corners in their tree, I 
> believe
> neither exist anymore though.
>
> I think we should just try and move Plugins into Frames in the 
> FrameTree and
> ScrollBars into the LayerTree and then just delete Widget.  But I 
> don't feel
> like I've stared at the problem enough to write down an exact plan for 
> someone.
>
> https://codereview.chromium.org/26936002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[atreat, 2013-10-11 15:17:16]

New cl for your review.  This one cleans up a bit by using helper cast
toScrollView in FrameView.

[atreat, 2013-10-11 15:20:44]

Adding Julien Chaffraix who might be interested.

[eseidel, 2013-10-11 15:27:22]

https://codereview.chromium.org/26936002/diff/19001/Source/core/platform/Widg...
File Source/core/platform/Widget.h (left):

https://codereview.chromium.org/26936002/diff/19001/Source/core/platform/Widg...
Source/core/platform/Widget.h:125: virtual IntRect convertToContainingView(const
IntRect&) const;
If ScrollView is always the container, why use Widget here?

https://codereview.chromium.org/26936002/diff/19001/Source/core/platform/Widg...
File Source/core/platform/Widget.h (right):

https://codereview.chromium.org/26936002/diff/19001/Source/core/platform/Widg...
Source/core/platform/Widget.h:48: // scrollbars are always leaves of the tree.
Only Widgets can have children
"Only ScrollViews can have children" rather...

https://codereview.chromium.org/26936002/diff/19001/Source/core/platform/Widg...
Source/core/platform/Widget.h:95: Widget* parent() const { return m_parent; }
Making these SrollView* is fine.  It just means that callers always have to
include ScrollView.h if they want to do anything with the parent.  Are there
callers who don't want that?

Having your parent be a special class (which you yourself don't know about)
seems OK to me:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[atreat, 2013-10-11 15:34:23]

On 2013/10/11 15:27:22, eseidel wrote:
>
https://codereview.chromium.org/26936002/diff/19001/Source/core/platform/Widg...
> File Source/core/platform/Widget.h (left):
> 
>
https://codereview.chromium.org/26936002/diff/19001/Source/core/platform/Widg...
> Source/core/platform/Widget.h:125: virtual IntRect
convertToContainingView(const
> IntRect&) const;
> If ScrollView is always the container, why use Widget here?
> 
>
https://codereview.chromium.org/26936002/diff/19001/Source/core/platform/Widg...
> File Source/core/platform/Widget.h (right):
> 
>
https://codereview.chromium.org/26936002/diff/19001/Source/core/platform/Widg...
> Source/core/platform/Widget.h:48: // scrollbars are always leaves of the tree.
> Only Widgets can have children
> "Only ScrollViews can have children" rather...
> 
>
https://codereview.chromium.org/26936002/diff/19001/Source/core/platform/Widg...
> Source/core/platform/Widget.h:95: Widget* parent() const { return m_parent; }
> Making these SrollView* is fine.  It just means that callers always have to
> include ScrollView.h if they want to do anything with the parent.  Are there
> callers who don't want that?
> 
> Having your parent be a special class (which you yourself don't know about)
> seems OK to me:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

If I leave ScrollView* in the Widget.h header, then Widget.cpp will need to use
ScrollView and include ScrollView.h and thus have a dependency upon ScrollView
which has layering violations.  That means PlatformScreen.h will have to include
Widget.h and thus can not be moved to Source/platform.

The motivation here is to do this so I can move Widget.* to Source/platform so
that I can move PlatformScreen.

The other option if I want to move PlatformScreen is to get rid of Widget.*
altogether as we have discussed, but this is a longer term effort and would
block blink_platform.dll in the meantime.

Hope that makes the motivation clear :)

[eseidel, 2013-10-11 15:36:43]

Yeah.  I understand your reasons.  Please fix the comment nits above.

I accept this as an intermediate state towards the death of Widget.

[eseidel, 2013-10-11 15:37:00]

Yeah.  I understand your reasons.  Please fix the comment nits above.

I accept this as an intermediate state towards the death of Widget.

[atreat, 2013-10-11 15:44:23]

On 2013/10/11 15:37:00, eseidel wrote:
> Yeah.  I understand your reasons.  Please fix the comment nits above.
> 
> I accept this as an intermediate state towards the death of Widget.

Done.  Onwards to killing Widget! :)

[eseidel, 2013-10-11 15:55:23]

lgtm

Kinda ugly, but I accept this as an intermediate step.

[eseidel, 2013-10-11 15:55:35]

Thanks for driving this Adam!

[commit-bot: I haz the power, 2013-10-11 15:55:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adam.treat@samsung.com/26936002/26001

[commit-bot: I haz the power, 2013-10-11 16:30:24]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-11 18:11:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adam.treat@samsung.com/26936002/36001

[commit-bot: I haz the power, 2013-10-11 18:13:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adam.treat@samsung.com/26936002/36001

[commit-bot: I haz the power, 2013-10-11 23:10:10]

Change committed as 159483

[Nico, 2013-10-28 18:39:46]

Should the setParent() in RenderScrollbar.h have its parameter changed too? (and
marked OVERRIDE while there?)

[atreat, 2013-10-28 18:51:08]

On 2013/10/28 18:39:46, Nico wrote:
> Should the setParent() in RenderScrollbar.h have its parameter changed too?
(and
> marked OVERRIDE while there?)

Ouch. Fixing here: https://codereview.chromium.org/48733005/