Currently, the context menu of resource items in the Resources tab is ["Save File"] whereas the context menu of resource items in the Timelines tab is ["Save File", "Copy as cURL"].
<rdar://problem/28049135>
Created attachment 287237 [details] Patch
Comment on attachment 287237 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287237&action=review > Source/WebInspectorUI/UserInterface/Models/Resource.js:725 > + addContextMenuItems(contextMenu) I think Brian may have some comments about whether or not this is a layering violation. I once did this with Breakpoint.js and it eventually got moved out. <https://webkit.org/b/127328> Web Inspector: Move the Popover code out of the Breakpoint model object What I was considering was a global function in a file like ContextMenuUtilties.js or something: WebInspector.addContextMenuItems = function(contextMenu, representedObject) { if (representedObject instanceof WebInspector.Resource) return addResourceContextMenuItems(contextMenu, representedObject); if (representedObject instanceof WebInspector.Breakpoint) return addBreakpointContextMenuItems(contextMenu, representedObject); // DOMNode, RemoteObject, ... }
Comment on attachment 287237 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287237&action=review r-, please move context menu generator code into its own file. See discussion. Otherwise it looks fine. >> Source/WebInspectorUI/UserInterface/Models/Resource.js:725 >> + addContextMenuItems(contextMenu) > > I think Brian may have some comments about whether or not this is a layering violation. I once did this with Breakpoint.js and it eventually got moved out. > <https://webkit.org/b/127328> Web Inspector: Move the Popover code out of the Breakpoint model object > > What I was considering was a global function in a file like ContextMenuUtilties.js or something: > > WebInspector.addContextMenuItems = function(contextMenu, representedObject) > { > if (representedObject instanceof WebInspector.Resource) > return addResourceContextMenuItems(contextMenu, representedObject); > if (representedObject instanceof WebInspector.Breakpoint) > return addBreakpointContextMenuItems(contextMenu, representedObject); > // DOMNode, RemoteObject, ... > } I would prefer this to be out of line of the Models classes as it's only used by View code and has lots of UIStrings etc. It seems unlikely to me that the context menu items will need to use private details of model classes, so they are just as easy to write against the public methods of model classes. Typically, context menu generation follows the view hierarchy upwards, so we probably still want the existing event propagation code. The various view classes to call into ContextMenuUtilities with the correct represented object, which is not always possible to figure out automatically.
Created attachment 287327 [details] Patch
Comment on attachment 287327 [details] Patch r=me
Comment on attachment 287327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287327&action=review > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:28 > + Nit: Remove empty line. > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:35 > + console.assert(contextMenu instanceof WebInspector.ContextMenu); > + if (!(contextMenu instanceof WebInspector.ContextMenu)) > + return; > + > + console.assert(resource instanceof WebInspector.Resource); > + if (!(resource instanceof WebInspector.Resource)) > + return; These seen kinda unnecessary to me. If anyone is calling this method with the wrong values it their own mistake, and it seems super unlikely.
Comment on attachment 287327 [details] Patch Clearing flags on attachment: 287327 Committed r205151: <http://trac.webkit.org/changeset/205151>
All reviewed patches have been landed. Closing bug.