Bug 29225 – Add ENABLE(CONTEXT_MENU)

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Greg Bolsinga]

Wrap Context Menu code in an ENABLE wrapper. This is for platforms (such as iPhone) that do not support context menus.

[Greg Bolsinga]

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?

[Mark Rowe (bdash)]

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.

[Greg Bolsinga]

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.

[Greg Bolsinga]

<rdar://problem/6732593>

[Mark Rowe (bdash)]

The DerivedSources.make change still assumes this is set in FEATURE_DEFINES.

[Greg Bolsinga]

Created attachment 39558 [details]
Fix DerivedSources.make

Thanks!

[Greg Bolsinga]

Created attachment 39559 [details]
Fix DerivedSources.make for all platforms, not just MacOS

[Greg Bolsinga]

This is being conditionalized because it is not enabled on iPhone.

[Mark Rowe (bdash)]

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.

[Greg Bolsinga]

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.

[Greg Bolsinga]

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.

[David Kilzer (:ddkilzer)]

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

[Greg Bolsinga]

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