That's odd. Both `test-window-utils` and `test-window-utils2` passes on Samsung Galaxy Tab 10.1 and Firefox 16.0. Erik, could you provide more information about this bug?

sorry test-window-observer not test-window-utils

Pointer to Github pull-request

Comment on attachment 677865 [details] Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/648 The patch doesn't solve the issue, it's basically mimic what Bug 804187 is for. This Bug should provide a proper implementation of the unit test, not skipping it. If the module itself doesn't support Fennec (so it's not just the test), then the module should raise a "Unsupported Application" exception (https://github.com/ZER0/addon-sdk/blob/selection/bug803027/lib/sdk/selection.js#L11-16) that will be catch properly by the test (https://github.com/ZER0/addon-sdk/blob/selection/bug803027/test/test-selection.js#L698-710). See also the pull in Bug 804187 for further details. Otherwise, if the module it supposed to work in Fennec, then this bug should be used to fix it.

I reopened this one in order to use the `needinfo` flag. Erik, as I wrote in my previous comment, this bug should be keep opened: it's not the same of bug 804187, here we should provide a proper implementation of `test-window-observer` for Fennec. If we close this bug it means we're not going to support `test-window-observer` on Fennec, because it doesn't make sense on that platform, and therefore a bug is not needed. If it's that the case, please write that in a comment. Otherwise, it should be leave open.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #6) > I reopened this one in order to use the `needinfo` flag. > > Erik, as I wrote in my previous comment, this bug should be keep opened: > it's not the same of bug 804187, here we should provide a proper > implementation of `test-window-observer` for Fennec. > If we close this bug it means we're not going to support > `test-window-observer` on Fennec, because it doesn't make sense on that > platform, and therefore a bug is not needed. > If it's that the case, please write that in a comment. Otherwise, it should > be leave open. Well the module should not throw an exception I think, because the code would work, if new windows were allowed to be opened, but they shouldn't be and often cause crashes or are not displayed correctly, so the module doesn't make sense for Fennec, and testing it would cause crashes if we opened real windows. This module depends on deprecated modules exclusively too, so I imagine writing tests for the module as it is, by providing fake input modules, would be messy task. This is a pattern that we try to avoid too I remember Myk saying to me once.

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #7) > Well the module should not throw an exception I think, because the code > would work, if new windows were allowed to be opened, but they shouldn't be > and often cause crashes or are not displayed correctly, so the module > doesn't make sense for Fennec, and testing it would cause crashes if we > opened real windows. Perfect. So, if I understood correctly, the module `windows/observer` is not compatible with Fennec, and shouldn't be used on that platform. Is it correct? In that case, I made it depends by bug 725409, so I will handle this module properly in that bug where I'm already handle the compatibility on several modules, adding Firefox as supported app (because there is no other details, I won't add Thunderbird for the time being); therefore instead have a "Unsupported Test" we'll have a "Unsupported Application". Let me know if I'm wrong, so I won't proceed in that way.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #8) The module works on Fennec, but it is useless there atm, so useless we can't test it. I don't know what that means for bug bug 725409.

Matteo can you do something about this one?

As previously said, if we're not going to implement `windows/observer` for Fennec, and there are no Fennec compatible module that depends on it, we can make the module not compatible with Fennec. However, it seems that it's used at least from `keyboard/observer` that is supposed to work on Fennec; so before add the compatibility flag we should refactoring the `keyboard/observer` in order to don't use the `windows/observer` – it should be easy now with the event streams. Because you implement the tabs / windows stuff on Fennec, are you aware of any other module that is supposed to be loaded in Fennec that requires `windows/observer`? If we're not going to implement this module on Fennec, I would change the priority, it doesn't seems to me a P1.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #11) > As previously said, if we're not going to implement `windows/observer` for > Fennec, and there are no Fennec compatible module that depends on it, we can > make the module not compatible with Fennec. > > However, it seems that it's used at least from `keyboard/observer` that is > supposed to work on Fennec; so before add the compatibility flag we should > refactoring the `keyboard/observer` in order to don't use the > `windows/observer` – it should be easy now with the event streams. > > Because you implement the tabs / windows stuff on Fennec, are you aware of > any other module that is supposed to be loaded in Fennec that requires > `windows/observer`? > > If we're not going to implement this module on Fennec, I would change the > priority, it doesn't seems to me a P1. I said in response to this in comment 9, "the module works on Fennec, but it is useless there atm, so useless we can't test it." This is grey area, not black or white, and the longer we wait here the longer bug 787352 takes, which means we end up waiting for bug 809143 longer.

I think there is a misunderstood Erik. This test is passing on Fennec, it's just skipped because it doesn't make sense run it: https://github.com/mozilla/addon-sdk/blob/master/test/test-window-observer.js#L50-L58 So, if we're not going to implement this module on Fennec, we're going to add the compatibility flag, and therefore we'll have something like: https://github.com/mozilla/addon-sdk/blob/master/test/test-panel.js#L852-L864 And as you can see there is really no so much difference in practical terms. It's more about decide if `windows/observer` should be supported by Fennec or not, but has nothing to do with bug 787352, because in both case we the test will pass – in one case we skip because is not implemented, in other case because the app is unsupported. That's also why to me is not a P1.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #13) > I think there is a misunderstood Erik. This test is passing on Fennec, it's > just skipped because it doesn't make sense run it: > > https://github.com/mozilla/addon-sdk/blob/master/test/test-window-observer. > js#L50-L58 Ah ok, I think we should rename this bug now then. This bug was created because the tests were running and failing. > So, if we're not going to implement this module on Fennec, we're going to > add the compatibility flag, and therefore we'll have something like: > > https://github.com/mozilla/addon-sdk/blob/master/test/test-panel.js#L852-L864 The module should work (and support) on Fennec, which it does, but opening new windows does not work, so we can't test the module really, we can test that requiring the module doesn't throw and that using the apis don't throw too though, that should be good enough. We should make a new bug for that..