1350235 - Support Copy submenu in the Context menu

Status: VERIFIED FIXED

(DevTools :: Netmonitor, enhancement, P1)

Description

[[e-mail]]

We'd like moving all copy related menus into a `Copy` sub-menu

Comment 1

[Michael Hoffmann]

Hi, I'd like to work on this bug

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Michael Brennan from comment #1)
> Hi, I'd like to work on this bug

Sounds great, assigning to you ;-)

Fred is mentor for this bug, so don't hesitate to ask questions!

Honza

Comment 3

[[e-mail]]

Michael, welcome!

If its the first time you try gecko, you could check https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build to build a workable firefox.


When you look at request-list-context-menu, you will see all right click menu items when you right click above the netmonitor's entry
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/request-list-context-menu.js

You can take a look how framework/menu and framework/menu-item works with submenu. Put copy related menu-item into the submenu and append back to the menu in the right place.


We'd like to put `har` related items into a submenu as well, but its fine to do it in a separate bug.


If you have any question, you can need info me in bugzilla, ping me on irc #devtools channel, or via devtools-html slack #netmonitor channel https://devtools-html.slack.com

Comment 4

[Michael Hoffmann]

Attachment: bug1350235.patch

Thanks Fred for the info!

I'm attaching a patch for feedback.  I put the `copyAllAsHar` menu item into the new sub-menu but also left it on the main menu.  I'm thinking that that menu item might belong to both the copy and HAR sub-menus.

Comment 5

[[e-mail]]

Comment on attachment 8852424 [details] [diff] [review]
bug1350235.patch

Thanks Michael! The patch looks good to me, here are some nits need to be addressed before send the review:

* current netmonitor sources are restructured, so you need rebase (or better create a new branch to port your change into there)
* You can just put `Copy All as HAR` into the submenu and remove the one outside. At the end it will looks like

Copy URL
Copy URL Parameters
Copy as cURL
---
Copy Request Headers
Copy Response Headers
Copy Response
---
Copy All as HAR

Comment 6

[[e-mail]]

Attachment: no arrow in submenu

Honza, I also saw the submenu doesn't have any visual difference from the normal menu item. (see as attachment)

I think we should not ship this issue until we fix the submenu visual issue.

Comment 7

[[e-mail]]

Ooh I found even in normal page's right click menu, their submenu doesn't have any visual hint as well.
So never mind, we can land this :)

Comment 8

[Michael Hoffmann]

Attachment: bug1350235.patch

Thanks for the feedback!

Here's an updated patch.  Btw, I do have a small arrow on my system.  I'm on Linux running Xfce.

Comment 9

[Michael Hoffmann]

(In reply to Michael Brennan from comment #8)
> Created attachment 8852818 [details] [diff] [review]
> bug1350235.patch
> 
> Thanks for the feedback!
> 
> Here's an updated patch.  Btw, I do have a small arrow on my system.  I'm on
> Linux running Xfce.

Oops, I see I actually missed to commit the `Copy All as HAR` change.  I'll put up a new patch in a bit.

Comment 10

[Michael Hoffmann]

Attachment: bug1350235.patch

Comment 11

[[e-mail]]

Comment on attachment 8852866 [details] [diff] [review]
bug1350235.patch

Looks good to me, thanks!

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8852866 [details] [diff] [review]
bug1350235.patch

Review of attachment 8852866 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work here!

Looks good and works as expected, R+

Honza

Comment 13

[[e-mail]]

Attachment: Bug 1350235 - Support Copy submenu in the Context menu

Review commit: https://reviewboard.mozilla.org/r/126674/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126674/

Comment 14

[[e-mail]]

Thanks Michael, I create a PR to do the auto test on our try server, if tests all passed, we can land your patch

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This doesn't have sufficient reviews set in MozReview for autoland to push it.

Comment 16

[[e-mail]]

Ooh, land the .patch is sufficient

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27dcd50df857
Move copy related menus into a sub-menu in netmonitor. r=gasolin, r=Honza

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27dcd50df857

Comment 19

[Emil Ghitta, QA [:emilghitta]]

I’ve reproduced the issue described in comment 0 using old Firefox 55.0a1 (Build Id:20170324030205) and verified that the issue is not reproducible anymore using Firefox 55.0a1 (Build Id:20170409123541) on Windows 10 64bit.

Comment 20

[[e-mail]]

all copy menu is now live within submenu, we should update MDN accordingly 

https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Context_menu