941749 - getWindows should return tabs

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[James Lal [:lightsofapollo]]

I have a firefox profile with some extensions that is running with marionette enabled. When it starts it is in a tab running "system.gaiamobile.org" when I start the session I am now in about:blank and cannot find the tab with getWindows.

Comment 1

[Malini Das [:mdas] - Away, not checking bugmail]

geWindows should return all available tabs, and right now we just retrieve "navigator:browser" type windows, which doesn't return tabs.

Comment 2

[Malini Das [:mdas] - Away, not checking bugmail]

also related: Bug 941770 (stop creating the new tab)

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Attachment: Test case demonstrating the issue

Two test cases I came up with when I thought this was bug 1097221.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Basing the window handle commands on tabs rather than windows should be doable, but will require a pretty extensive re-work of marionette's internal mechanism for keeping track of things.

Comment 7

[James Graham [:jgraham]]

(It really helps to talk about top level browsing contexts here rather than "tabs". Otherwise it's hard to tell if you are going to implement the right thing).

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: bz://941749/chmanchester

Comment 9

[Chris Manchester (limited bugmail, email directly)]

/r/751 - Bug 941749 - A test managing open windows and tabs with marionette.
/r/753 - Bug 941749 - Add support for returning both known tabs and chrome windows from the marionette server.

Pull down these commits:

hg pull review -r 5921494d714ba584753e5f9020c8f78fa5777c6a

Comment 10

[Chris Manchester (limited bugmail, email directly)]

The commits above are a rough version of what we discussed in the m21s meeting yesterday. I'm interested in feedback on the api and implementation.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e6de410e3933

Comment 11

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/749/#review343

::: testing/marionette/client/marionette/tests/unit/test_window_handles.py
(Diff revision 1)
> +        self.marionette.navigate("about:blank")
> +        self.marionette.navigate("about:home")
> +        self.marionette.navigate("about:blank")

about:home loads external content, consider using data URI's with a <title> tag or something.

Comment 12

[Chris Manchester (limited bugmail, email directly)]

A b2g-oriented try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b6c236cca413

Comment 13

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/749/#review397

from a quick look this looks great. I would like to see how it fares on Try

::: testing/marionette/client/marionette/tests/unit/test_window_handles.py
(Diff revision 1)
> +    def test_tab_and_window_handles(self):

This scenario is the most important one really, thanks for writing a test!

Comment 14

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/749/#review403

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b6c236cca413
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e6de410e3933

Comment 15

[Chris Manchester (limited bugmail, email directly)]

/r/751 - Bug 941749 - A test managing open windows and tabs with marionette.
/r/753 - Bug 941749 - Add support for returning both known tabs and chrome windows from the marionette server.

Pull down these commits:

hg pull review -r 7b84c35142ecdc0ba3bb4aeffdbef1d86d14591b

Comment 16

[Chris Manchester (limited bugmail, email directly)]

I noticed from pushing my updated patch to try that wpt relies on the old behavior, so we'll need an update there before landing this.

Comment 17

[Chris Manchester (limited bugmail, email directly)]

This should fix the wpt issue (I'll submit a pr upstream when the changes are finalized and ready to land):

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ed769b37d4d5

Comment 18

[Chris Manchester (limited bugmail, email directly)]

(In reply to Chris Manchester [:chmanchester] from comment #17)
> This should fix the wpt issue (I'll submit a pr upstream when the changes
> are finalized and ready to land):
> 
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ed769b37d4d5

According to jgraham this intention of this assertion is that top level browsing contexts aren't being leaked between tests, but isn't quite doing that because of just this bug. So all we need here is a stopgap to keep things working between when this bug lands in tree and the upstream harness can be fixed to take the it in to account and enforce the original intent.

Comment 19

[Chris Manchester (limited bugmail, email directly)]

/r/751 - Bug 941749 - A test managing open windows and tabs with marionette.
/r/753 - Bug 941749 - Add support for returning both known tabs and chrome windows from the marionette server.

Pull down these commits:

hg pull review -r fa56c5604c21cd1b2236c697d769036dde27cd6f

Comment 20

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/749/#review537

::: testing/marionette/marionette-server.js
(Diff revision 3)
> +    if (appName == 'Firefox' && this.curBrowser.curFrameId) {

:whimboo noted on irc that this checks for appName == 'Firefox' here and appName != 'B2G' below. Not sure if one is prefereable to the other, but they should be consistent.

Comment 21

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/749/#review551

Ship It!

Comment 22

[Chris Manchester (limited bugmail, email directly)]

Opened https://github.com/w3c/wptrunner/pull/58 to prevent wpt bustage when this lands.

Comment 23

[Chris Manchester (limited bugmail, email directly)]

/r/751 - Bug 941749 - A test managing open windows and tabs with marionette.;r=automatedtester
/r/753 - Bug 941749 - Add support for returning both known tabs and chrome windows from the marionette server.;r=automatedtester

Pull down these commits:

hg pull review -r 4775706a1f7a4c345fa1481cfed453ccd8be160c

Comment 24

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8524791 [details]
MozReview Request: bz://941749/chmanchester

Didn't mean for mozreview to re-set that.

Comment 25

[Chris Manchester (limited bugmail, email directly)]

Attachment: Remove assertion in wpt invalidated by the fix.

Comment 26

[James Graham [:jgraham]]

Comment on attachment 8529186 [details] [review]
Remove assertion in wpt invalidated by the fix.

Forgot to say here that I merged the wptrunner fix upstream, so we just need to pull the latest version into the tree.

Comment 27

[Chris Manchester (limited bugmail, email directly)]

I just found pushing to try this fails a test on b2g, investigating now.

Comment 28

[Chris Manchester (limited bugmail, email directly)]

New try run with a fix:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9aff47efdace

Comment 29

[Chris Manchester (limited bugmail, email directly)]

/r/751 - Bug 941749 - A test managing open windows and tabs with marionette.;r=automatedtester
/r/753 - Bug 941749 - Add support for returning both known tabs and chrome windows from the marionette server.;r=automatedtester

Pull down these commits:

hg pull review -r 3d26a4a0d9ab5b72b02c62f0b8891204579be0c9

Comment 30

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/749/#review783

::: testing/marionette/marionette-server.js
(Diff revisions 4 - 5)
> +        winId += (appName == "B2G") ? "-b2g" : "";

The previous patch didn't add the '-b2g' suffix, so tests switching windows on b2g started failing.

Wasn't sure whether this required re-review, so decided to err on the side of caution.

Comment 31

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/749/#review829

Ship It!

Comment 32

[Chris Manchester (limited bugmail, email directly)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/957cfd26d6e1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9011ae0f6be8

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/957cfd26d6e1
https://hg.mozilla.org/mozilla-central/rev/9011ae0f6be8

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Chris, one thing I noticed here is the different implementation as shown below:

>   2.275    "getCurrentWindowHandle":  MarionetteServerConnection.prototype.getWindowHandle,  // Selenium 2 compat
>   2.276 +  "getChromeWindowHandle": MarionetteServerConnection.prototype.getChromeWindowHandle,

Was there a reason not to use getCurrentChromeWindowHandle together with .current_chrome_window_handle? Was it just the length which let you do the shorter property name? .chrome_window_handle doesn't really tell that it is the one for the currently selected chrome window.

Comment 35

[Chris Manchester (limited bugmail, email directly)]

(In reply to Henrik Skupin (:whimboo) from comment #34)
> Chris, one thing I noticed here is the different implementation as shown
> below:
> 
> >   2.275    "getCurrentWindowHandle":  MarionetteServerConnection.prototype.getWindowHandle,  // Selenium 2 compat
> >   2.276 +  "getChromeWindowHandle": MarionetteServerConnection.prototype.getChromeWindowHandle,
> 
> Was there a reason not to use getCurrentChromeWindowHandle together with
> .current_chrome_window_handle? Was it just the length which let you do the
> shorter property name? .chrome_window_handle doesn't really tell that it is
> the one for the currently selected chrome window.

Yes, for a reasonably short name. I think it's ok, but I wouldn't really have a problem with changing it.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Chris Manchester [:chmanchester] from comment #35)
> > >   2.275    "getCurrentWindowHandle":  MarionetteServerConnection.prototype.getWindowHandle,  // Selenium 2 compat
> > >   2.276 +  "getChromeWindowHandle": MarionetteServerConnection.prototype.getChromeWindowHandle,
> > 
> > Was there a reason not to use getCurrentChromeWindowHandle together with
> > .current_chrome_window_handle? Was it just the length which let you do the
> > shorter property name? .chrome_window_handle doesn't really tell that it is
> > the one for the currently selected chrome window.
> 
> Yes, for a reasonably short name. I think it's ok, but I wouldn't really
> have a problem with changing it.

David, what is your opinion here?

Comment 37

[David Burns :automatedtester]

No problem changing it, let's get this sorted ASAP

Comment 39

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 941749 - Add support for returning both known tabs and chrome windows from the marionette server.;r=automatedtester

Comment 40

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 941749 - A test managing open windows and tabs with marionette.;r=automatedtester