Closed Bug 1318478 Opened 8 years ago Closed 7 years ago

Remove get_permission / push_permission

Categories

(Remote Protocol :: Marionette, defect, P5)

Version 3
defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1368674

People

(Reporter: whimboo, Unassigned)

References

Details

We got both methods implemented on bug 1149618 for b2g only. So we shouldn't need those anymore. There are only 2 unit tests which make use of it but nothing else. Jonathan, maybe you could have a look at the following and tell me (if you still can remember) that those are b2g only? I have to ask because the manifest file doesn't really mention that. https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/unit/test_switch_remote_frame.py https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/unit/test_getactiveframe_oop.py There is no other code in the tree which actually uses those two methods. So my proposal would be to remove those two methods and tests.
Flags: needinfo?(jgriffin)
Blocks: 1306604
Those were indeed written just for FFOS. Since then, however, it seems like support might have been added to desktop in bug 1238160. It's likely no one is writing Marionette tests for those things now, however they could in the future. For future flexibility, we may want to keep these methods around.
Flags: needinfo?(jgriffin)
Ah, sorry, I thought you were asking about the frame switching stuff, didn't read the description closely enough. Yes, get_permission and push_permission were added for managing WebAPI's in Gaia. If no one is using those, I have no objection to removing that; it's easy enough to re-introduce later if we end up needing it in the future.
(In reply to Jonathan Griffin (:jgriffin) from comment #1) > Those were indeed written just for FFOS. Since then, however, it seems like > support might have been added to desktop in bug 1238160. It's likely no one > is writing Marionette tests for those things now, however they could in the > future. For future flexibility, we may want to keep these methods around. Thanks Jonathan for the pointer. Given that J. Ryan was working on this other bug it might make sense to get his feedback about what could be removed (API, and test wise).
Flags: needinfo?(jryans)
My impression from bug 1318218 and others under bug 1306391 is that permission handling is being removed, but Ehsan would likely know for sure. For my use case of mozbrowser frames on desktop, permission handling is not needed because that are used on chrome:// pages that automatically get full permission.
Flags: needinfo?(jryans)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4) > My impression from bug 1318218 and others under bug 1306391 is that > permission handling is being removed, but Ehsan would likely know for sure. Ehsan, mind helping me to figure this out? Thanks!
Flags: needinfo?(ehsan)
Permissions themselves are alive and well, but the b2g model of giving apps permissions has been removed. To clarify, since permission is such an overloaded word, we use nsIPermissionManager to store permissions for arbitrary principals. We used to have app-specific permissions for b2g which are now removed. Those are essentially the permisisons in this giant list: <https://hg.mozilla.org/mozilla-central/rev/592927e727fd#l20.40> Looking at <https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#878>, that code just uses the permission manager, and looking at the consumers it seems like that function only set the "browser" permission which is on the list of removed permissions. Therefore I think it's OK to remove these functions.
Flags: needinfo?(ehsan)
Thanks Ehsan! That opens another question for me. In Firefox Puppeteer (a mixin with ui/api helpers for Marionette) we currently make use of Permissions to enable/disable different features for specific hosts: https://dxr.mozilla.org/mozilla-central/source/testing/puppeteer/firefox/firefox_puppeteer/api/utils.py#105 Now I wonder if it would make sense to remove any app related code from the Marionette permission methods and only leave the feature to add, remove, and get site permissions like install, geolocation and such. By a glance over the Marionette code it looks like that it is doing something similar. Am I on the right track with that idea?
Flags: needinfo?(ehsan)
Yes, but please check the individual permissions and only remove the ones that weren't in PermissionsTable.jsm.
Flags: needinfo?(ehsan)
Priority: -- → P5
That was done by David on bug 1368674.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.