1268444 - Convert network monitor to use new key shortcut API

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Alexandre Poirot [:ochameau] [PTO back August 21]]

We need to stop using xul for key shortcuts and instead use the upcoming module from bug 1260493.
http://mxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/netmonitor.xul#85

Comment 1

[Marco Mucci [:MarcoM]]

Hi Alex, will this bug require QA verification?

Comment 2

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Yes. We would need to verify that related key shortcuts works the same.

Comment 3

[[e-mail]]

What needs to be done:

1. convert key freeTextFilterKey&command in netmonitor.xul
2. convert requestsMenuSortKeyboardEvent in reqeust-menu-view
3. convert requestsMenuFilterKeyboardEvent in reqeust-menu-view

4. remove netmonitor.dtd if its land after bug 1308440

Comment 4

[[e-mail]]

Attachment: Bug 1268444 - Convert network monitor to use new key shortcut API;

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

Comment 5

[[e-mail]]

Instead of Comment 3, at the end I found only freeTextFilterKey&command in netmonitor.xul is needed to migrate

Comment 6

[[e-mail]]

Note: I refered `inspector.js` to put shortcut binding in panel.js
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/inspector/inspector.js#L379

Comment 7

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

Comment on attachment 8802051 [details]
Bug 1268444 - Convert network monitor to use new key shortcut API;

https://reviewboard.mozilla.org/r/86604/#review85564

Thanks for working on this!

See my inline comments.

Honza

::: devtools/client/locales/en-US/netmonitor.properties:482
(Diff revision 1)
>  netmonitor.footer.filterOther=Other
>  
>  # LOCALIZATION NOTE (netmonitor.footer.filterFreetext): This is the label displayed
>  # in the network details footer for the url filtering textbox.
>  netmonitor.footer.filterFreetext.label=Filter URLs
> -netmonitor.footer.filterFreetext.key=F
> +netmonitor.footer.filterFreetext.key=CmdOrCtrl+F

If you are changing a string you also need to change the string ID so, localizers are notified about the change. See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1309191

::: devtools/client/netmonitor/panel.js:34
(Diff revision 1)
> +  });
> +  shortcuts.on(L10N.getStr("netmonitor.footer.filterFreetext.key"), (name, event) => {
> +    event.preventDefault();
> +    this._view.RequestsMenu.freetextFilterBox.focus();
> +  });
>  }

Shouldn't you call shortcuts.destroy() to clean up the 'keydown' listener?

Comment 8

[[e-mail]]

Comment on attachment 8802051 [details]
Bug 1268444 - Convert network monitor to use new key shortcut API;

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

Comment 9

[[e-mail]]

Issue addressed, please kindly review it

Comment 10

[[e-mail]]

Comment on attachment 8802051 [details]
Bug 1268444 - Convert network monitor to use new key shortcut API;

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

Comment 11

[[e-mail]]

Note: `netmonitor.footer.filterFreetext.key` is renamed to `netmonitor.toolbar.filterFreetext.key` to align with renaming in Bug 1309191

Comment 12

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

Comment on attachment 8802051 [details]
Bug 1268444 - Convert network monitor to use new key shortcut API;

https://reviewboard.mozilla.org/r/86604/#review85832

LGTM!

Honza

Comment 13

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

Comment on attachment 8802051 [details]
Bug 1268444 - Convert network monitor to use new key shortcut API;

https://reviewboard.mozilla.org/r/86604/#review85836

R+ assuming Try is green

Honza

Comment 14

[[e-mail]]

The try is green (with 1 unrelated framework/registration error)

thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f05cfb4a8dc
Convert network monitor to use new key shortcut API;r=Honza

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1f05cfb4a8dc

Comment 17

[Ciprian Georgiu, Desktop QA]

This issue is verified fixed on latest Nightly build 52.0a1 from 2016-10-20 under the following OSes:
- Windows 10 x64
- Ubuntu 16.04 x64
- macOS 10.12.1 Beta

The related network monitor shortcut key (Ctrl/Cmd+F) is still working the same on all platforms. Marking here accordingly.