Closed
Bug 1298025
Opened 8 years ago
Closed 8 years ago
Expose installing temporary extensions as an extension command in Marionette
Categories
(Remote Protocol :: Marionette, defect)
Tracking
(firefox52 fixed, firefox53 fixed)
RESOLVED
FIXED
mozilla53
People
(Reporter: standard8, Assigned: ato)
References
Details
Attachments
(1 file)
Marionette has a function whereby you can install a temporary add-on in a profile.
https://dxr.mozilla.org/mozilla-central/rev/01748a2b1a463f24efd9cd8abad9ccfd76b037b8/testing/marionette/client/marionette_driver/addons.py#41
This is very useful for testing unsigned add-ons in official release builds. Hence it would be good if we could make it available to selenium/geckodriver setups so that users don't have to duplicate the temporary install scripts manually (and a specific API is likely to be better for visibility of the possibility).
:ato mentioned on irc this could be done by making the function an extension command within Marionette.
Assignee | ||
Updated•8 years ago
|
OS: Unspecified → All
Hardware: Unspecified → All
Assignee | ||
Comment 1•8 years ago
|
||
Cc’ing ahal as he did the original implementation of the chrome scripts that currently live within addons.py.
ahal, do you think this is a reasonable thing to do? I suspect we can remove the internal implementation in addons.py as a result of this so that it can be shared across different Marionette clients.
Flags: needinfo?(ahalberstadt)
Comment 2•8 years ago
|
||
Just to make sure I'm on the same page, as I'm not sure what an "extension command" is.. the proposal here is to move the JS logic to the server (as some sort of extension?) and provide a built-in API for clients to call? That sounds like a good idea to me. I think it was just done with execute_script originally because it was easier and could be proven out first.
Flags: needinfo?(ahalberstadt)
Assignee | ||
Comment 3•8 years ago
|
||
I should’ve explained what an ‘extension command’ is more carefully. It is correct that it is just another command in the server, except that it will be exposed by geckodriver under a vendor prefixed namespace such as /session/{session id}/moz/addon. It is described in more detail in the WebDriver specification:
http://w3c.github.io/webdriver/webdriver-spec.html#protocol-extensions
In terms of implementation, it is no different to any other Marionette command.
Assignee | ||
Comment 4•8 years ago
|
||
My current proposal for API is:
POST /session/{session id}/moz/addon
body: {addon: <Base64 encoded addon file>, temporary: <boolean>}
return: {value: <addon id string>}
Marionette: Marionette:installAddon
DELETE /session/{session id}/moz/addon/{addon id}
body: {}
return: {}
Marionette: Marionette:uninstallAddon
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8785277 [details]
Bug 1298025 - Move addon installation internally to Marionette
https://reviewboard.mozilla.org/r/74546/#review72506
Attachment #8785277 -
Flags: review?(dburns) → review+
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8785277 [details]
Bug 1298025 - Move addon installation internally to Marionette
https://reviewboard.mozilla.org/r/74546/#review73542
Sorry for the delay. Meant to do this on Monday but forgot.
This looks good to me! I skimmed the driver.js changes though, as I'm not very familiar with that area.
::: testing/marionette/addon.js:79
(Diff revision 1)
> + }
> + aInstall.addListener(listener);
> + aInstall.install();
> + });
> + } else {
> + AddonManager.addAddonListener(listener);
Just an fyi.. The addAddonListener object here (only used by temporary addons) will only call "onInstalled". In other words, if installation of a temporary addon fails for some reason, this promise won't get resolved. I tried fairly hard to find a way to detect if installation failed, but I couldn't figure out how to do it.
I only mention it because now that this code lives server side, you might want higher standards. I assume the marionette call will eventually timeout, but you might want a smaller timeout here with a customized error message. Fwiw, I've never seen a temporary addon fail to install.
Attachment #8785277 -
Flags: review?(ahalberstadt) → review+
Comment 8•8 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #7)
> Just an fyi.. The addAddonListener object here (only used by temporary
> addons) will only call "onInstalled". In other words, if installation of a
> temporary addon fails for some reason, this promise won't get resolved. I
> tried fairly hard to find a way to detect if installation failed, but I
> couldn't figure out how to do it.
>
> I only mention it because now that this code lives server side, you might
> want higher standards. I assume the marionette call will eventually timeout,
> but you might want a smaller timeout here with a customized error message.
> Fwiw, I've never seen a temporary addon fail to install.
Dave, can you give an advice maybe?
Flags: needinfo?(dtownsend)
Updated•8 years ago
|
Flags: needinfo?(dtownsend) → needinfo?(rhelmer)
Comment 9•8 years ago
|
||
AddonManager.installTemporaryAddon() returns a promise, you shouldn't need to install a listener. The promise rejects if installation fails: http://searchfox.org/mozilla-central/rev/95b0ecf4b59a01e0524ca02f6c96ecaabe38f4d5/toolkit/mozapps/extensions/AddonManager.jsm#2313-2320
Flags: needinfo?(rhelmer)
Assignee | ||
Comment 10•8 years ago
|
||
I suppose it would be best to rewrite this using the AddonManager.installTemporaryAddon promise rhelmer refers to.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•8 years ago
|
||
After closer examination I think the current approach is better as we need to resolve the `addon.install` promise with the ID of the installed addon, which the install listener gives us. In the case of an error we need to convert it and it’s better to avoid code duplication by doing this once in the install listener.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•8 years ago
|
||
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/260e64c44531
Move addon installation internally to Marionette; r=ahal,automatedtester
Comment 16•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 18•8 years ago
|
||
It would be great to have this test-only change in the upcoming ESR. Requesting uplift for Aurora.
status-firefox52:
--- → affected
Whiteboard: [checkin-needed-aurora]
Comment 19•8 years ago
|
||
bugherder uplift |
Whiteboard: [checkin-needed-aurora]
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•