Andreas Tolfsen ❲:ato❳ Assignee
	Description • 7 years ago

In preparation for the window tracking refactoring we will want to
disassociate the frameRegsPending state from curBrowser (browser.Context).
It is a simple counter that could just as well live as state on the
GeckoDriver class.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 2 • 7 years ago
mozreview-review

Comment on attachment 8934711 [details]
Bug 1423358 - Disassociate frameRegsPending from curBrowser.

https://reviewboard.mozilla.org/r/205612/#review211502

::: commit-message-18a9c:7
(Diff revision 1)
> +
> +This moves the frameRegsPending state from browser.Context (curBrowser)
> +to GeckoDriver.  This probably isn't the ideal place for it either,
> +but will make it easier for the window tracking refactoring to
> +remove browser.Context in its entirety.
> +

Just to make sure, we have only one single instance of browser.Context running at any time? If not we won't be able to hold that state per browser context.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 3 • 7 years ago
mozreview-review-reply

Comment on attachment 8934711 [details]
Bug 1423358 - Disassociate frameRegsPending from curBrowser.

https://reviewboard.mozilla.org/r/205612/#review211502

> Just to make sure, we have only one single instance of browser.Context running at any time? If not we won't be able to hold that state per browser context.

Well no, multiple browser.Contexts are stored in GeckoDriver#browsers,
but we currently only support registering one frame at a time due
to the way GeckoDriver#registerPromise works.

This whole thing will go away with the window tracking refactor,
and for as long as it doesn’t cause any tests to regress I’m happy
to bleed a little shared state in GeckoDriver.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 4 • 7 years ago
mozreview-review-reply

Comment on attachment 8934711 [details]
Bug 1423358 - Disassociate frameRegsPending from curBrowser.

https://reviewboard.mozilla.org/r/205612/#review211502

> Well no, multiple browser.Contexts are stored in GeckoDriver#browsers,
> but we currently only support registering one frame at a time due
> to the way GeckoDriver#registerPromise works.
> 
> This whole thing will go away with the window tracking refactor,
> and for as long as it doesn’t cause any tests to regress I’m happy
> to bleed a little shared state in GeckoDriver.

Ok, please add a comment in the code next to the definition of the property.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8934711 [details]
Bug 1423358 - Disassociate frameRegsPending from curBrowser.

https://reviewboard.mozilla.org/r/205612/#review211520

::: testing/marionette/driver.js:141
(Diff revision 1)
>    this.mainFrame = null;
>    // chrome iframe that currently has focus
>    this.curFrame = null;
>    this.mozBrowserClose = null;
>    this.currentFrameElement = null;
> +  this.frameRegsPending = 0;

Shouldn't we reset this property now when the curBrowser gets switched? Otherwise state of the old context would remain.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 6 • 7 years ago
mozreview-review-reply

Comment on attachment 8934711 [details]
Bug 1423358 - Disassociate frameRegsPending from curBrowser.

https://reviewboard.mozilla.org/r/205612/#review211520

> Shouldn't we reset this property now when the curBrowser gets switched? Otherwise state of the old context would remain.

frameRegsPending gets reset in https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/testing/marionette/driver.js#507.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 7 • 7 years ago
mozreview-review-reply

Comment on attachment 8934711 [details]
Bug 1423358 - Disassociate frameRegsPending from curBrowser.

https://reviewboard.mozilla.org/r/205612/#review211520

> frameRegsPending gets reset in https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/testing/marionette/driver.js#507.

But it is behind the check `(mm.childCount !== 0)`. It means if that count is 0 we will keep the value from the former browser context.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 8 • 6 years ago
mozreview-review

Comment on attachment 8934711 [details]
Bug 1423358 - Disassociate frameRegsPending from curBrowser.

https://reviewboard.mozilla.org/r/205612/#review213376

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 9 • 6 years ago

(In reply to Henrik Skupin (:whimboo) [mostly away Dec 11th - Jan
3rd] from comment #7)

> > frameRegsPending gets reset in
> > https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b131669
> > 70e17809a3b5d6a555/testing/marionette/driver.js#507.
> 
> But it is behind the check `(mm.childCount !== 0)`. It means if
> that count is 0 we will keep the value from the former browser
> context.

Yes, but that code is buggy as you will see from the try run.

In any case, I guess due how apparently controversial
this change it, I will address it as part of
https://bugzil.la/marionette-window-tracking instead.