Bug 151629 - Web Inspector: allow multiple UI components to add menu items upon getting a "contextmenu" event
Summary: Web Inspector: allow multiple UI components to add menu items upon getting a ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks: 141742
  Show dependency treegraph
 
Reported: 2015-11-26 20:49 PST by BJ Burg
Modified: 2015-11-29 16:39 PST (History)
8 users (show)

See Also:


Attachments
Proposed Fix (45.54 KB, patch)
2015-11-26 21:00 PST, BJ Burg
timothy: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2015-11-26 20:49:54 PST
Prerequisite for adding a debug-only global "Reload Web Inspector" context menu item. Also gives us a few more context menu items for free.
Comment 1 Radar WebKit Bug Importer 2015-11-26 20:50:06 PST
<rdar://problem/23673554>
Comment 2 BJ Burg 2015-11-26 21:00:38 PST
Created attachment 266192 [details]
Proposed Fix
Comment 3 Timothy Hatcher 2015-11-28 13:26:46 PST
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?
Comment 4 WebKit Commit Bot 2015-11-28 13:28:11 PST
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
Comment 5 Timothy Hatcher 2015-11-28 13:34:17 PST
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.
Comment 6 BJ Burg 2015-11-29 16:36:45 PST
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.
Comment 7 BJ Burg 2015-11-29 16:39:20 PST
Committed r192789: <http://trac.webkit.org/changeset/192789>