Closed
Bug 1492730
Opened 6 years ago
Closed 6 years ago
DevTools Edit and resend broken when filter by XHR
Categories
(DevTools :: Netmonitor, defect, P3)
Tracking
(firefox64 fixed)
RESOLVED
FIXED
Firefox 64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: petcuandrei, Assigned: meag.harty, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: good-first-bug, Whiteboard: good-first-bug,)
Attachments
(1 file, 3 obsolete files)
4.29 KB,
patch
|
Honza
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:64.0) Gecko/20100101 Firefox/64.0 Build ID: 20180919220108 Steps to reproduce: Go to any web page that has XHR requests for example https://www.theguardian.com/politics/2018/sep/19/theresa-may-tells-eu27-she-wont-delay-brexit-despite-lack-of-a-deal Open dev tools, network monitor and filter requests by XHR Select an entry and hit edit and resend. Video https://vimeo.com/290841061 Actual results: The side panel gets closed instead of switching to edit and resend mode. Expected results: If instead of filtering by XHR, I select all requests then the edit and resend works fine. I expect edit and resend to work both with the filter and without.
Reporter | ||
Comment 1•6 years ago
|
||
This is a long standing bug. I reported it just now since it is the first time I got an actual reproducible list of steps. I used a new profile, not my main one.
Reporter | ||
Updated•6 years ago
|
Component: Untriaged → Netmonitor
Product: Firefox → DevTools
Reporter | ||
Comment 2•6 years ago
|
||
This affects Firefox stable v62 and ESR 60.2.0esr so it's not something recently introduced.
Comment 3•6 years ago
|
||
Thanks for the report! I can reproduce the problem on my machine Win10, Firefox Nightly 64 Honza
Comment 4•6 years ago
|
||
Some information for anyone interested in fixing this bug: 0) It all starts with filtered request list (XHR filter applied in this case) 1) When Edit & Resend action is executed a clone of the selected request is created. Here is the context menu handler: https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/widgets/RequestListContextMenu.js#183 CLONE_SELECTED_REQUEST action is fire handled here: https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/reducers/requests.js#120 2) You can see that not every prop is cloned here: https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/reducers/requests.js#133 The new clone is also used as the new current selection: https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/reducers/requests.js#147 3) When a new request appears in the list (a clone in this case) the list is automatically filtered These functions are used for filtering: https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/utils/filter-predicates.js#10 But, these functions use proper that are not available in the clone (e.g. mimeType). So, the clone is filtered out and thus the current selected request (clone) is not visible. That also means that the Details panel is auto-closed: https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/components/MonitorPanel.js#84 ... and that's the bug 4) One solution is to always treat a clone as something that should *not* be filtered out. This approach seems to be fine since already used for sorting: https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/selectors/requests.js#11-15 The filtering selector function is here: https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/selectors/requests.js#38-46 Perhaps we could do something like as follows (or better introduce filterWithClone function): const getFilterFn = createSelector( state => state.filters, filters => r => { const matchesType = Object.keys(filters.requestFilterTypes).some(filter => { if (r.id.endsWith("-clone")) { return true; } return filters.requestFilterTypes[filter] && Filters[filter] && Filters[filter](r); }); return matchesType && isFreetextMatch(r, filters.requestFilterText); } ); This also needs a test case: This one might be used as an inspiration: https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/test/browser_net_edit_resend_caret.js#13 Honza
Blocks: netmonitor-edit-and-resend
Mentor: odvarko
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: good-first-bug,
Assignee | ||
Comment 5•6 years ago
|
||
Hi! I'm interested in working on this bug. Meag
Updated•6 years ago
|
Assignee: nobody → meag.harty
Status: NEW → ASSIGNED
Comment 6•6 years ago
|
||
@Meag: Hi, any progress on this bug? Are you still working on it? Honza
Flags: needinfo?(meag.harty)
Assignee | ||
Comment 7•6 years ago
|
||
Almost ready for a patch submission. I will look to submit one this evening. thanks!
Flags: needinfo?(meag.harty)
Comment 8•6 years ago
|
||
(In reply to meag.harty from comment #7) > Almost ready for a patch submission. I will look to submit one this evening. > thanks! Awesome! Honza
Assignee | ||
Comment 9•6 years ago
|
||
Your bug analysis was spot on, so this was a simple fix. Had some fun with the mochitests, let me know what you think. Thanks!
Attachment #9013883 -
Flags: review?(odvarko)
Assignee | ||
Comment 10•6 years ago
|
||
Comment on attachment 9013883 [details] [diff] [review] bug1492730-patch ># HG changeset patch ># User meagonqz <meag.harty@gmail.com> ># Date 1537849869 14400 ># Tue Sep 25 00:31:09 2018 -0400 ># Node ID 893521e25268dd3e5bd78c4d09df57c6c55c4824 ># Parent 221c18ebe962f68358b4cba927df9099ea935b40 >Bug 1492730 - network: show resent XHR when filtering r=honza > >diff --git a/devtools/client/netmonitor/src/selectors/requests.js b/devtools/client/netmonitor/src/selectors/requests.js >--- a/devtools/client/netmonitor/src/selectors/requests.js >+++ b/devtools/client/netmonitor/src/selectors/requests.js >@@ -35,10 +35,13 @@ function sortWithClones(requests, sorter > return sorter(a, b); > } > >-const getFilterFn = createSelector( >+const getFilterWithCloneFn = createSelector( > state => state.filters, > filters => r => { > const matchesType = Object.keys(filters.requestFilterTypes).some(filter => { >+ if (r.id.endsWith("-clone")) { >+ return true; >+ } > return filters.requestFilterTypes[filter] && Filters[filter] && Filters[filter](r); > }); > return matchesType && isFreetextMatch(r, filters.requestFilterText); >@@ -78,7 +81,7 @@ const getSortedRequests = createSelector > > const getDisplayedRequests = createSelector( > state => state.requests, >- getFilterFn, >+ getFilterWithCloneFn, > getSortFn, > ({ requests }, filterFn, sortFn) => { > const arr = [...requests.values()].filter(filterFn).sort(sortFn); >diff --git a/devtools/client/netmonitor/test/browser.ini b/devtools/client/netmonitor/test/browser.ini >--- a/devtools/client/netmonitor/test/browser.ini >+++ b/devtools/client/netmonitor/test/browser.ini >@@ -120,6 +120,7 @@ subsuite = clipboard > skip-if = (os == 'mac') # Bug 1479782 > [browser_net_header-docs.js] > [browser_net_edit_resend_caret.js] >+[browser_net_edit_resend_with_filtering.js] > [browser_net_filter-01.js] > [browser_net_filter-02.js] > [browser_net_filter-03.js] >diff --git a/devtools/client/netmonitor/test/browser_net_edit_resend_with_filtering.js b/devtools/client/netmonitor/test/browser_net_edit_resend_with_filtering.js >new file mode 100644 >--- /dev/null >+++ b/devtools/client/netmonitor/test/browser_net_edit_resend_with_filtering.js >@@ -0,0 +1,67 @@ >+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ >+/* vim: set ft=javascript ts=2 et sw=2 tw=80: */ >+/* Any copyright is dedicated to the Public Domain. >+ * http://creativecommons.org/publicdomain/zero/1.0/ */ >+ >+"use strict"; >+ >+/** >+* Tests if resending a XHR request while filtering X displays >+* the correct requests >+*/ >+ >+add_task(async function() { >+ const { tab, monitor } = await initNetMonitor(POST_RAW_URL); >+ >+ const { document, store, windowRequire, parent } = monitor.panelWin; >+ const parentDocument = parent.document; >+ const Actions = windowRequire("devtools/client/netmonitor/src/actions/index"); >+ store.dispatch(Actions.batchEnable(false)); >+ >+ // Execute 1 XHR requests >+ await performRequests(monitor, tab, 1); >+ >+ // Filter by XHR >+ info("Filtering by XHR... "); >+ EventUtils.sendMouseEvent({ type: "click" }, >+ document.querySelector(".requests-list-filter-xhr-button")); >+ >+ // Confirm XHR request and click it >+ info("Confirming XHR request... "); >+ const xhrRequestItem = document.querySelectorAll(".request-list-item")[0]; >+ const waitForItem = waitUntil(() => xhrRequestItem); >+ await waitForItem; >+ info("Clicking XHR request... "); >+ EventUtils.sendMouseEvent({ type: "mousedown" }, xhrRequestItem); >+ >+ const { >+ getSelectedRequest >+ } = windowRequire("devtools/client/netmonitor/src/selectors/index"); >+ const firstRequest = getSelectedRequest(store.getState()); >+ >+ // Open context menu and execute "Edit & Resend". >+ EventUtils.sendMouseEvent({ type: "contextmenu" }, xhrRequestItem); >+ const contextMenuResend = parentDocument.querySelector("#request-list-context-resend"); >+ info("Clicking resend menu... "); >+ contextMenuResend.click(); >+ >+ // Click Resend >+ const resendButton = document.querySelector("#custom-request-send-button"); >+ await waitUntil(() => resendButton); >+ resendButton.click(); >+ >+ // Filtering by "other" so the resent request is visible after completion >+ EventUtils.sendMouseEvent({ type: "click" }, >+ document.querySelector(".requests-list-filter-other-button")); >+ >+ // Select the cloned request >+ const cloneRequestItem = document.querySelectorAll(".request-list-item")[0]; >+ cloneRequestItem.click(); >+ const resendRequest = getSelectedRequest(store.getState()); >+ >+ ok(resendRequest.id !== firstRequest.id, "The second XHR request was made and is unique"); >+ >+ ok(resendRequest.id.replace(/-clone$/, "") == firstRequest.id, "The second XHR request is a clone of the first"); >+ >+ return teardown(monitor); >+});
Assignee | ||
Comment 11•6 years ago
|
||
oops! This is the correct diff.
Attachment #9013883 -
Attachment is obsolete: true
Attachment #9013883 -
Flags: review?(odvarko)
Attachment #9013885 -
Flags: review?(odvarko)
Comment 12•6 years ago
|
||
Comment on attachment 9013885 [details] [diff] [review] bug1492730-patch-2 Review of attachment 9013885 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch, it fixes the problem for me! Please see my inline comments related to the test. Also, can you please look at bug 1432724, it looks like duplicate. Honza ::: devtools/client/netmonitor/src/selectors/requests.js @@ +35,4 @@ > return sorter(a, b); > } > > +const getFilterWithCloneFn = createSelector( Please create a comment in front of the method. Something like: /** * Take clones into account when filtering. If a request is * a clone it's not filtered out. */ ::: devtools/client/netmonitor/test/browser_net_edit_resend_with_filtering.js @@ +9,5 @@ > +* Tests if resending a XHR request while filtering X displays > +* the correct requests > +*/ > + > +add_task(async function() { Please format the comment as: /** * */ And remove the empty line between the comment and the function name @@ +23,5 @@ > + > + // Filter by XHR > + info("Filtering by XHR... "); > + EventUtils.sendMouseEvent({ type: "click" }, > + document.querySelector(".requests-list-filter-xhr-button")); What this querySelector is supposed to do? @@ +26,5 @@ > + EventUtils.sendMouseEvent({ type: "click" }, > + document.querySelector(".requests-list-filter-xhr-button")); > + > + // Confirm XHR request and click it > + info("Confirming XHR request... "); Please remove the trailing space in the log (after three dots). This repeats in several other places. @@ +29,5 @@ > + // Confirm XHR request and click it > + info("Confirming XHR request... "); > + const xhrRequestItem = document.querySelectorAll(".request-list-item")[0]; > + const waitForItem = waitUntil(() => xhrRequestItem); > + await waitForItem; Why you awaiting the return value from querySelector? It's either there or not, but not changing over time. @@ +46,5 @@ > + contextMenuResend.click(); > + > + // Click Resend > + const resendButton = document.querySelector("#custom-request-send-button"); > + await waitUntil(() => resendButton); Again why the await here? Can we just remove that? @@ +51,5 @@ > + resendButton.click(); > + > + // Filtering by "other" so the resent request is visible after completion > + EventUtils.sendMouseEvent({ type: "click" }, > + document.querySelector(".requests-list-filter-other-button")); Similarly as before what this querySelector is supposed to do? You are not using its return value...
Attachment #9013885 -
Flags: review?(odvarko)
Comment 13•6 years ago
|
||
Just to make sure you know.. The following bug is related and could be your next: Bug 1377741 - Edit and resend doesn't work with filtered content Honza
QA Contact: odvarko
Updated•6 years ago
|
Assignee: meag.harty → nobody
Status: ASSIGNED → NEW
Updated•6 years ago
|
Assignee: nobody → meag.harty
Status: NEW → ASSIGNED
QA Contact: odvarko
Assignee | ||
Comment 14•6 years ago
|
||
Fixed up the comment style, fixed up a few missing indentations that made the use of querySelectors unclear (they were part of a sending a mouseEvent), and removed the info logging in favor of comments. I removed what I intended to be the waiting for the querySelector to be truthy since locally the tests passed fine without it; I should have made the value passed to waitUntil( () => querySelector("#item") ) without the predefined variable.
Attachment #9013885 -
Attachment is obsolete: true
Attachment #9015147 -
Flags: review?(odvarko)
Assignee | ||
Comment 15•6 years ago
|
||
Small comment fixup and don't use EventUtils.sendMouseEvent for click events
Attachment #9015156 -
Flags: review?(odvarko)
Assignee | ||
Comment 16•6 years ago
|
||
Comment on attachment 9015147 [details] [diff] [review] bug1492730-patch3 ># HG changeset patch ># User meagonqz <meag.harty@gmail.com> ># Date 1537849869 14400 ># Tue Sep 25 00:31:09 2018 -0400 ># Node ID c71f97397718b8cd392980c5b47045a0930db980 ># Parent 221c18ebe962f68358b4cba927df9099ea935b40 >Bug 1492730 - network: show resent XHR when filtering r=honza > >diff --git a/devtools/client/netmonitor/src/selectors/requests.js b/devtools/client/netmonitor/src/selectors/requests.js >--- a/devtools/client/netmonitor/src/selectors/requests.js >+++ b/devtools/client/netmonitor/src/selectors/requests.js >@@ -35,10 +35,17 @@ function sortWithClones(requests, sorter > return sorter(a, b); > } > >-const getFilterFn = createSelector( >+/** >+ * Take clones into account when filtering. If a request is >+ * a clone, it's not filtered out. >+ */ >+const getFilterWithCloneFn = createSelector( > state => state.filters, > filters => r => { > const matchesType = Object.keys(filters.requestFilterTypes).some(filter => { >+ if (r.id.endsWith("-clone")) { >+ return true; >+ } > return filters.requestFilterTypes[filter] && Filters[filter] && Filters[filter](r); > }); > return matchesType && isFreetextMatch(r, filters.requestFilterText); >@@ -78,7 +85,7 @@ const getSortedRequests = createSelector > > const getDisplayedRequests = createSelector( > state => state.requests, >- getFilterFn, >+ getFilterWithCloneFn, > getSortFn, > ({ requests }, filterFn, sortFn) => { > const arr = [...requests.values()].filter(filterFn).sort(sortFn); >diff --git a/devtools/client/netmonitor/test/browser.ini b/devtools/client/netmonitor/test/browser.ini >--- a/devtools/client/netmonitor/test/browser.ini >+++ b/devtools/client/netmonitor/test/browser.ini >@@ -120,6 +120,7 @@ subsuite = clipboard > skip-if = (os == 'mac') # Bug 1479782 > [browser_net_header-docs.js] > [browser_net_edit_resend_caret.js] >+[browser_net_edit_resend_with_filtering.js] > [browser_net_filter-01.js] > [browser_net_filter-02.js] > [browser_net_filter-03.js] >diff --git a/devtools/client/netmonitor/test/browser_net_edit_resend_with_filtering.js b/devtools/client/netmonitor/test/browser_net_edit_resend_with_filtering.js >new file mode 100644 >--- /dev/null >+++ b/devtools/client/netmonitor/test/browser_net_edit_resend_with_filtering.js >@@ -0,0 +1,59 @@ >+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ >+/* vim: set ft=javascript ts=2 et sw=2 tw=80: */ >+/* Any copyright is dedicated to the Public Domain. >+ * http://creativecommons.org/publicdomain/zero/1.0/ */ >+ >+"use strict"; >+ >+/** >+ * Tests if resending a XHR request while filtering XHR displays >+ * the correct requests >+ */ >+add_task(async function() { >+ const { tab, monitor } = await initNetMonitor(POST_RAW_URL); >+ >+ const { document, store, windowRequire, parent } = monitor.panelWin; >+ const parentDocument = parent.document; >+ const Actions = windowRequire("devtools/client/netmonitor/src/actions/index"); >+ store.dispatch(Actions.batchEnable(false)); >+ >+ // Execute 1 XHR requests >+ await performRequests(monitor, tab, 1); >+ EventUtils.sendMouseEvent({ type: "click" }, >+ document.querySelector(".requests-list-filter-xhr-button")); >+ >+ // Confirm XHR request and click it >+ const xhrRequestItem = document.querySelectorAll(".request-list-item")[0]; >+ EventUtils.sendMouseEvent({ type: "mousedown" }, xhrRequestItem); >+ >+ const { >+ getSelectedRequest >+ } = windowRequire("devtools/client/netmonitor/src/selectors/index"); >+ const firstRequest = getSelectedRequest(store.getState()); >+ >+ // Open context menu and execute "Edit & Resend". >+ EventUtils.sendMouseEvent({ type: "contextmenu" }, xhrRequestItem); >+ EventUtils.sendMouseEvent({ type: "click" }, >+ parentDocument.querySelector("#request-list-context-resend")); >+ >+ // Click Resend >+ EventUtils.sendMouseEvent({ type: "click" }, >+ document.querySelector("#custom-request-send-button")); >+ >+ // Filtering by "other" so the resent request is visible after completion >+ EventUtils.sendMouseEvent({ type: "click" }, >+ document.querySelector(".requests-list-filter-other-button")); >+ >+ // Select the cloned request >+ EventUtils.sendMouseEvent({ type: "click" }, >+ document.querySelectorAll(".request-list-item")[0]); >+ const resendRequest = getSelectedRequest(store.getState()); >+ >+ ok(resendRequest.id !== firstRequest.id, >+ "The second XHR request was made and is unique"); >+ >+ ok(resendRequest.id.replace(/-clone$/, "") == firstRequest.id, >+ "The second XHR request is a clone of the first"); >+ >+ return teardown(monitor); >+});
Attachment #9015147 -
Attachment is obsolete: true
Attachment #9015147 -
Flags: review?(odvarko)
Comment 17•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a70f042f65f0a93512c20dcde65053697cf62b8d
Comment 18•6 years ago
|
||
Comment on attachment 9015156 [details] [diff] [review] bug1492730-patch4 Review of attachment 9015156 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! R+ assuming try is green Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=41a1f0334f9a23df8e36112690d23ca221ec02e3 Thanks for the help. Honza
Attachment #9015156 -
Flags: review?(odvarko) → review+
Comment 19•6 years ago
|
||
@Meag: try push is green and the next step is marking the bug as `checkin-needed` so somebody can land it in the repo. I am doing it now for you, so you can see how it's done. You can do it yourselves next time. Thanks for the help! Honza
Keywords: checkin-needed
Comment 20•6 years ago
|
||
Pushed by aciure@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c6e6514203c9 network: show resent XHR when filtering r=honza
Keywords: checkin-needed
Comment 21•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c6e6514203c9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in
before you can comment on or make changes to this bug.
Description
•