Bug 17811 - Widget class refactoring
Summary: Widget class refactoring
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2008-03-12 14:52 PDT by Rodney Dawes
Modified: 2012-11-02 14:22 PDT (History)
1 user (show)

See Also:

Patch to refactor the Widget class (42.81 KB, patch)
2008-03-12 14:53 PDT, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch to fix tabs vs. spaces (42.83 KB, patch)
2008-03-13 07:01 PDT, Rodney Dawes
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rodney Dawes 2008-03-12 14:52:18 PDT
Currently, the Widget class is split up with several #if PLATFORM() statements, with duplicate code across many platforms, as well as a circular dependency on the derived class ScrollView. This patch is intended to clean up the circular dependence and provide shared methods for the many duplicate methods, rather than copying the code between platforms.
Comment 1 Rodney Dawes 2008-03-12 14:53:11 PDT
Created attachment 19717 [details]
Patch to refactor the Widget class
Comment 2 Rodney Dawes 2008-03-13 07:01:53 PDT
Created attachment 19724 [details]
Updated patch to fix tabs vs. spaces

Oops. Forgot to fix the issues where tabs came up instead of spaces last night when submitting the patch. This revision fixes that.
Comment 3 Darin Adler 2008-03-13 09:56:10 PDT
Comment on attachment 19724 [details]
Updated patch to fix tabs vs. spaces

This won't compile on Mac. There are two implementations of Widget::frameGeometry.

Having a m_frameRect is wrong for PlatformScrollBarQt and for Mac widgets. And having that field there, but unused, on those platforms is a recipe for disaster.

We need to find a different way to share this code.

I'm really sad to see so much use of virtual functions. Our normal cross-platform strategy is to use compile time #if, not virtual functions.

Changing setParent to take a Widget* is a bad idea. That's going in the wrong direction. The only kind of parent we support is a FrameView*. We don't need a general purpose "make any widget a child of any other widget" call.

I didn't get a chance to review this whole thing, but I think we need to do this refactoring in smaller pieces rather than all at once, and keep all the platforms working.
Comment 4 Eric Seidel (no email) 2012-11-02 14:22:49 PDT
I suspect this patch/bug is obsolete by now and can be closed?