[webkit-reviews] review granted: [Bug 21511] Add some PLATFORM(CHROMIUM) ifdefs to WebCore : [Attachment 24257] patch - all but platform/graphics

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Darin Fisher (Google)
<darin at chromium.org>'s request for review:
Bug 21511: Add some PLATFORM(CHROMIUM) ifdefs to WebCore
https://bugs.webkit.org/show_bug.cgi?id=21511

Attachment 24257: patch - all but platform/graphics
https://bugs.webkit.org/attachment.cgi?id=24257&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
 #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.