Bug 161301 - Web Inspector: Unify Resource entry context menus
Summary: Web Inspector: Unify Resource entry context menus
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: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-08-28 12:47 PDT by Devin Rousso
Modified: 2016-08-29 15:12 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.17 KB, patch)
2016-08-28 13:20 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (7.31 KB, patch)
2016-08-29 14:17 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2016-08-28 12:47:49 PDT
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"].
Comment 1 Radar WebKit Bug Importer 2016-08-28 12:48:13 PDT
<rdar://problem/28049135>
Comment 2 Devin Rousso 2016-08-28 13:20:52 PDT
Created attachment 287237 [details]
Patch
Comment 3 Joseph Pecoraro 2016-08-29 11:53:24 PDT
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 4 BJ Burg 2016-08-29 12:53:40 PDT
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.
Comment 5 Devin Rousso 2016-08-29 14:17:35 PDT
Created attachment 287327 [details]
Patch
Comment 6 BJ Burg 2016-08-29 14:21:37 PDT
Comment on attachment 287327 [details]
Patch

r=me
Comment 7 Joseph Pecoraro 2016-08-29 14:59:43 PDT
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 8 WebKit Commit Bot 2016-08-29 15:12:02 PDT
Comment on attachment 287327 [details]
Patch

Clearing flags on attachment: 287327

Committed r205151: <http://trac.webkit.org/changeset/205151>
Comment 9 WebKit Commit Bot 2016-08-29 15:12:07 PDT
All reviewed patches have been landed.  Closing bug.