Some of these methods that use atoms assume they are called from the system frame. Let's follow where we have in other cases and make them switch to the frame first.

Rob, would not be surprised if this busts some assumptions in existing tests so we will test with caution (on functional tests side)

Comment on attachment 8416503 [details] github pr Fix works great except it breaks the fmradio_play endurance test, after I noticed that I verified that it also breaks the functional fmradio tests (makes sense). I did not test any of the other functional tests. For the endurance test (https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/endurance/test_endurance_fmradio_play.py), if you could please add a "self.apps.switch_to_displayed_app()" just before line 38 and that will fix it. Thanks Zac!

Comment on attachment 8416503 [details] github pr I have added a question and a discussion point to the PR.

:rwood, yeah not surprised it broke something! I'll fix it up later today and do a full functional test run.

This is an approach that I think might be good for dealing with functions that need to be executed from the system frame. This decorator would first switch to the system frame, then call the decorated function, then switch back to whatever frame was active before the system frame was switched to. If we use this decorator approach we both save from having to include all the frame switching code in each function, and we also remove the need for a consumer of any of these methods to know which frame they will be in after the function returns. Ideally, if a function is switching away from the current frame it should also switch back.

Comment on attachment 8418041 [details] Link to a branch that shows decorator usage I like this, however I believe this will fail if we're in a nested frame. If we can either detect and fail early in that scenario or somehow support switching back to the nested frame, this would be a really nice enhancement!

@davehunt: Will it fail because switch_to_frame() doesn't work with nested frames, or because get_active_frame() doesn't return the correct frame when it is a nested frame? Either of those sound like marionette issues that should be addressed (and maybe there is even an open bug for them).

(In reply to Bob Silverberg [:bsilverberg] from comment #7) > @davehunt: Will it fail because switch_to_frame() doesn't work with nested > frames, or because get_active_frame() doesn't return the correct frame when > it is a nested frame? Either of those sound like marionette issues that > should be addressed (and maybe there is even an open bug for them). I don't recall ever looking into this myself, so :zac might be a better person to ask. I believe the issue is that get_active_frame will simply return the frame element, which may not be available from the top frame. You would somehow need to know a frame's ancestors and methodically switch through each of these.

(In reply to Dave Hunt (:davehunt) from comment #8) > (In reply to Bob Silverberg [:bsilverberg] from comment #7) > > @davehunt: Will it fail because switch_to_frame() doesn't work with nested > > frames, or because get_active_frame() doesn't return the correct frame when > > it is a nested frame? Either of those sound like marionette issues that > > should be addressed (and maybe there is even an open bug for them). > > I don't recall ever looking into this myself, so :zac might be a better > person to ask. I believe the issue is that get_active_frame will simply > return the frame element, which may not be available from the top frame. You > would somehow need to know a frame's ancestors and methodically switch > through each of these. Yes that's correct. This bump is the reason we have not used this technique sooner throughout gaiatest. Bob I recall you hitting this in bug 937336. I'll have a look at the commit closer a bit later but as we can't switch into nested frames it seems we'll never entirely avoid some frame switching inside the test case, but we can at least have gaiatest do the majority of it automatically. It's a decision between allowing gaiatest to do some stuff automatically or leaving it entirely to be explicitly done by the test author.

Comment on attachment 8418041 [details] Link to a branch that shows decorator usage Doesn't really seem that ground breaking but it looks OK, let's give it a shot! f+

Comment on attachment 8418041 [details] Link to a branch that shows decorator usage hmmm, one thing that I've just discovered while using this is that when we split GaiaDevice, GaiaApps etc out into separate files we'll need this class in each of the files. Bob given this context, does it change the way you would implement it?

(In reply to Zac C (:zac) from comment #11) > Comment on attachment 8418041 [details] > Link to a branch that shows decorator usage > > hmmm, one thing that I've just discovered while using this is that when we > split GaiaDevice, GaiaApps etc out into separate files we'll need this class > in each of the files. > Bob given this context, does it change the way you would implement it? I still think the implementation would be similar, but we'd have to decide how to package it. For example, we could have the decorator in a file all by itself (e.g., `needs_system_frame_decorator.py`) and then simply import it into any of the other files that need it (i.e., `from .need_system_frame_decorator import needs_system_frame).

Thanks Bob, I'm still having some problems - can you have a look at the Travis job to see the error?

:zac I had to change some things. Please see the PR for comments and my suggested changes.

Not really keen on importing another package just for this. Rob if you need to switch to frame in some methods let's just do it on a case-by-case basis.

We could run into problems when we split these files out so will make this depending on the gaiatest packaging.

As per comment #16, let's do this in case-by-case basis. In this case running without --restart, using set_permission at the start of a test will run it in the context of the homescreen instead of the system frame.

Comment on attachment 8447090 [details] github pr r+, waiting for Travis before merging.

Comment on attachment 8447090 [details] github pr It seems there where some issue in the TBPL run with the lockscreen tests. This requires further investigation.

(In reply to Robert Chira [:RobertC] from comment #20) > Comment on attachment 8447090 [details] > github pr > > It seems there where some issue in the TBPL run with the lockscreen tests. > This requires further investigation. I think there were some problems in the tree at the time. Re-triggers of the test show it coming up roses (green :P )