Closed Bug 1308440 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox52 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 52
Iteration:
52.3 - Nov 14
Tracking Flags:
Tracking Status
firefox52 --- verified

People

(Reporter: steveck, Assigned: gasolin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(2 files, 1 obsolete file)

Bug 1308440 - Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel;
58 bytes, text/x-review-board-request
Honza
: review+
Details
0001-Bug-1308440-fix-related-contextMenu-tests-r-Tomcat.patch
3.10 KB, patch
cbook
: review+
Details | Diff | Splinter Review 
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.
Whiteboard: [devtools-html]
Whiteboard: [netmonitor]
Flags: qe-verify+
QA Contact: ciprian.georgiu
Assignee: nobody → gasolin
See Also: → 1266478
Status: NEW → ASSIGNED
Priority: -- → P1
Depends on: 1308500
Iteration: --- → 52.3 - Nov 7
Attachment #8802368 - Attachment is obsolete: true
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.
Attachment #8802369 - Flags: review?(odvarko)
> 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
Thanks for catching that! I'll update the patch soon
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
Attachment #8802369 - Flags: review?(odvarko)
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 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 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
Attachment #8802369 - Flags: review?(odvarko) → review+
thanks
Keywords: checkin-needed
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b49b26e41929
Migrate Net Panel Context Menu with framework/menu API in NetMonitor panel;r=Honza
Keywords: checkin-needed
Here's the patch to fix contextmenu related tests
Attachment #8803815 - Flags: review?(cbook)
Attachment #8803815 - Flags: review?(cbook) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08efaee1d568
fix related contextMenu tests; r=Tomcat
https://hg.mozilla.org/mozilla-central/rev/b49b26e41929
https://hg.mozilla.org/mozilla-central/rev/08efaee1d568
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
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.
(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
Depends on: 1315175
(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.
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.
(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.
Thanks Jarda, for your quick reply. I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: