1309120 - Move RequestsMenuView to its own module

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Jarda Snajdr [:jsnajdr]]

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

Move shared functions and constants to shared modules as needed.

Comment 1

[Jarda Snajdr [:jsnajdr]]

Attachment: Bug 1309120 - Move RequestsMenuView to its own module

Review commit: https://reviewboard.mozilla.org/r/84804/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84804/

Comment 2

[Jarda Snajdr [:jsnajdr]]

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 3

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8799671 [details]
Bug 1309120 - Move RequestsMenuView to its own module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84804/diff/1-2/

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/888eb236f1b2
Move RequestsMenuView to its own module r=Honza

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/888eb236f1b2

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 8

[Ciprian Georgiu, Desktop QA]

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.