Closed Bug 1874918 Opened 1 year ago Closed 1 year ago

Support "userContext" parameter for "browsingContext.create" command

Categories

(Remote Protocol :: WebDriver BiDi, task, P2)

task
Points:
3

Tracking

(firefox124 fixed)

RESOLVED FIXED
124 Branch
Tracking Status
firefox124 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 2 open bugs, )

Details

(Whiteboard: [webdriver:m10][wptsync upstream][webdriver:relnote])

Attachments

(3 files)

The spec PR adding support for user contexts also adds a new optional parameter for browsingContext.create in order to create tabs in a specific user context.

Summary: Support userContext parameter for browsingContext.create → Support "userContext" parameter for "browsingContext.create" command

Hi Gijs,

I was looking at how to create a new window and make sure the first tab opens with a specific user context id.

At the moment we use browser.js OpenBrowserWindow which mostly forwards to BrowserWindowTracker.openWindow. The only way I could make this work was to build a window arguments array in order to set the 5th argument to the expected user context id.

While this works, this does not look great and seems hard to maintain. At the moment we are not passing any argument to OpenBrowserWindow except for a potential isPrivate flag. First we need to make sure we use correct "default" values for all the arguments we are not explicitly setting. We also rely on OpenBrowserWindow to pick the right default url, but this is bypassed if we pass an args parameter, meaning I would have to duplicate the logic to compute the default url. (but that should be easy to expose). But mainly I can see that there are many places building such window arguments arrays in the codebase, and I imagine it would be nice to avoid adding yet another one if possible.

I saw you filed Bug 1485961 in order to simplify window arguments handling, so I was wondering if you knew about a better way to achieve this?

Flags: needinfo?(gijskruitbosch+bugs)

Do you have an existing browser window when you're calling this? I assume so, in order to invoke OpenBrowserWindow. And do you normally load a URI (maybe hardcoded to about:blank if not provided) ?

If so, maybe URILoadingHelper.sys.mjs should provide a more ergonomic API, I think? Something like:

URILoadingHelper.openTrustedLinkIn(
  window,
  uriAsString,
  "window",
  { userContextId }
);

should work, I think?

Ideally that code shouldn't require a window, and then we could transition more consumers to it instead of everyone re-inventing this particular wheel - I thought I'd filed a bug for that but can't find it off-hand.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jdescottes)

Hm, I guess you answered the default URL bit. For webdriver-bidi, do you not just want to default to about:blank if nothing was passed?

Anyway, I guess if required, exposing the "determine the default URI" bit as a separate method somewhere (either BWT or URILoadingHelper) would be a good idea.

Thanks!

I am trying out openTrustedLinkIn. It seems to work fine for BiDi, but since this is also used by Marionette let's wait for try to see if there's any regression: https://treeherder.mozilla.org/jobs?repo=try&revision=c26496517219f0f6e1f7d88cbce38e5674e5b663

Since I need a reference to the created window, right now I use resolveOnContentBrowserCreated and then I get the ownerGlobal of the returned browser element.

Compared to our current implementation, I think that resolveOnContentBrowserCreated is only triggered when the window is fully ready so I think the additional events we are waiting for at https://searchfox.org/mozilla-central/rev/9d88be0557a5bc39d3da288f9aff71414baa303e/remote/shared/WindowManager.sys.mjs#205-212 have already been emitted. I'll push to try to see if this leads to regressions in Marionette or BiDi.

Regarding the URL, I agree it's weird for BiDi, especially since we enforce "about:blank" when creating tabs. It might be useful for non-BiDi consumers of this helper? As you said we can just expose the default URL bit if really needed.

If my try push looks fine I will file a separate bug to track this and block this bug.

Flags: needinfo?(jdescottes)
See Also: → 1875290
Depends on: 1875299
Flags: needinfo?(jdescottes)

User context support is required for M10.

Flags: needinfo?(jdescottes)
Whiteboard: [webdriver:m10]
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

FYI we currently use the userContext as both optional or null for its value. As far as I can see this is not needed here. As such I've created a pull request to get this fixed: https://github.com/w3c/webdriver-bidi/pull/650

Hi Olivia,

I'm having troubles to create geckoview tabs assigned to a certain container / user context id.

We currently use GeckoViewTabUtil.createNewTab(url) in order to create new tabs. This API only supports a URL string as parameter, so there is no way to set a user context id.

I could see that the userContextId is sometimes used as a cookieStoreId, and this seems exposed via GeckoViewTabBridge.createNewTab, but I think this GeckoViewTabBridge is only meant for webextensions? I tried using this but without success.

I'm not sure if containers/user contexts are fully supported on Android? If yes, do you have a suggestion to easily create a new tab assigned to a specific user context id?

Flags: needinfo?(ohall)
Blocks: 1877953

Moving the needinfo to the new bug dedicated to Android support

Flags: needinfo?(ohall)
Attachment #9377202 - Attachment description: Bug 1874918 - [wdspec] Add more tests for browsingContext.create with userContext → Bug 1874918 - [wdspec] Add invalid test for browsingContext.create with removed userContext
Points: --- → 3
Priority: -- → P2
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/649fd8134223 [bidi] Support userContext parameter for browsingContext.create r=webdriver-reviewers,Sasha,whimboo https://hg.mozilla.org/integration/autoland/rev/9958e50aa342 [wdspec] Add invalid test for browsingContext.create with removed userContext r=webdriver-reviewers,Sasha https://hg.mozilla.org/integration/autoland/rev/4be6b1980ac2 [wdspec] Add test for browser.removeUserContext to check browsing contexts are closed r=webdriver-reviewers,whimboo
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/44417 for changes under testing/web-platform/tests
Whiteboard: [webdriver:m10] → [webdriver:m10], [wptsync upstream]
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 124 Branch
Upstream PR merged by moz-wptsync-bot
Whiteboard: [webdriver:m10], [wptsync upstream] → [webdriver:m10][wptsync upstream][webdriver:relnote]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: