Closed Bug 1321278 Opened 8 years ago Closed 8 years ago

Return current profile directory in capabilities

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox51 wontfix, firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

0001-Bug-1321278-Rename-processId-capability-to-moz-proce.patch
10.02 KB, patch
Details | Diff | Splinter Review
0002-Bug-1321278-Return-profile-directory-in-capabilities.patch
3.31 KB, patch
Details | Diff | Splinter Review 
As requested in https://github.com/mozilla/geckodriver/issues/334#issuecomment-263845908, FirefoxDriver returns the current profile directory in its capabilities.  Since we already return the current PID, I’m sympathetic to also returning the profile directory.
Assignee: nobody → ato
Comment on attachment 8815735 [details]
Bug 1321278 - Rename processId capability to moz:processID;

https://reviewboard.mozilla.org/r/96564/#review96832

::: testing/marionette/client/marionette_driver/marionette.py:1268
(Diff revision 1)
>          body = {"capabilities": desired_capabilities, "sessionId": session_id}
>          resp = self._send_message("newSession", body)
>  
>          self.session_id = resp["sessionId"]
>          self.session = resp["value"] if self.protocol == 1 else resp["capabilities"]
> +        self.process_id = self.session.get("moz:processID", self.session.get("processId"))

This landed for Firefox 49 (bug 1281750) and can be removed with Firefox 55. Please add a comment here.

::: testing/marionette/driver.js:162
(Diff revision 1)
>      "raisesAccessibilityExceptions": false,
>      "rotatable": this.appName == "B2G",
>      "proxy": {},
>  
>      // proprietary extensions
>      "specificationLevel": 0,

We don't need the `moz:` prefix for the specificationLevel?
Attachment #8815735 - Flags: review?(hskupin) → review+
Comment on attachment 8815736 [details]
Bug 1321278 - Return profile directory in capabilities;

https://reviewboard.mozilla.org/r/96566/#review96838

::: testing/marionette/client/marionette_driver/marionette.py:577
(Diff revision 1)
>          self.bin = bin
>          self.instance = None
>          self.session = None
>          self.session_id = None
>          self.process_id = None
> +        self.profile = None

The profile is usually accessible via self.marionette.instance.mozrunner.profile when we have an instance. So maybe that can cause confusion. Otherwise in case of when Marionette connects to an already running instance, we would get the profile directory.

Maybe we should add a feature to mozrunner so it could be instantiated with necessary data like process id, profile path and such. With that we would always have an instance available. But that is clearly something for the future.

::: testing/marionette/driver.js:2938
(Diff revision 1)
> +function getCurrentDirectoryPath() {
> +  try {
> +    let profd = Cc["@mozilla.org/file/directory_service;1"]
> +        .getService(Ci.nsIProperties)
> +        .get("ProfD", Ci.nsIFile);
> +    return profd.path;

You can use `Services.dirsvc` here.

::: testing/marionette/harness/marionette/tests/unit/test_capabilities.py:48
(Diff revision 1)
>          self.assertEqual(self.marionette.process_id, self.appinfo["processID"])
>  
> +        current_profile = self.marionette.instance.runner.profile.profile
> +        self.assertIn("moz:profile", self.caps)
> +        self.assertEqual(self.caps["moz:profile"], current_profile)
> +        self.assertEqual(self.marionette.profile, current_profile)

The first assert has a reversed order of arguments. So flip those or fix the latter two asserts.
Attachment #8815736 - Flags: review?(hskupin) → review+
Attachment #8815735 - Flags: review?(mjzffr)
Attachment #8815735 - Flags: review?(dburns)
Attachment #8815736 - Flags: review?(mjzffr)
Attachment #8815736 - Flags: review?(dburns)
Comment on attachment 8815735 [details]
Bug 1321278 - Rename processId capability to moz:processID;

https://reviewboard.mozilla.org/r/96564/#review96832

> We don't need the `moz:` prefix for the specificationLevel?

We do, technically, but I hope to remove this soon.  It will be gone when we make the switch from using the isDisplayed Selenium atom to the WebDriver conformant interactability checks I added recently.  Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1321516 about this.
Comment on attachment 8815736 [details]
Bug 1321278 - Return profile directory in capabilities;

https://reviewboard.mozilla.org/r/96566/#review96838

> You can use `Services.dirsvc` here.

Nice, thanks for pointing that out!

> The first assert has a reversed order of arguments. So flip those or fix the latter two asserts.

With `assertIn` you need the first argument to be the value to look for in the second argument (the dict).
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da9707f081f9
Rename processId capability to moz:processID; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/df839937b415
Return profile directory in capabilities; r=whimboo
https://hg.mozilla.org/mozilla-central/rev/da9707f081f9
https://hg.mozilla.org/mozilla-central/rev/df839937b415
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Sheriffs: Test-only uplift, please, because Marionette is hidden behind flag.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
Needs rebasing.
Flags: needinfo?(ato)
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> Needs rebasing.

Maybe we missed to uplift another patch which landed lately?
I wish there was an easy way to tell.  I don’t think I care enough to do the legwork.
Flags: needinfo?(ato)
Bug 1103196 looks particularly relevant looking at the hg history.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> Bug 1103196 looks particularly relevant looking at the hg history.

Thanks Ryan!  It’s on my list to uplift that to Aurora, since it is our next ESR.
Sheriffs: Rebased patches to apply cleanly on top of Aurora.  Please uplift as test-only changes since Marionette is hidden behind a flag.
Whiteboard: [checkin-needed-aurora]
Too late for 51. Mark 51 won't fix.
status-firefox51: affected → wontfix
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: