Web Inspector: Unify Resource entry context menus

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

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"].

[Radar WebKit Bug Importer]

<rdar://problem/28049135>

[Devin Rousso]

Created attachment 287237 [details]
Patch

[Joseph Pecoraro]

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, ...
    }

[BJ Burg]

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.

[Devin Rousso]

Created attachment 287327 [details]
Patch

[BJ Burg]

Comment on attachment 287327 [details]
Patch

r=me

[Joseph Pecoraro]

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.

[WebKit Commit Bot]

Comment on attachment 287327 [details]
Patch

Clearing flags on attachment: 287327

Committed r205151: <http://trac.webkit.org/changeset/205151>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.