change "windows" module to use EventEmitter event registration model

RESOLVED FIXED

Status

Add-on SDK
General
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: irakli, Assigned: irakli)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
Assignee: nobody → rFobic
Created attachment 480925 [details] [diff] [review]
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.
Attachment #480925 - Flags: review?(avarma)
Created attachment 481043 [details] [diff] [review]
v2

Forgot to include updates for docs
Attachment #480925 - Attachment is obsolete: true
Attachment #481043 - Flags: review?(avarma)
Attachment #480925 - Flags: review?(avarma)

Comment 4

8 years ago
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
Created attachment 481855 [details] [diff] [review]
v3
Attachment #481043 - Attachment is obsolete: true
Attachment #481855 - Flags: review?(avarma)

Comment 8

8 years ago
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

8 years ago
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
Last Resolved: 8 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

Comment 14

8 years ago
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.
Created attachment 487350 [details] [diff] [review]
v4

Please note that tests break on latest nightly with or without my changes and it looks like failures are caused by tab tests.
Attachment #481855 - Attachment is obsolete: true
Attachment #487350 - Flags: review?(myk)
P.S: tests will pass on nightly as well if tab tests won't be included.
Attachment #487350 - Flags: review?(myk) → review+
https://github.com/mozilla/addon-sdk/commit/3c483822c63fb7b44b39e8c81acdbc9da3620aa9
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 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.