Closed Bug 1474207 Opened 2 years ago Closed 2 years ago
Network Monitor response payload testing method variances
47 bytes, text/x-phabricator-request
|Details | Review|
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:61.0) Gecko/20100101 Firefox/61.0 Build ID: 20180704003137 Steps to reproduce: At the moment, we have several different ways of testing the contents of CodeMirror in the "Response payload" panel of the Developer Tools Network Monitor. 1. Using CodeMirror.getValue(): https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/devtools/client/netmonitor/test/browser_net_cyrillic-02.js#53 2. Selecting ".CodeMirror-line" nodes and joining their textContent: https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/devtools/client/netmonitor/test/browser_net_post-data-01.js#139 3. Grabbing textContent directly from the container: https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/devtools/client/netmonitor/test/browser_net_post-data-02.js#45 Unfortunately, the DOM access based methods (2 & 3) are non-deterministic since CodeMirror renders its text virtually. The lines rendered to the DOM depend on the current scroll position and what's visible. What's visible can change due changes unrelated to CodeMirror's container. For example: - In bug 1353319, the default height of the CodeMirror container div was changed since an additional HTML preview panel was added. - In bug 1428521, simply adding a horizontal scrollbar changed what lines CodeMirror chose to render to the DOM. - If the content of the testing fixture/payload shifts around, a test may fail since CodeMirror may not render that line. I think method 1 is the best since it grabs text that CodeMirror plans to render directly from memory. It works irrespective of the initial height of the developer tools, and where the tested payload is in the network response. Here's its documentation: https://codemirror.net/doc/manual.html#getValue I'd like to change all our network tests to use method 1, so changes unrelated to CodeMirror don't randomly fail. I'm a volunteer, so I won't start work on this until someone agrees with the above and confirms the bug.
Component: Untriaged → Netmonitor
Product: Firefox → DevTools
I should mention that I'm biased towards method 1 (CodeMirror.getValue) since I discovered it while working on a different bug. There may be problems with it I'm not seeing.
Thanks for the report and helping with the tests! I agree that #1 seems to be the right way - especially because the official doc does clearly say that this method is used to get the current editor content. So, yes #2 should use that too. However #3 is not related to code mirror. Check out the selector ".tree-section .treeLabel", it's a query for tree view component coming from DevTools code base: https://searchfox.org/mozilla-central/source/devtools/client/shared/components/tree Post data are not rendered using code mirror. Btw. this part is not virtualized, so there should be no problem with initial size. Anyway fixing #2 and avoid intermittent failures makes good sense to me! Honza
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Should I assign the bug to you? Honza
Assignee: nobody → brandon.cheng
Status: NEW → ASSIGNED
Assignee: brandon.cheng → nobody
Status: ASSIGNED → NEW
Assignee: nobody → sachdev.hemakshi
Status: NEW → ASSIGNED
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/f32615dcb330 Network Monitor response payload testing method variances. r=Honza
You need to log in before you can comment on or make changes to this bug.