Closed
Bug 1423358
Opened 7 years ago
Closed 6 years ago
Disassociate frameRegsPending from curBrowser
Categories
(Remote Protocol :: Marionette, enhancement)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: ato, Assigned: ato)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
Details |
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.
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
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.
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.
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.
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.
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.
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.
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
Attachment #8934711 -
Flags: review?(hskupin)
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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•