Closed Bug 1361296 Opened 7 years ago Closed 7 years ago

Add Open in Debugger and Open In Style Editor in context menu

Categories

(DevTools :: Netmonitor, enhancement, P3)

enhancement

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: ntim, Assigned: vincent)

Details

(Keywords: dev-doc-needed, good-first-bug)

Attachments

(1 file)

      No description provided.
Hello,

Can I work on this ? :-)

Vincent
(In reply to Vincent Lequertier from comment #1)
> Hello,
> 
> Can I work on this ? :-)
> 
> Vincent

Sure!

Here's the context menu code: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/request-list-context-menu.js

Let us know if you need extra information.
Assignee: nobody → vi.le
Update: I've got the feature implemented, now I will focus on the "visible" and "accesskey" properties in the "menu.push()" entry, and tests for the feature.
Here is the patch. I'm a bit stuck with the tests. I'd like to add 2 tests : one that opens a request in the Debugger and one that opens a request in the Style Editor. The issue  is that the test I'm writing times out. So I have 2 questions :

- I tried to use tests to click on the buttons on the context menu but it seems like it does not find the button for some reasons. Can you point me to the right direction to fix the issue please ?

- How can test if the requests have been successfully opened in the Debugger/ Style Editor?

Thanks,

Vincent
Who should I put as a reviewer of the patch?

Vincent
Attachment #8870519 - Flags: review?(odvarko)
Comment on attachment 8870519 [details]
Bug 1361296 - Add Open in Debugger and Open in Style Editor in the netmonitor panel context menu;

https://reviewboard.mozilla.org/r/141954/#review149704

Thanks for working on this, I like that feature a lot!

Please see my inline comments.

Honza

::: devtools/client/netmonitor/src/request-list-context-menu.js:249
(Diff revision 1)
> +  /**
> +   * Opens selected item in the style editor
> +   */
> +  openInStyleEditor() {
> +    let toolbox = gDevTools.getToolbox(getTabTarget());
> +    toolbox.viewSourceInStyleEditor(this.selectedRequest.url, 0);

If the Style Editor panel isn't visible by default the action does nothing, which might be confusing fof the user. I think the panel should automatically appear in such case.

Btw. you can go to the Settings panel (click the little gear at the right side of Toolbox toolbar) to make the panel visible.

It's ok for me if this is done as a follow up.

::: devtools/client/netmonitor/test/browser_net_open_in_debugger.js:14
(Diff revision 1)
> +add_task(function* () {
> +  let { tab, monitor } = yield initNetMonitor(EXAMPLE_URL + "html_content-type-without-cache-test-page.html");
> +  info("Starting test... ");
> +
> +  let { document, store, windowRequire } = monitor.panelWin;
> +

You should switch of action batching to avoid async processing:

let Actions = windowRequire("devtools/client/netmonitor/src/actions/index");

store.dispatch(Actions.batchEnable(false));

::: devtools/client/netmonitor/test/browser_net_open_in_debugger.js:15
(Diff revision 1)
> +  let { tab, monitor } = yield initNetMonitor(EXAMPLE_URL + "html_content-type-without-cache-test-page.html");
> +  info("Starting test... ");
> +
> +  let { document, store, windowRequire } = monitor.panelWin;
> +
> +  let wait = waitForNetworkEvents(monitor, EXAMPLE_URL + "html_content-type-without-cache-test-page.html");

The second argument shoiuld be number of requests you are waiting for. I believe 8 in this case.

::: devtools/client/netmonitor/test/browser_net_open_in_debugger.js:27
(Diff revision 1)
> +    document.querySelectorAll(".request-list-item")[2]);
> +  EventUtils.sendMouseEvent({ type: "contextmenu" },
> +    document.querySelectorAll(".request-list-item")[2]);
> +
> +  monitor.panelWin.parent.document
> +    .querySelector("#request-list-context-open-in-debugger").click();

This will fail since the "Open in Debugger" action is only available for "script" requests while your test executes "xhr" requests only.

This also opens related questions. Should this support JS files (CSS files) received throug XHR? (e.g. by checking the mime type?)
Attachment #8870519 - Flags: review?(odvarko) → review-
(In reply to Jan Honza Odvarko [:Honza] from comment #7)
> Comment on attachment 8870519 [details]
> Bug 1361296 - Add Open in Debugger and Open in Style Editor in the
> netmonitor panel context menu;
> 
> https://reviewboard.mozilla.org/r/141954/#review149704
> 
> Thanks for working on this, I like that feature a lot!
> 
> Please see my inline comments.
> 
> Honza
> 
> ::: devtools/client/netmonitor/src/request-list-context-menu.js:249
> (Diff revision 1)
> > +  /**
> > +   * Opens selected item in the style editor
> > +   */
> > +  openInStyleEditor() {
> > +    let toolbox = gDevTools.getToolbox(getTabTarget());
> > +    toolbox.viewSourceInStyleEditor(this.selectedRequest.url, 0);
> 
> If the Style Editor panel isn't visible by default the action does nothing,
> which might be confusing fof the user. I think the panel should
> automatically appear in such case.
> 
> Btw. you can go to the Settings panel (click the little gear at the right
> side of Toolbox toolbar) to make the panel visible.
> 

As a workaround, I've updated the condition so the button is not displayed if the style editor is not enabled.

> This will fail since the "Open in Debugger" action is only available for
> "script" requests while your test executes "xhr" requests only.
> 
> This also opens related questions. Should this support JS files (CSS files)
> received throug XHR? (e.g. by checking the mime type?)

I'm now checking the mime type.

Thanks for reviewing my patch !

Vincent
Comment on attachment 8870519 [details]
Bug 1361296 - Add Open in Debugger and Open in Style Editor in the netmonitor panel context menu;

https://reviewboard.mozilla.org/r/141954/#review149704

> If the Style Editor panel isn't visible by default the action does nothing, which might be confusing fof the user. I think the panel should automatically appear in such case.
> 
> Btw. you can go to the Settings panel (click the little gear at the right side of Toolbox toolbar) to make the panel visible.
> 
> It's ok for me if this is done as a follow up.

As a workaround, I've updated the condition so the button is not displayed if the style editor is not enabled.

> This will fail since the "Open in Debugger" action is only available for "script" requests while your test executes "xhr" requests only.
> 
> This also opens related questions. Should this support JS files (CSS files) received throug XHR? (e.g. by checking the mime type?)

I'm now checking the mime type.
Comment on attachment 8870519 [details]
Bug 1361296 - Add Open in Debugger and Open in Style Editor in the netmonitor panel context menu;

https://reviewboard.mozilla.org/r/141954/#review152924

Thanks for the update!

Needs rebasing:

patching file devtools/client/netmonitor/test/browser.ini
Hunk #1 FAILED at 114
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/test/browser.ini.rej

+ one inline comment.

Honza

::: devtools/client/netmonitor/src/request-list-context-menu.js:213
(Diff revisions 1 - 2)
>        id: "request-list-context-open-in-style-editor",
>        label: L10N.getStr("netmonitor.context.openInStyleEditor"),
>        accesskey: L10N.getStr("netmonitor.context.openInStyleEditor.accesskey"),
>        visible: !!(selectedRequest &&
>                 selectedRequest.responseContent &&
> -               selectedRequest.cause.type === "stylesheet"),
> +               require("sdk/preferences/service").get("devtools.styleeditor.enabled") &&

Please don't use Add-on SDK modules. We are removing it from the code base (see also bug 1370172)

Use `Services.prefs.getBoolPref` instead
Attachment #8870519 - Flags: review?(odvarko) → review-
Comment on attachment 8870519 [details]
Bug 1361296 - Add Open in Debugger and Open in Style Editor in the netmonitor panel context menu;

https://reviewboard.mozilla.org/r/141954/#review154982

Looks great now, thanks!

Honza
Attachment #8870519 - Flags: review?(odvarko) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #13)
> Comment on attachment 8870519 [details]
> Bug 1361296 - Add Open in Debugger and Open in Style Editor in the
> netmonitor panel context menu;
> 
> https://reviewboard.mozilla.org/r/141954/#review154982
> 
> Looks great now, thanks!
> 
> Honza

Thank you for the review :-) Should I run tests? What is needed to land this?

Vincent
I just triggered a try push, let's see how it goes.
If all green we can land it.

Honza
There are some errors e.g https://treeherder.mozilla.org/logviewer.html#?job_id=110103968&repo=try&lineNumber=4769

> TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/netmonitor/test/browser_net_open_in_debugger.js:9:1 | Use the global form of 'use strict'. (strict)

The second line of the file's license is missing at the top.
(In reply to Vincent Lequertier from comment #17)
> The second line of the file's license is missing at the top.
Yeah, can you please update the patch, I'll retrigger.

Honza
Done
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78649ac1e5a6
Add Open in Debugger and Open in Style Editor in the netmonitor panel context menu; r=Honza
https://hg.mozilla.org/mozilla-central/rev/78649ac1e5a6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
This bug was about adding "Open in Debugger and Open In Style Editor" in context menu of NetMonitor and I have seen the feature being implemented with latest Nightly in Windows 7, 64 Bit!

This bug's fix is verified with latest Nightly!

Build ID   : 20170724030204
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170719]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.