|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
58 bytes, text/x-review-board-request
|Details | Review|
Move the RequestsMenuView class out of netmonitor-view.js into its own module. Move shared functions and constants to shared modules as needed.
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 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
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/888eb236f1b2 Move RequestsMenuView to its own module r=Honza
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.