Created attachment 44365 [details] [IMAGE] Screenshot while running with patch. This patch extends ContextMenuController in order to allow custom context menus on pages. So far, inspector is the only client of the new API, but this might change once HTML5's contextmenu is implemented.
Created attachment 44366 [details] [PATCH] Proposed change
Comment on attachment 44366 [details] [PATCH] Proposed change > + class MenuSelectionHandler : public ContextMenuSelectionHandler > + { Brace should be on the same line. > + virtual ~MenuSelectionHandler() {} I think we informally like a space between the braces. { } vs {}. > + MenuSelectionHandler(InspectorFrontendHost* frontendHost) > + : m_frontendHost(frontendHost) > + { > + } We informally like to put empty constructors on one line. > + this._appendItem(WebInspector.UIString("Edit as HTML"), this._noop.bind(this)); > + this._appendItem(WebInspector.UIString("Add attribute"), this._noop.bind(this)); > + this._appendSeparator(); > + this._appendItem(WebInspector.UIString("Copy"), this._copy.bind(this)); > + this._appendItem(WebInspector.UIString("Delete"), this._delete.bind(this)); This confused me, since those file is generic. We should land this file mostly empty until it is ready to work. > + _appendItem: function(label, handler) > + { > + var id = this._items.length; > + this._items.push({id: id, label: label}); > + this._handlers[id] = handler; > + }, > + > + _appendSeparator: function() > + { > + this._items.push({}); > + }, I think in the end these functions will be publicly used and should drop the underscore. But maybe I don't understand how the final design will work. > +WebInspector.documentContextMenu = function(event) Maybe contextMenuEventFired is a more descriptive name. > + if (item->action() >= ContextMenuItemBaseCustomTag) { > + ASSERT(m_selectionHandler); > + m_selectionHandler->contextMenuItemSelected(item); > + return; > + } Does ContextMenuItemBaseApplicationTag get handled before this block? What prevents another tag higher that isn't a "ContextMenuItemBaseCustomTag" from hitting this? > + class ContextMenuSelectionHandler : public RefCounted<ContextMenuSelectionHandler> > + { Brace should be on the same line. > + ContextMenuSelectionHandler() {} > + virtual ~ContextMenuSelectionHandler() {}; Put a space in the braces.
(In reply to comment #2) > (From update of attachment 44366 [details]) > > > + class MenuSelectionHandler : public ContextMenuSelectionHandler > > + { > > Brace should be on the same line. > Done. > > > + virtual ~MenuSelectionHandler() {} > > I think we informally like a space between the braces. { } vs {}. > Done. > > > + MenuSelectionHandler(InspectorFrontendHost* frontendHost) > > + : m_frontendHost(frontendHost) > > + { > > + } > > We informally like to put empty constructors on one line. > Done. > > > + this._appendItem(WebInspector.UIString("Edit as HTML"), this._noop.bind(this)); > > + this._appendItem(WebInspector.UIString("Add attribute"), this._noop.bind(this)); > > + this._appendSeparator(); > > + this._appendItem(WebInspector.UIString("Copy"), this._copy.bind(this)); > > + this._appendItem(WebInspector.UIString("Delete"), this._delete.bind(this)); > > This confused me, since those file is generic. We should land this file mostly > empty until it is ready to work. > I'll migrate to a context-per view schema in subsequent change. So far this context menu triggering is anyway commented out. > > > + _appendItem: function(label, handler) > > + { > > + var id = this._items.length; > > + this._items.push({id: id, label: label}); > > + this._handlers[id] = handler; > > + }, > > + > > + _appendSeparator: function() > > + { > > + this._items.push({}); > > + }, > > I think in the end these functions will be publicly used and should drop the > underscore. But maybe I don't understand how the final design will work. > They will be public, yes. > > > +WebInspector.documentContextMenu = function(event) > > Maybe contextMenuEventFired is a more descriptive name. > > Done. > > + if (item->action() >= ContextMenuItemBaseCustomTag) { > > + ASSERT(m_selectionHandler); > > + m_selectionHandler->contextMenuItemSelected(item); > > + return; > > + } > > Does ContextMenuItemBaseApplicationTag get handled before this block? What > prevents another tag higher that isn't a "ContextMenuItemBaseCustomTag" from > hitting this? > There is a following check above: if (item->action() >= ContextMenuItemBaseApplicationTag) { m_client->contextMenuItemSelected(item, m_contextMenu.get()); return; } > > > + class ContextMenuSelectionHandler : public RefCounted<ContextMenuSelectionHandler> > > + { > > Brace should be on the same line. > > Done. > > + ContextMenuSelectionHandler() {} > > + virtual ~ContextMenuSelectionHandler() {}; > > Put a space in the braces. Done.
Comment on attachment 44366 [details] [PATCH] Proposed change I'll land manually with v8's [Custom] stub for showContextMenu.
Committing to http://svn.webkit.org/repository/webkit/trunk ... C WebCore/inspector/InspectorFrontendHost.idl => WebCore/inspector/front-end/ContextMenu.js C WebCore/inspector/InspectorFrontendHost.idl => WebCore/page/ContextMenuSelectionHandler.h M WebCore/ChangeLog M WebCore/English.lproj/localizedStrings.js M WebCore/GNUmakefile.am M WebCore/WebCore.gypi M WebCore/WebCore.pro M WebCore/WebCore.vcproj/WebCore.vcproj M WebCore/WebCore.xcodeproj/project.pbxproj M WebCore/bindings/js/JSInspectorFrontendHostCustom.cpp M WebCore/bindings/v8/custom/V8CustomBinding.h M WebCore/bindings/v8/custom/V8InspectorFrontendHostCustom.cpp M WebCore/inspector/InspectorFrontend.cpp M WebCore/inspector/InspectorFrontend.h M WebCore/inspector/InspectorFrontendHost.cpp M WebCore/inspector/InspectorFrontendHost.h M WebCore/inspector/InspectorFrontendHost.idl M WebCore/inspector/front-end/WebKit.qrc M WebCore/inspector/front-end/inspector.html M WebCore/inspector/front-end/inspector.js M WebCore/page/ContextMenuController.cpp M WebCore/page/ContextMenuController.h M WebCore/platform/ContextMenu.cpp M WebCore/platform/ContextMenuItem.h Committed r51839