Closed Bug 598980 Opened 10 years ago Closed 10 years ago
change "windows" module to use Event
Emitter event registration model
No description provided.
Assignee: nobody → rFobic
I have merged this window module with some work I have done with the side-bar. Please note that behavior of `openWindow` changed slightly. In original implementation onOpen option passed to the `openWindow` was waiting for the tab to be loaded even though doc's where stating opposite. This was also inconsistent with onOpen handlers of `browserWindows` since they where called before any tab where loaded. I changed this so that 'open' events are emitted when window are loaded and 'ready' events are emitted when the tab content is loaded.
pull request on github: http://github.com/Gozala/jetpack-sdk/pull/11
Forgot to include updates for docs
Comment on attachment 481043 [details] [diff] [review] v2 Hmm, what version of Firefox are these changes supposed to work with? For Firefox 4.0b6, I get these errors from 'cfx testall': test-windows.testOpenAndCloseWindow: failure test-windows.testOnOpenOnCloseListeners: failure test-windows.testActiveWindow: failure My code review comments are on your original pull request: http://github.com/Gozala/jetpack-sdk/pull/11
Attachment #481043 - Flags: review?(avarma) → review-
So first of all all comments can be found here (I have renamed repo in order to fork the mozillalabs one) http://github.com/Gozala/jetpack-sdk.back/pull/11
(In reply to comment #4) > Comment on attachment 481043 [details] [diff] [review] > v2 > > Hmm, what version of Firefox are these changes supposed to work with? For > Firefox 4.0b6, I get these errors from 'cfx testall': > > test-windows.testOpenAndCloseWindow: failure > test-windows.testOnOpenOnCloseListeners: failure > test-windows.testActiveWindow: failure > > My code review comments are on your original pull request: > > http://github.com/Gozala/jetpack-sdk/pull/11 Yes I had some issues related to the package splitting not sure what happened but I had to update patch to match new dir structure anyway so I did it and all window tests pass on 3.6, 4b6 and nightly. But there is some test for widget that fails both with or without my change and if you run test all it fails some of my tests cause widget failure leaves one of the windows open there for number of windows is different form what test expects. I have addressed all the issues you noted in the review. http://github.com/mozillalabs/jetpack-sdk/pull/11
Thanks, will the link you mentioned in comment 5 be permanent? It would be bad if all these code review comments can't be seen by anyone looking at this bug in the future...
Comment on attachment 481855 [details] [diff] [review] v3 Looks good, thanks!
Attachment #481855 - Flags: review?(avarma) → review+
Do you mind copying them, I would've do it myself but they will be from my name then. fixed by changeset: d1b2b899440d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
I backed this out, as it was causing a test failure in bug 604801 that was blocking the 0.9 release. Here are the changesets that backed out the patch: https://hg.mozilla.org/labs/jetpack-sdk/rev/28f897d10b20 https://hg.mozilla.org/labs/jetpack-sdk/rev/e80ffb9aa4ca https://hg.mozilla.org/labs/jetpack-sdk/rev/761c5181ec91 https://hg.mozilla.org/labs/jetpack-sdk/rev/378391fa4783 Reopening, now that this is no longer fixed. Irakli: can you take the lead on figuring out the issues here and submitting an updated patch that addresses them?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm working on it.
it seems that issue occurs in combination of: cfx testall -F 'windows|tab-browser' and test that hang the browser are: testEventsAndLengthStayInModule testTabModuleActiveTab_getterAndSetter form test-tab-browser I'll be working further to find an actual cause of this
It could be a complete coincidence, but bug 601242 also mentions that testEventsAndLengthStayInModule has a dependency on the 'tabs' module, which is unfortunate because 'tabs' is from addon-kit, which jetpack-core cannot depend on lest a circular package dependency chain be created.
Please note that tests break on latest nightly with or without my changes and it looks like failures are caused by tab tests.
P.S: tests will pass on nightly as well if tab tests won't be included.
Attachment #487350 - Flags: review?(myk) → review+
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product. To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.