Bug 151629 – Web Inspector: allow multiple UI components to add menu items upon getting a "contextmenu" event

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

Prerequisite for adding a debug-only global "Reload Web Inspector" context menu item. Also gives us a few more context menu items for free.

[Radar WebKit Bug Importer]

<rdar://problem/23673554>

[Blaze Burg]

Created attachment 266192 [details]
Proposed Fix

[Timothy Hatcher]

Comment on attachment 266192 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=266192&action=review

Nice!

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:43
> +        let editBreakpoint = () => {

const?

[WebKit Commit Bot]

Comment on attachment 266192 [details]
Proposed Fix

Rejecting attachment 266192 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 266192, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
erface/Views/SourceCodeTextEditor.js
patching file Source/WebInspectorUI/UserInterface/Views/TabBarItem.js
patching file Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js
patching file Source/WebInspectorUI/UserInterface/Views/Toolbar.js
patching file Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorTreeItem.js

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Timothy Hatcher']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/489882

[Timothy Hatcher]

Comment on attachment 266192 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=266192&action=review

> Source/WebInspectorUI/UserInterface/Views/ContextMenu.js:164
> +        if (!event[WebInspector.ContextMenu.ProposedMenuSymbol] && !onlyExisting)
> +            event[WebInspector.ContextMenu.ProposedMenuSymbol] = new WebInspector.ContextMenu(event);
> +
> +        return event[WebInspector.ContextMenu.ProposedMenuSymbol] || null;

It might make sense to append a separator item if the menu is already existing, that way appended menu items are auto separated. This will likely require the show code to prune prefix, trailing and double separator items.

[Blaze Burg]

Comment on attachment 266192 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=266192&action=review

>> Source/WebInspectorUI/UserInterface/Views/ContextMenu.js:164
>> +        return event[WebInspector.ContextMenu.ProposedMenuSymbol] || null;
> 
> It might make sense to append a separator item if the menu is already existing, that way appended menu items are auto separated. This will likely require the show code to prune prefix, trailing and double separator items.

Since we use a _pendingSeparator flag, only one can ever be added between two items. The flag only takes effect on the next item if there was a previous item. So, I don't think this is an issue.

As for separator placement, it's really confusing at the moment where they should belong. Are there any good Cocoa documents with tips on how to organize context menus? My gut feeling is to have a section (or submenu) per "context" that could have been clicked. So, clicking in console would show object items, then console items, then global items. It would be straightforward to make it so that each UI component that appends to the menu gets its own divider.

Let's defer adjusting context menu ordering for now. In all my testing, "Reload Web Inspector" always showed up in a section with Inspect Element and separated from other menus.

[Blaze Burg]

Committed r192789: <http://trac.webkit.org/changeset/192789>