Move RequestsMenuView to its own module

VERIFIED FIXED in Firefox 52

Status

()

Firefox
Developer Tools: Netmonitor
P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: jsnajdr, Assigned: jsnajdr)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified)

Details

(Whiteboard: [netmonitor])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
Move the RequestsMenuView class out of netmonitor-view.js into its own module.

Move shared functions and constants to shared modules as needed.
(Assignee)

Updated

a year ago
Assignee: nobody → jsnajdr
Blocks: 1307743
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
Moved the RequestsMenuView class away from netmonitor-view.js to a separate requests-menu-view.js module. Unfortunately, the diff doesn't detect that X contiguous lines were cut out of netmonitor-view.js and displays a chaotic list of changes.

Extracted a "l10n" module, as it's now required by multiple modules. Changed the tests to import the L10N symbols from the right place. That's why so many test files are changed in this patch.

Extracted a "prefs" module, and defined it as a global in netmonitor-controller.js. If I didn't have to keep tests compatible, I would just "require" it in every module that needs it.

Started using a BrowserLoader in netmonitor-controller.js - this makes "document" and "window" available in modules. Useful for compatibility, and also for future React imports.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1308495
Whiteboard: [netmonitor]
Flags: qe-verify+
QA Contact: ciprian.georgiu
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Blocks: 1308480

Comment 4

a year ago
mozreview-review
Comment on attachment 8799671 [details]
Bug 1309120 - Move RequestsMenuView to its own module

https://reviewboard.mozilla.org/r/84804/#review83752

Changes in this patch are a bit hard to follow (patch is mixed up), but I don't see anything wrong.
I rely mostly on tests and try seems to be nicely green (at 99%).

Interesting is that two tests fail for me on Win (not on Mac) when running locally,
but they alrady failed before so, the problem is not caused by this patch.

Just for the record:

29704 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_charts-03.js | The second sum's value is correct. - Got World 36,60, expected World 36.60
Stack trace:
    chrome://mochikit/content/browser-test.js:test_is:905
    chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_charts-03.js:null:101
    Tester_execTest@chrome://mochikit/content/browser-test.js:729:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:649:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:743:59
29705 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_timing-division.js | The division on the seconds time scale looks legit. -
Stack trace:
    chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_timing-division.js:null:57
    Tester_execTest@chrome://mochikit/content/browser-test.js:729:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:649:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:743:59
SUITE-END | took 132s

Honza
Attachment #8799671 - Flags: review?(odvarko) → review+

Comment 5

a year ago
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/888eb236f1b2
Move RequestsMenuView to its own module r=Honza

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/888eb236f1b2
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Iteration: --- → 52.2 - Oct 17
Priority: -- → P1
Just comment (In reply to Jan Honza Odvarko [:Honza] from comment #4)
> Comment on attachment 8799671 [details]
> Bug 1309120 - Move RequestsMenuView to its own module
> 
> https://reviewboard.mozilla.org/r/84804/#review83752
> 
> Changes in this patch are a bit hard to follow (patch is mixed up), but I
> don't see anything wrong.
> I rely mostly on tests and try seems to be nicely green (at 99%).
> 
> Interesting is that two tests fail for me on Win (not on Mac) when running
> locally,
So, this is because I am using Czech Win OS and tests are supposed to be run in en-US

Honza
This issue is verified fixed on latest Nightly 52.0a1 (2016-10-16) under the following OSes:
- Windows 10 x64
- Ubuntu 12.04 x86 LTS
- Mac OS X 10.11.4 
Requests-menu-view works as expected and nothing is broken. I will mark here accordingly.
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.