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)
DevTools
Netmonitor
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.
Reporter | ||
Updated•7 years ago
|
Keywords: dev-doc-needed,
good-first-bug
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•7 years ago
|
||
Hello, Can I work on this ? :-) Vincent
Reporter | ||
Comment 2•7 years ago
|
||
(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
Assignee | ||
Comment 3•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
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
Assignee | ||
Comment 6•7 years ago
|
||
Who should I put as a reviewer of the patch? Vincent
Reporter | ||
Updated•7 years ago
|
Attachment #8870519 -
Flags: review?(odvarko)
Comment 7•7 years ago
|
||
mozreview-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/#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-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
(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
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
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 11•7 years ago
|
||
mozreview-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/#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 hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 14•7 years ago
|
||
(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
Comment 15•7 years ago
|
||
I just triggered a try push, let's see how it goes. If all green we can land it. Honza
Assignee | ||
Comment 17•7 years ago
|
||
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.
Comment 18•7 years ago
|
||
(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
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
Done
Comment 21•7 years ago
|
||
New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ae10e6b46d99781a7c155809cbeea32db7807aa Honza
Comment 22•7 years ago
|
||
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
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/78649ac1e5a6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment 24•7 years ago
|
||
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]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•