598980 - change "windows" module to use EventEmitter event registration model

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: v1

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.

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

pull request on github: http://github.com/Gozala/jetpack-sdk/pull/11

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: v2

Forgot to include updates for docs

Comment 4

[Atul Varma [:atul]]

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

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(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

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: v3

Comment 8

[Atul Varma [:atul]]

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 9

[Atul Varma [:atul]]

Comment on attachment 481855 [details] [diff] [review]
v3

Looks good, thanks!

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Do you mind copying them, I would've do it myself but they will be from my name then.  

fixed by changeset: d1b2b899440d

Comment 11

[Myk Melez [:myk] [@mykmelez]]

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?

Comment 12

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I'm working on it.

Comment 13

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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

Comment 14

[Atul Varma [:atul]]

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.

Comment 15

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: v4

Please note that tests break on latest nightly with or without my changes and it looks like failures are caused by tab tests.

Comment 16

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

P.S: tests will pass on nightly as well if tab tests won't be included.

Comment 17

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

https://github.com/mozilla/addon-sdk/commit/3c483822c63fb7b44b39e8c81acdbc9da3620aa9

Comment 18

[Myk Melez [:myk] [@mykmelez]]

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".