1109203 - current_audio_channel getter switches context to the system app, but doesn't return it back to the visible app

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::UI Tests, defect)

Comment 1

[Martijn Wargers (dead)]

Attachment: audio

Comment 2

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 8533918 [details] [review]
audio

r-, per comment in PR. 

The current code always leaves the getter with displayed-app context, which isn't a big improvement over leaving with system context. 

Since it's actually a little more variable, it might be a bit worse because it means if the test calls a function, that calls a function, that uses the getter, now that twice-indirect code has to know what app the test left current to know whether the context is usable. That becomes a problem when writing general library functions because you have to pass around a lot of state into them.

I'd much prefer to see us save off current context, whatever it is, and restore that context at the end of the getter. If we can't do that, I'd prefer we WONTFIX this and leave the side effect at setting to system context, since at least every piece of code will unambiguously know where we're at.

Comment 3

[Martijn Wargers (dead)]

I guess I should use get_active_frame() for this.
http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py#995
There are gaia-ui-test unit tests here, it seems:
http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/tests/unit/
I think it would be also good to add a unit test for current_audio_channel, that it works as expected in various circumstances (dialogs, homescreen, etc).

Comment 4

[Martijn Wargers (dead)]

Attachment: audio2

Comment 5

[Martijn Wargers (dead)]

I think there are more functions/properties in gaia_test.py where the context is changed, where you wouldn't expect it, I filed bug 1115905 for it.

Comment 6

[Martijn Wargers (dead)]

See bug 1115905, comment 4.
"
(In reply to Martijn Wargers [:mwargers] (QA) from comment #3)
> Dave, can't we use self.marionette.get_active_frame() to keep a reference to
> the old frame and then switch back to it after we're done? (like I used in
> the patch in bug 1109203)

Only if the active frame is not a nested one, otherwise you won't be able to switch directly to it from the system app frame - you'd first need to switch to it's ancestor available from the system app and then through each of it's ancestors until you can reach it.
"

So this isn't guaranteed to work, especially in nested frames. Not sure when that happens, I guess in webapps like Marketplace.

Comment 7

[Martijn Wargers (dead)]

For the cases where we're using this, it's working, though.

Comment 8

[Florin Strugariu [:Bebe]]

The changes look good to me.

Let's see how the addhoc runs 
http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/542/

Comment 9

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 8541871 [details] [review]
audio2

(In reply to Martijn Wargers [:mwargers] (QA) from comment #7)
> For the cases where we're using this, it's working, though.

Hate to say it, but that's reason enough to r- this.

It's an API, it has to work sanely for the general case from any caller. We can't consider just what the tests are doing today in terms of correctness. 

And we can't just document "use it in this case but not in this case" because of the indirect call issue. If I call some other API that wants to call this one, now you have to tip off the first API as to whether it's safe to use it or what.

I see a few options besides that one:

1) We try to walk up nested contexts beforehand and switch back down them at the end, per Dave's comment. 

If it's just a matter of looking recursively at a parent attribute until you get to the top that might actually be easily doable--save them in a list, then pop the list until you're back to where you started. This would be something we'd implement as a general utility function, possibly at the marionette level.

2) We document that it leaves you at current-app context. Calling code needs to restore its own context. In other words, we go to the first solution you posted, along with a side-effect comment.

What I don't like about this is that it effectively randomizes the context if there's an indirect call chain, per above, as otherwise every caller needs to know what the top level app would be.

3) We document that it leaves you at system context. Calling code needs to restore its own context. In other words, aside from adding a side-effect comment we leave it alone. 

This has the advantage that the call chain just has to document "leaves you in system context" upwards from this call. No randomization.

4) We document that it leaves you at an unknown context, and context must be restored by calling code. Similar advantages to 3, only we're not even promising system context at that point. 

This has the advantage that we can change it later, make it reliable, and any existing code will still do the right thing because they weren't counting on anything at all.

I'd be up for dropping this bug this until/unless we can implement (or request) option 1. 

But in the meantime, I'd pick option 4: just add a single comment saying don't count on any particular context after returning, and make sure existing tests aren't counting on system context.

Comment 10

[Martijn Wargers (dead)]

For current_audio_channel, it is switching to the system app. I think that's almost always the case for the cases in bug 1115905.
I guess you mean every time you use current_audio, you want a comment saying that it causes marionette to switch to the system app?

I'm not sure how easy it is to crawl upwards from sandboxed iframes, but I guess that's the way to go to get this fixed, so option 1 then.

Comment 11

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

I'm actually recommending that we comment that it leaves your context undefined.

In truth, right now it does switch you to the system app, but by making no promises whatsoever, means we can modify it later to do the "right thing" without breaking existing code.

If we document that it switches to system context, now it really needs to do so forever since we've promised potential client code that's what it'll do.

API writing is tricky that way. You don't want clients (test code, in this case) to count on what the code actually does behind the scenes--only on what you document it'll do. That's how you keep things from being too coupled to maintain.

Comment 12

[Martijn Wargers (dead)]

Marking WONTFIX, sorry for the bug spam. If somebody still wants to work on this, please file a new bug for it.