Closed Bug 1506523 Opened 1 year ago Closed 1 year ago

Adapt Marionette so it can run on Thunderbird

Categories

(Testing :: Marionette, enhancement, P3)

Version 3
enhancement

Tracking

(firefox65 fixed)

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(1 file, 2 obsolete files)

There's a few minor changes required before Thunderbird can run Marionette, and therefore Mochitest.
Priority: -- → P3
Attached patch 1506523-marionette-1.diff (obsolete) — Splinter Review
I *think* these are all the changes needed, but I can't rule out more later on. I'm only planning to use this for "browser-chrome" style tests, so I think I can get away with some thing just not working (for Thunderbird).
Attachment #9024364 - Flags: review?(hskupin)
Depends on: 1506706
Blocks: 1506715
Comment on attachment 9024364 [details] [diff] [review]
1506523-marionette-1.diff

Review of attachment 9024364 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my inline comments. Also I would like to see an additional review from Andreas because he currently works on refactoring the window handling.

::: testing/marionette/assert.js
@@ +101,5 @@
> + *
> + * @throws {UnsupportedOperationError}
> + *     If current browser is not Firefox or Thunderbird.
> + */
> +assert.firefoxOrThunderbird = function(msg = "") {

Maybe we should just call this `assert.desktop` which would include all desktop apps.

::: testing/marionette/components/marionette.js
@@ +408,5 @@
> +      // Thunderbird only, instead of sessionstore-windows-restored.
> +      case "mail-startup-done":
> +        this.finalUIStartup = true;
> +        this.init();
> +        break;

I don't know about the startup paths for Thunderbird. So it would be good to also get a review from a Thunderbird peer / module owner if it is fine to hook into that observer, or if we have to wait a bit longer.

::: testing/marionette/driver.js
@@ +562,5 @@
>  };
>  
>  GeckoDriver.prototype.listeningPromise = function() {
> +  if (this.appId == APP_ID_THUNDERBIRD) {
> +    return Promise.resolve();

Is this because Thunderbird doesn't use e10s? If yes, we shouldn't check by app but if the e10s mode is enabled or not.

@@ +722,5 @@
>    let registerBrowsers = this.registerPromise();
>    let browserListening = this.listeningPromise();
>  
>    let waitForWindow = function() {
> +    let windowType = (this.appId == APP_ID_THUNDERBIRD) ? "mail:3pane" : "navigator:browser";

Please change that to a switch statement.

@@ +767,5 @@
>    if (this.mainFrame) {
>      this.mainFrame.focus();
>    }
>  
> +  if (this.curBrowser.tab && this.curBrowser.contentBrowser) {

Why do you need this change?

@@ +3308,5 @@
>    return {cause: await quitApplication};
>  };
>  
>  GeckoDriver.prototype.installAddon = function(cmd) {
> +  assert.firefoxOrThunderbird();

I assume this is the only command you make use of for now. So I'm fine with just changing only this one.
Attachment #9024364 - Flags: review?(hskupin)
Attachment #9024364 - Flags: review?(ato)
Attachment #9024364 - Flags: review-
Comment on attachment 9024364 [details] [diff] [review]
1506523-marionette-1.diff

Review of attachment 9024364 [details] [diff] [review]:
-----------------------------------------------------------------

If you’re fine with this breaking at any point due to no upstream
testing of Thunderbird in mozilla-central, I’m happy for Marionette
to take on preliminary Thunderbird support.

::: testing/marionette/driver.js
@@ +767,5 @@
>    if (this.mainFrame) {
>      this.mainFrame.focus();
>    }
>  
> +  if (this.curBrowser.tab && this.curBrowser.contentBrowser) {

In this patch, I think this is the only part I’m worried about.
Attachment #9024364 - Flags: review?(ato) → review+
(In reply to Henrik Skupin (:whimboo) from comment #2)
> ::: testing/marionette/driver.js
> @@ +562,5 @@
> >  };
> >  
> >  GeckoDriver.prototype.listeningPromise = function() {
> > +  if (this.appId == APP_ID_THUNDERBIRD) {
> > +    return Promise.resolve();
> 
> Is this because Thunderbird doesn't use e10s? If yes, we shouldn't check by
> app but if the e10s mode is enabled or not.

(In reply to Andreas Tolfsen ❲:ato❳ from comment #3)
> ::: testing/marionette/driver.js
> @@ +767,5 @@
> >    if (this.mainFrame) {
> >      this.mainFrame.focus();
> >    }
> >  
> > +  if (this.curBrowser.tab && this.curBrowser.contentBrowser) {
> 
> In this patch, I think this is the only part I’m worried about.

I think I've managed to do away with both of these changes. Will post a new patch when I'm sure.
Attached patch 1506523-marionette-2.diff (obsolete) — Splinter Review
Attachment #9024364 - Attachment is obsolete: true
Attachment #9025795 - Flags: review?(hskupin)
Comment on attachment 9025795 [details] [diff] [review]
1506523-marionette-2.diff

Review of attachment 9025795 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the update! Those changes look fine. There is just a minor thing, which you can find as an inline comment.

::: testing/marionette/components/marionette.js
@@ +411,5 @@
> +      // Thunderbird only, instead of sessionstore-windows-restored.
> +      case "mail-startup-done":
> +        this.finalUIStartup = true;
> +        this.init();
> +        break;

Mind putting it right before `sessionstore-windows-restored`?
Attachment #9025795 - Flags: review?(hskupin) → review+
Done.
Attachment #9025795 - Attachment is obsolete: true
Attachment #9027058 - Flags: review+
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d8c6059114
Adapt Marionette so it can run on Thunderbird; r=whimboo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f9d8c6059114
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.