1308440 - Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Steve Chung [:steveck]]

We'll need to finish following tasks for Net Panel Context Menu:
- Use standard menu API (just like for the Inspector panel)
- Use client/framework/menu component. Note: the component still uses XUL underneath.

Comment 1

[[e-mail]]

Attachment: s

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

Comment 2

[[e-mail]]

Attachment: Bug 1308440 - Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel;

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

Comment 3

[[e-mail]]

Comment on attachment 8802369 [details]
Bug 1308440 - Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86784/diff/1-2/

Comment 4

[[e-mail]]

Comment on attachment 8802369 [details]
Bug 1308440 - Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86784/diff/2-3/

Comment 5

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

Comment on attachment 8802369 [details]
Bug 1308440 - Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel;

https://reviewboard.mozilla.org/r/86784/#review86184

Please, see my inline comment.

Honza

::: devtools/client/netmonitor/requests-menu-view.js:1857
(Diff revision 3)
> +
> +    menu.append(new MenuItem({
> +      id: "request-menu-context-copy-post-data",
> +      label: L10N.getStr("netmonitor.context.copyPostData"),
> +      accesskey: L10N.getStr("netmonitor.context.copyPostData.accesskey"),
> +      visible: selectedItem && selectedItem.attachment.requestPostData,

The `selectedItem && selectedItem.attachment.requestPostData` condition is wrong in this context since it can return `undefined` in case where `requestPostData` are undefined.

You should set the `visible` filed to `undefined` as in such case the default value (`true`) is used. Please see: devtools/client/framework/menu-item.js

You should alwasy pass `true` or `false` so, the correct condition looks as follows:

`!!(selectedItem && selectedItem.attachment.requestPostData)`

Please fix all other conditions too.

You might alsoe make a comment about this, in front of the entire `_openMenu` method.

Comment 6

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

> You should set the `visible` filed to `undefined` as in such case the default value (`true`) is used. > Please see: devtools/client/framework/menu-item.js

CORRECTION: You should *not* set the `visible` field to `undefined`

Honza

Comment 7

[[e-mail]]

Thanks for catching that! I'll update the patch soon

Comment 8

[[e-mail]]

Comment on attachment 8802369 [details]
Bug 1308440 - Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86784/diff/3-4/

Comment 9

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

Comment on attachment 8802369 [details]
Bug 1308440 - Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel;

https://reviewboard.mozilla.org/r/86784/#review86512

Good, the visibility flag is now fixed, thanks!

Two more comments.

Honza

::: devtools/client/netmonitor/netmonitor.xul
(Diff revisions 3 - 4)
> -  <commandset>
> -    <command id="freeTextFilterCommand"
> -             oncommand="NetMonitorView.RequestsMenu.freetextFilterBox.focus()"/>
> -  </commandset>
> -
> -  <keyset>
> -    <key id="freeTextFilterKey"
> -         data-localization="key=netmonitor.footer.filterFreetext.key"
> -         modifiers="accel"
> -         command="freeTextFilterCommand"/>
> -  </keyset>
> -

Shouldn't this be part of the patch for bug 1268444 ?

::: devtools/client/netmonitor/requests-menu-view.js:1831
(Diff revisions 3 - 4)
>      });
>    },
>  
>    /**
>     * Handle the context menu opening. Hide items if no request is selected.
> +   * Since visible attribute only accept boolean value but the method call may 

nit: remove space at the end of the line

Comment 10

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

One more nit, the commit message you are using is as follows:

Bug 1308440 - Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel;r=honza

I believe there should be a space between the semicolon and reviewer name like so: 

Bug 1308440 - Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel; r=honza

See also: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

Honza

Comment 11

[[e-mail]]

Comment on attachment 8802369 [details]
Bug 1308440 - Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel;

https://reviewboard.mozilla.org/r/86784/#review86512

> Shouldn't this be part of the patch for bug 1268444 ?

Since bug 1268444  is laneded, this part is gone after rebase

Comment 12

[[e-mail]]

Comment on attachment 8802369 [details]
Bug 1308440 - Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86784/diff/4-5/

Comment 13

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

Comment on attachment 8802369 [details]
Bug 1308440 - Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel;

https://reviewboard.mozilla.org/r/86784/#review86540

LGTM!

R+ assuming try is green.

Honza

Comment 14

[[e-mail]]

Comment on attachment 8802369 [details]
Bug 1308440 - Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86784/diff/5-6/

Comment 15

[[e-mail]]

thanks

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b49b26e41929
Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel;r=Honza

Comment 17

[[e-mail]]

Attachment: 0001-Bug-1308440-fix-related-contextMenu-tests-r-Tomcat.patch

Here's the patch to fix contextmenu related tests

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08efaee1d568
fix related contextMenu tests; r=Tomcat

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b49b26e41929
https://hg.mozilla.org/mozilla-central/rev/08efaee1d568

Comment 20

[Jarda Snajdr [:jsnajdr]]

Just got this patch from upstream mozilla-central and merging it into my WIP changes... It would have been much much better if the context menu code (creating the menu and all the onContext* methods) were in a separate module, not in requests-menu-view.js.

Comment 21

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

(In reply to Jarda Snajdr [:jsnajdr] from comment #20)
> Just got this patch from upstream mozilla-central and merging it into my WIP
> changes... It would have been much much better if the context menu code
> (creating the menu and all the onContext* methods) were in a separate
> module, not in requests-menu-view.js.

Jarda, please file a follow up to migrate it into separate module.

Honza

Comment 22

[Jarda Snajdr [:jsnajdr]]

(In reply to Jan Honza Odvarko [:Honza] from comment #21)
> Jarda, please file a follow up to migrate it into separate module.

Filed bug 1315175.

Comment 23

[Ciprian Georgiu, Desktop QA]

I verified this issue on latest Nightly build 52.0a1 from 2016-10-09, using Windows 10 x64, Ubuntu 14.04 x86 LTS and Mac OS X 10.11. 
 
 -- Not sure if this is entirely related to this bug, but I noticed that Context menu is not showing up if I right click on the 'Filter URLs' from Net Panel Toolbar. This used to work a few days ago with an older Nightly build (52.0a1 Build ID 20161007030207). 

What do you think, Fred? Should I file a separate bug for this? Leaving the flags in place for now.

Comment 24

[Jarda Snajdr [:jsnajdr]]

(In reply to Ciprian Georgiu, QA [:ciprian_georgiu] from comment #23)
>  -- Not sure if this is entirely related to this bug, but I noticed that
> Context menu is not showing up if I right click on the 'Filter URLs' from
> Net Panel Toolbar. This used to work a few days ago with an older Nightly
> build (52.0a1 Build ID 20161007030207). 
> 
> What do you think, Fred? Should I file a separate bug for this? Leaving the
> flags in place for now.

It's not related to this one, but it's a regression introduced in bug 1309192, which converted the Filter URL field from XUL to React. Context menu is no longer displayed on right click. It contains useful actions like Copy or Paste.

Comment 25

[Ciprian Georgiu, Desktop QA]

Thanks Jarda, for your quick reply. I will mark this as verified fixed.