Review if TabManager.getBrowserIdForBrowsingContext is needed
Categories
(Remote Protocol :: Agent, task, P3)
Tracking
(firefox101 fixed)
| Tracking | Status | |
|---|---|---|
| firefox101 | --- | fixed |
People
(Reporter: jdescottes, Assigned: Sasha)
Details
(Whiteboard: [bidi-m3-mvp])
Attachments
(1 file)
See https://phabricator.services.mozilla.com/D141944#inline-782837
We have several methods to retrieve a TabManager id for a browsing context:
- getIdForBrowser
- getBrowserIdForBrowsingContext
- getIdForBrowsingContext
getIdForBrowsingContext is the most useful one. For a given BrowsingContext it will always return the TabManager unique id that should be used to represent this context. Meaning :
- for a toplevel BrowsingContext, return a custom unique id tied to the Browser owning the BrowsingContext
- for a nested BrowsingContext, return the BrowsingContext id
getBrowserIdForBrowsingContext is only used in tests, and is very specific. It will get the toplevel BrowsingContext to a provided context and will return the unique id for it. It's only used in tests, and having it available is probably more confusing than anything else.
If needed, it can be emulated easily with getIdForBrowsingContext(context.top).
On a sidenote, I'm also on the fence about getIdForBrowser. While it is useful internally, I am not sure expose it as a public API is good. External callers could replace it with getIdForBrowsingContext(browser.browsingContext). That would be slightly less efficient than directly calling getIdForBrowser, but having a single API to build IDs would be nice.
|
||
Updated•2 years ago
|
|
Assignee | |
Updated•2 years ago
|
|
|
Assignee | |
Comment 1•2 years ago
|
||
To summarize the scope
- remove
getBrowserIdForBrowsingContext(and it's usage) - mark
getIdForBrowseras internal (_getIdForBrowser) - replace usage of
getIdForBrowseroutside of https://searchfox.org/mozilla-central/source/remote/shared/TabManager.jsm withgetIdForBrowsingContext(browser.browsingContext)
What do you think, Julian? Have I covered everything?
|
Reporter | |
Comment 2•2 years ago
|
||
(In reply to Alexandra Borovova from comment #1)
To summarize the scope
- remove
getBrowserIdForBrowsingContext(and it's usage)- mark
getIdForBrowseras internal (_getIdForBrowser)- replace usage of
getIdForBrowseroutside of https://searchfox.org/mozilla-central/source/remote/shared/TabManager.jsm withgetIdForBrowsingContext(browser.browsingContext)What do you think, Julian? Have I covered everything?
That sounds good to me. I can see 6 call sites for getIdForBrowser outside of TabManager.jsm, but having a single API feels worth the update.
|
|
Assignee | |
Comment 3•2 years ago
|
||
After some investigation it appeared that we can not use getIdForBrowsingContext when we deal with the browser element of a tab, because we can't guarantee that browser context is attached to it, therefore we have to keep getIdForBrowser for such cases. So the scope of the bug narrows to the removal of getBrowserIdForBrowsingContext
|
|
Assignee | |
Comment 4•2 years ago
|
||
Pushed by aborovova@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f34f141d3b60 Remove unneeded getBrowserIdForBrowsingContext. r=jdescottes,webdriver-reviewers
|
||
Comment 6•2 years ago
|
||
| bugherder | ||
Description
•