Wrap Context Menu code in an ENABLE wrapper. This is for platforms (such as iPhone) that do not support context menus.
Created attachment 39518 [details] This patch implements the needed changes This will compile and link Mac OS X without Context Menu support. Of course Mac OS X (and many other platforms) will never ship in this configuration, so should the Mac code be conditionally compiled for this? I also left in the oncontextmenu event / attribute, since it just doesn't do anything on iPhone. Is this best?
Comment on attachment 39518 [details] This patch implements the needed changes As I mentioned in another bug, it doesn't make sense to set ENABLE_CONTEXT_MENU in both wtf/Platform.h and FeatureDefines.xcconfig. Since this is a platform difference and not a feature, the former makes more sense.
Created attachment 39552 [details] Address Mark's comments. Removes the FeatureDefines.xcconfig changes, and the build-webkit changes, since this is Platform related as well.
<rdar://problem/6732593>
The DerivedSources.make change still assumes this is set in FEATURE_DEFINES.
Created attachment 39558 [details] Fix DerivedSources.make Thanks!
Created attachment 39559 [details] Fix DerivedSources.make for all platforms, not just MacOS
This is being conditionalized because it is not enabled on iPhone.
Comment on attachment 39559 [details] Fix DerivedSources.make for all platforms, not just MacOS In general it seems that the plural form of ENABLE(CONTEXT_MENUS) would read better than ENABLE(CONTEXT_MENU). > diff --git a/WebKit/mac/WebView/WebUIDelegatePrivate.h b/WebKit/mac/WebView/WebUIDelegatePrivate.h > index eadc761..a28ba5a 100644 > --- a/WebKit/mac/WebView/WebUIDelegatePrivate.h > +++ b/WebKit/mac/WebView/WebUIDelegatePrivate.h > @@ -107,7 +107,9 @@ enum { > // FIXME: If we ever make this method public, it should include a WebFrame parameter. > - (BOOL)webViewShouldInterruptJavaScript:(WebView *)sender; > - (void)webView:(WebView *)sender willPopupMenu:(NSMenu *)menu; > +#if ENABLE_CONTEXT_MENU > - (void)webView:(WebView *)sender contextMenuItemSelected:(NSMenuItem *)item forElement:(NSDictionary *)element; > +#endif > - (void)webView:(WebView *)sender saveFrameView:(WebFrameView *)frameView showingPanel:(BOOL)showingPanel; > > /*! This header file is used from outside of WebKit. In that context ENABLE_CONTEXT_MENU will not be defined which will result in this method being omitted. That doesn't seem like the behavior we're after here. I suspect we'll need to wrap the header in something like: #if !defined(ENABLE_CONTEXT_MENUS) #define ENABLE_CONTEXT_MENUS 1 #define DEFINED_ENABLE_CONTEXT_MENUS 1 #endif [ .. ] #if DEFINED_ENABLE_CONTEXT_MENUS #undef ENABLE_CONTEXT_MENUS #undef DEFINED_ENABLE_CONTEXT_MENUS #endif I think we do something similar for dashboard support in some WebKit headers.
Alternately, should this be applicable to Mac OS X WebKit at all? I agree with the ENABLE_CONTEXT_MENUS plural change. Adding a new patch shortly.
Created attachment 39624 [details] Uses ENABLE_CONTEXT_MENUS This leaves the WebKit changes as is; I'm not sure if it is best to patch Mac OS X for this, since Mac OS X will always ship with this on.
Comment on attachment 39624 [details] Uses ENABLE_CONTEXT_MENUS > + * DerivedSources.make: Use new WebCore.ContextMenu.exp file if ENABLE_CONTEXT_MENUS. > + * WebCore.base.exp: Move ContextMenu only exports to WebCore.ContextMenu.exp. > + * WebCore.xcodeproj/project.pbxproj: Add WebCore.ContextMenu.exp. I think "WebCore.ContextMenu.exp" should be renamed to "WebCore.ContextMenus.exp" to match the plurality of the ENABLE(CONTEXT_MENUS) macro. > +ifeq ($(ENABLE_CONTEXT_MENUS), 1) > + WEBCORE_EXPORT_DEPENDENCIES := $(WEBCORE_EXPORT_DEPENDENCIES) WebCore.ContextMenu.exp > +endif Ditto. > diff --git a/WebCore/WebCore.ContextMenu.exp b/WebCore/WebCore.ContextMenu.exp Ditto. > + FEFD102C105C41470002855E /* WebCore.ContextMenu.exp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.exports; path = WebCore.ContextMenu.exp; sourceTree = "<group>"; }; Ditto. > + FEFD102C105C41470002855E /* WebCore.ContextMenu.exp */, Ditto. > diff --git a/WebCore/page/ContextMenuController.cpp b/WebCore/page/ContextMenuController.cpp > index 0ec9d1c..063978d 100644 > --- a/WebCore/page/ContextMenuController.cpp > +++ b/WebCore/page/ContextMenuController.cpp > @@ -26,6 +26,7 @@ > #include "config.h" > #include "ContextMenuController.h" > > +#if ENABLE(CONTEXT_MENUS) > #include "Chrome.h" > #include "ContextMenu.h" > #include "ContextMenuClient.h" > @@ -335,3 +336,4 @@ void ContextMenuController::contextMenuItemSelected(ContextMenuItem* item) > } > > } // namespace WebCore > +#endif // ENABLE(CONTEXT_MENUS) Please add a blank line after the "#if ENABLE(.." statement and before the "#endif // ..." statement. > diff --git a/WebCore/platform/ContextMenu.cpp b/WebCore/platform/ContextMenu.cpp > index cca11b5..5cf6131 100644 > --- a/WebCore/platform/ContextMenu.cpp > +++ b/WebCore/platform/ContextMenu.cpp > @@ -27,6 +27,7 @@ > #include "config.h" > #include "ContextMenu.h" > > +#if ENABLE(CONTEXT_MENUS) > #include "ContextMenuController.h" > #include "ContextMenuClient.h" > #include "CSSComputedStyleDeclaration.h" > @@ -782,3 +783,4 @@ void ContextMenu::checkOrEnableIfNeeded(ContextMenuItem& item) const > } > > } > +#endif // ENABLE(CONTEXT_MENUS) Ditto for blank lines. Adding a comment for the "}" used to end the WebCore namespace would be nice, too, but not necessary. > diff --git a/WebCore/platform/mac/ContextMenuItemMac.mm b/WebCore/platform/mac/ContextMenuItemMac.mm > index c5e2942..531c834 100644 > --- a/WebCore/platform/mac/ContextMenuItemMac.mm > +++ b/WebCore/platform/mac/ContextMenuItemMac.mm > @@ -26,6 +26,7 @@ > #include "config.h" > #include "ContextMenuItem.h" > > +#if ENABLE(CONTEXT_MENUS) > #include "ContextMenu.h" > > namespace WebCore { > @@ -153,3 +154,4 @@ bool ContextMenuItem::enabled() const > } > > } > +#endif // ENABLE(CONTEXT_MENUS) Ditto. > diff --git a/WebCore/platform/mac/ContextMenuMac.mm b/WebCore/platform/mac/ContextMenuMac.mm > index b56d0b9..60832b2 100644 > --- a/WebCore/platform/mac/ContextMenuMac.mm > +++ b/WebCore/platform/mac/ContextMenuMac.mm > @@ -26,6 +26,7 @@ > #include "config.h" > #include "ContextMenu.h" > > +#if ENABLE(CONTEXT_MENUS) > #include "ContextMenuController.h" > > @interface WebCoreMenuTarget : NSObject { > @@ -152,3 +153,4 @@ NSMutableArray* ContextMenu::releasePlatformDescription() > } > > } > +#endif // ENABLE(CONTEXT_MENUS) Ditto. > diff --git a/WebKit/mac/ChangeLog b/WebKit/mac/ChangeLog > diff --git a/WebKit/mac/DefaultDelegates/WebDefaultContextMenuDelegate.mm b/WebKit/mac/DefaultDelegates/WebDefaultContextMenuDelegate.mm > diff --git a/WebKit/mac/WebCoreSupport/WebContextMenuClient.mm b/WebKit/mac/WebCoreSupport/WebContextMenuClient.mm > diff --git a/WebKit/mac/WebCoreSupport/WebViewFactory.mm b/WebKit/mac/WebCoreSupport/WebViewFactory.mm > diff --git a/WebKit/mac/WebView/WebHTMLView.mm b/WebKit/mac/WebView/WebHTMLView.mm > diff --git a/WebKit/mac/WebView/WebUIDelegatePrivate.h b/WebKit/mac/WebView/WebUIDelegatePrivate.h > diff --git a/WebKit/mac/WebView/WebView.mm b/WebKit/mac/WebView/WebView.mm As we discussed offline, please leave these changes out. It doesn't make sense to disable context menu support for code in WebKit/mac. When WebKit code for iPhone lands, it will be refactored or moved instead of shared within WebKit/mac. r=me
http://trac.webkit.org/changeset/48429