Bug 17324 - Remove PLATFORM ifdefs from ContextMenu.cpp
Summary: Remove PLATFORM ifdefs from ContextMenu.cpp
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Christian Dywan
Keywords: Gtk
Depends on:
Reported: 2008-02-12 01:28 PST by Eric Seidel (no email)
Modified: 2008-06-07 11:19 PDT (History)
2 users (show)

See Also:

Move webkit code to ContextMenuClient (12.37 KB, patch)
2008-06-07 09:31 PDT, Christian Dywan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-02-12 01:28:13 PST
Remove PLATFORM(GTK) ifdefs from ContextMenu.cpp


platform/ContextMenu.cpp:49:#if PLATFORM(GTK)
platform/ContextMenu.cpp:81:#if PLATFORM(MAC)
platform/ContextMenu.cpp:88:#if PLATFORM(MAC)
platform/ContextMenu.cpp:93:#if PLATFORM(MAC)
platform/ContextMenu.cpp:100:#if PLATFORM(MAC)
platform/ContextMenu.cpp:151:#if PLATFORM(MAC)
platform/ContextMenu.cpp:166:#if PLATFORM(GTK)
platform/ContextMenu.cpp:299:#if PLATFORM(MAC)
platform/ContextMenu.cpp:323:#if PLATFORM(GTK)
platform/ContextMenu.cpp:333:#if PLATFORM(GTK)
platform/ContextMenu.cpp:367:#if PLATFORM(MAC)
platform/ContextMenu.cpp:372:#if PLATFORM(MAC)
platform/ContextMenu.cpp:379:#if PLATFORM(GTK)
platform/ContextMenu.cpp:457:#if PLATFORM(MAC)
platform/ContextMenu.cpp:463:#if PLATFORM(MAC)
platform/ContextMenu.cpp:472:#if PLATFORM(GTK)
platform/ContextMenu.cpp:495:#if PLATFORM(MAC)
platform/ContextMenu.cpp:501:#if PLATFORM(GTK)
platform/ContextMenu.cpp:590:#if PLATFORM(GTK)
platform/ContextMenu.cpp:652:#if PLATFORM(GTK)
Comment 1 Alp Toker 2008-02-12 01:32:20 PST
Agreed. ContextMenu.cpp needs to be split up and put into the platform layers, though we still need to allow a mechanism for WebCore to insert menu entries as well.
Comment 2 Christian Dywan 2008-06-07 09:31:55 PDT
Created attachment 21560 [details]
Move webkit code to ContextMenuClient

This patch moves code that needs access to webkit to ContextMenuClientGtk.
Comment 3 Eric Seidel (no email) 2008-06-07 10:36:17 PDT
Comment on attachment 21560 [details]
Move webkit code to ContextMenuClient

Assuming the code still works, the move looks fine.

new WebKit::ContextMenuClient (webView)

seems to have an extra space.

+        WebKitWebView *m_webView;

Is in a WebCore file, but puts the * in the opposite place as the rest of WebCore. :)

It would be nice to land with those to changes, but in general looks fine.
Comment 4 Adam Roben (:aroben) 2008-06-07 10:56:41 PDT
Comment on attachment 21560 [details]
Move webkit code to ContextMenuClient

It seems to me that a better approach would be to move the menu items into ContextMenu{Gtk,Mac,Win,Wx,Qt}.cpp, instead of moving the logic up into WebKit. WebKit's for the most part supposed to be a thin API layer that wraps WebCore, so I don't think moving this kind of logic there is the right direction to go in.
Comment 5 Christian Dywan 2008-06-07 11:16:28 PDT
Landed in r34426, with style fixes as suggested by Eric.
Comment 6 Christian Dywan 2008-06-07 11:19:16 PDT
Comment on attachment 21560 [details]
Move webkit code to ContextMenuClient

Clearing review flag.