[webkit-reviews] review granted: [Bug 29225] Add ENABLE(CONTEXT_MENU) : [Attachment 39624] Uses ENABLE_CONTEXT_MENUS

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted Greg Bolsinga
<bolsinga at apple.com>'s request for review:
Bug 29225: Add ENABLE(CONTEXT_MENU)
https://bugs.webkit.org/show_bug.cgi?id=29225

Attachment 39624: Uses ENABLE_CONTEXT_MENUS
https://bugs.webkit.org/attachment.cgi?id=39624&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> +	   * 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