[webkit-reviews] review denied: [Bug 124747] Web Inspector: Allow showing a context menu on all mouse events. : [Attachment 217619] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Antoine Quint
<graouts at apple.com>'s request for review:
Bug 124747: Web Inspector: Allow showing a context menu on all mouse events.
https://bugs.webkit.org/show_bug.cgi?id=124747

Attachment 217619: Patch
https://bugs.webkit.org/attachment.cgi?id=217619&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=217619&action=review


There are a lot of layering violations here. r- for this patch because I'd like
to see a new patch with them addressed.

Source/WebCore/platform is special. It is supposed to be a base platform layer.
Files in that directory should not know about anything else inside of
Source/WebCore.

Also after digging around in WebCore more I see ContextMenuController which
seems to do a lot of what you want.

Sorry if I was misleading you before, I wasn't really familiar with the
ContextMenu handling in WebCore.

> Source/WebCore/ChangeLog:19
> +	   * platform/ContextMenu.h:
> +	   Add Mac-specific members and Objective-C class to handle the display
of an
> +	   NSMenu and act as the target of its NSMenuItems.

I think making this Mac-specific is unnecessarily ignoring other ports that
share the Inspector.

I don't yet know how this will be used by the inspector, but I would like to
avoid seeing code in the Inspector that looks like:

    #if PLATFORM(MAC)
    // Do something really cool
    #else
    // Fallback to something less cool
    #endif

For something like this you can define the interface for everyone, and put stub
notImplemented(); impls in other Platform ContextMenu* implementation files,
along with a // FIXME: Bug(foo) bugzilla bug for each port.

If the feature is something we can't do in the Inspector without implementation
support, then we can make runtime check functions
"canShowContextMenuAtAnyTime()" and return true on ports that support the
feature and false on others. Eventually when all ports support the feature we
could remove the if check. I don't think that would really be needed here
though.

So, worst case, the inspector code would end up looking like:

    if (hasCapability(...)) {
      // Do something really cool
    } else {
      // Do something less cool, or nothing at all. This port should really
implement capability(...)!
    }

> Source/WebCore/platform/ContextMenu.h:35
> +#include "Event.h"
> +#include "Page.h"

Layering violation.

> Source/WebCore/platform/ContextMenu.h:49
> +#ifdef __OBJC__
> + at class WebContextMenuTarget;
> +#else
> +class WebContextMenuTarget;
> +#endif

Ugly pattern eh? We have macros for this! You should be able to do:

    OBJC_CLASS WebContextMenuTarget;

> Source/WebCore/platform/ContextMenu.h:102
> +	   WebContextMenuTarget* m_contextMenuTarget;

Nit: RetainPtr

> Source/WebCore/platform/ContextMenu.h:103
> +	   Page* m_page;

m_page is no longer used.

> Source/WebCore/platform/mac/ContextMenuMac.mm:35
> +#include "Document.h"
> +#include "DOMWrapperWorld.h"
> +#include "EventTarget.h"
> +#include "FrameView.h"
> +#include "Node.h"
> +#include "MouseEvent.h"
> +#include "ScriptFunctionCall.h"
> +#include "UserGestureIndicator.h"

Here are a lot of layering violations that would need to be addressed.

> Source/WebCore/platform/mac/ContextMenuMac.mm:50
> +    m_contextMenuTarget = [[WebContextMenuTarget alloc] init];

This would be leaked as written because m_contextMenuTarget is never
deallocated. Also, the existing NSMutableArray allocation is sub-optimal. Using
RetainPtr would be better, less error prone. I'd much rather see
m_contextMenuTarget be a RetainPtr and this kind of constructor be rewritten
as:

    ContextMenu::ContextMenu()
	: m_platformDescription(adoptNS([[NSMutableArray alloc] init]))
	, m_contextMenuTarget(adoptNS([[WebContextMenuTarget alloc] init]))
    {
    }

Less retain/release churn, and now you don't need to worry about releasing the
ivars.

> Source/WebCore/platform/mac/ContextMenuMac.mm:182
> + at implementation WebContextMenuTarget
> +
> +- (void)menuSelection:(NSMenuItem *)menuItem
> +{
> +    JSC::ExecState* frontendExecState =
execStateFromPage(WebCore::debuggerWorld(), self.page);
> +    WebCore::ScriptObject frontendApiObject;
> +    if (!WebCore::ScriptGlobalObject::get(frontendExecState,
"InspectorFrontendAPI", frontendApiObject)) {
> +	   ASSERT_NOT_REACHED();
> +	   return;
> +    }
> +
> +    WebCore::UserGestureIndicator
gestureIndicator(WebCore::DefinitelyProcessingUserGesture);
> +    WebCore::ScriptFunctionCall function(frontendApiObject,
"contextMenuItemSelected");
> +    function.appendArgument([menuItem tag] -
WebCore::ContextMenuItemBaseCustomTag);
> +    function.call();
> +}
> +
> + at end

WebCore/platform/ContextMenu should certainly not be running JavaScript on a
page!

A better API model would be to give the ContextMenu a ContextMenuClient. And
that ContextMenuClient, presumably in the Inspector would be told of the menu
selection and maybe also if the menu was dismissed without a selection.

In fact, I think that is what WebCore/page/ContextMenuController.h already
provides.

    • ContextMenuController contains a platform ContextMenu
    • ContextMenuController takes in a Page and Client:
ContextMenuController(Page&, ContextMenuClient&);
    • ContextMenuController's API has: void showContextMenu(Event*,
PassRefPtr<ContextMenuProvider>);
    • ContextMenuClient has methods for a selection and when the menu is
destroyed (maybe that means dismissed):
	virtual void contextMenuDestroyed() = 0;
	virtual void contextMenuItemSelected(ContextMenuItem*, const
ContextMenu*) = 0;

It looks like ContextMenuController is already a platform abstraction over
"show a context menu". I think Inspector code should be able to create and use
your own ContextMenuController in Inspector code.

Some notes about ContextMenuController. It looks like is geared specifically
toward being a "Page" right click context menu. For example it has:

    #if ENABLE(INSPECTOR)
	if (m_page.inspectorController()->enabled())
	    addInspectElementItem();
    #endif

And a populate method with all kinds of page specific stuff.

I think you might want to abstract ContextMenuController to be more generic,
and make a PageContextMenuController for WebCore::Page, and use the newly clean
base ContextMenuController for the inspector.