Bug 21511 – Add some PLATFORM(CHROMIUM) ifdefs to WebCore

• Component: Platform
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

Add some PLATFORM(CHROMIUM) ifdefs to WebCore

This will be done in two parts.  The first part adds ifdefs to WebCore outside of the platform directory.  The second part adds them to platform.

[Darin Fisher (:fishd, Google)]

Created attachment 24247 [details]
part 1

[Darin Fisher (:fishd, Google)]

I broke the pan scrolling business out into a separate bug.  See bug 21515.

[Darin Fisher (:fishd, Google)]

Created attachment 24257 [details]
patch - all but platform/graphics

This covers everything except platform/graphics.

[Darin Fisher (:fishd, Google)]

Summary of changes:

loader/FTP*
 - trivial: should be using WIN_OS instead of WIN

loader/FrameLoader.cpp
 - extending an existing PLATFORM(WIN) ifdef

platform/ContextMenuItem.h
 - Chromium doesn't need anything here, so a default PlatformMenuItemDescription is sufficient.

platform/Cursor.h
 - PlatformCursor is defined in a separate header file.

platform/DragData.h
 - DragDataRef is defined in a separate header file.

platform/DragImage.h
 - DragImageRef is defined in a separate header file.

platform/Pasteboard.h
 - PasteboardPrivate is used to contain all private members for the Chromium port.

platform/PlatformKeyboardEvent.h
 - isSystemKey is shared with PLATFORM(WIN).
 - private becomes protected to support Chromium's approach to using a subclass of PlatformKeyboardEvent in order to initialize a PlatformKeyboardEvent object.

platform/PlatformMenuDescription.h
 - Chromium doesn't need anything here, so a default PlatformMenuDescription is sufficient.

platform/PlatformMouseEvent.h
platform/PlatformWheelEvent.h
 - Same private to protected change as done for PlatformKeyboardEvent

platform/PopupMenu.h
 - PopupMenuPrivate is used to contain all private members for the Chromium port.

platform/ScrollView.h
 - turns out this PLATFORM(WIN) ifdef was unnecessary.

platform/Widget.h
 - PlatformWidget is defined in a separate header file.

platform/network/NetworkStateNotifier.h
 - NetworkStateNotifierPrivate is used to contain all private members for the Chromium port.

[Darin Adler]

Comment on attachment 24257 [details]
patch - all but platform/graphics

 #elif PLATFORM(WX)
     typedef wxMenuItem* PlatformMenuItemDescription;
+#else
+    typedef void* PlatformMenuItemDescription;
 #endif

Yuck. Can we do better here? I see that Cursor.h does this already, but I don't understand why that's OK. It seems that just using an untyped pointer is not a great solution.

+#elif PLATFORM(CHROMIUM)
+#include "PlatformCursor.h"
 #endif

If putting the platform specifics into a separate header file is a better approach, we should be breaking out those cascading #ifs into separate files for all the other platforms too instead of just doing it for Chromium.

+#if PLATFORM(CHROMIUM)
+#include "PasteboardPrivate.h"
+#endif

Same comment. It seems that we're settling for improving things for Chromium without making the same improvements for the other platforms, which is OK if we just want to do the minimum. But I think it's not as good for the project as a whole as it would be to make the platforms all work that way.

Index: platform/PlatformMouseEvent.h
===================================================================
--- platform/PlatformMouseEvent.h	(revision 37449)
+++ platform/PlatformMouseEvent.h	(working copy)
@@ -137,7 +137,7 @@ namespace WebCore {
 #endif
 
-    private:
+    protected:

Same comment again. It seems you are advocating a new design here to avoid the per-platform constructors in the header: make data members protected and use a derived class just for construction.

But this patch does this new way only for Chromium, and with no comment in the header explaining the somewhat subtle technique. I'd much prefer a patch that does this for all platforms. And also, I think adding a protected inline init function would be better than just making all the data members protected.

r=me

I'll reluctantly allow that we can do these changes exactly as-is, but I'd like to see an approach where improvements to the cross-platform approach are done in a way that helps everybody, not just one platform.

[Darin Fisher (:fishd, Google)]

Thank you for being so accommodating.  I am very interested in changing all of the ports to match the pattern I'm advocating for Chromium, but I figure that it would best for me to demonstrate the full approach with Chromium first.  I plan on filing follow-up bugs for cleaning up the other ports.  It is just a bit beyond me at this point to try to move all of the ports to a new system.  Incremental seems best at this point.

[Sam Weinig]

It seems odd to me that this patch includes #includes for files that don't exist.  I would rather we waited to add those #includes until the files existed.

[Adam Barth]

Is this good to land or are we waiting for Sam's previous comment to be addressed?  Assigning to Sam for resolution.

[Darin Fisher (:fishd, Google)]

I spoke with Sam on #webkit.  He's cool with the patch as-is.  I'm going to land this once I get commit access (which I may already have, but I need to find bdash first).

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/37599

going to punt platform/graphics to another bug.