Closed Bug 1766240 Opened 11 months ago Closed 6 months ago

Implement basic "script.getRealms" command that supports WindowRealmInfo

Categories

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

task
Points:
5

Tracking

(firefox106 fixed)

RESOLVED FIXED
106 Branch
Tracking Status
firefox106 --- fixed

People

(Reporter: whimboo, Assigned: Sasha)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [webdriver:m4] , [wptsync upstream], [webdriver:relnote] )

Attachments

(4 files)

No description provided.
Points: --- → 13
Priority: -- → P3

If time permits for M4 I would suggest that we also keep this bug at least in the P2 queue for now. Some methods work with realm info or return it so having a proper implementation would be great. Lets discuss in next week's triage meeting.

Whiteboard: [webdriver:triage]
Points: 13 → 8
Priority: P3 → P2
Whiteboard: [webdriver:triage] → [webdriver:backlog]
Depends on: 1770480
Blocks: 1779231

Once bug 1785092 has been fixed this bug should be quite easy to get implemented.

Depends on: 1785092
Assignee: nobody → aborovova
Status: NEW → ASSIGNED
Whiteboard: [webdriver:backlog] → [webdriver:m4]
Priority: P2 → P1
Points: 8 → 5
Attachment #9292395 - Attachment description: WIP: Bug 1766240 - [bidi] Implement "script.getRealms" command. → Bug 1766240 - [bidi] Implement "script.getRealms" command.
Attachment #9292396 - Attachment description: WIP: Bug 1766240 - [wdspec] Add support for "script.getRealms" in wdspec tests. → Bug 1766240 - [wdspec] Add support for "script.getRealms" in wdspec tests.
Attachment #9292397 - Attachment description: WIP: Bug 1766240 - [wdspec] Add tests for "script.getRealms" command. → Bug 1766240 - [wdspec] Add tests for "script.getRealms" command.

While implementing this command, I first went with the approach of forwarding the command to a child process and returning the information of the realms, cached there. For the case when browsing context is not provided, it would mean getting all the browsing contexts and forwarding commands to each one of them.
Then I had a look at bug 1779231. Here we would need to get browsing context for a specified realmId that means that most likely we would need to have in the parent process a map between at least realmId and contextId, but we could of course store also all other required realm information. That could mean that when getRealms is called, we can just return this map. The only problem is, that we would not have realms stored which were not used for script evaluation. (We also could have already destroyed realms)
The next thought would be that ideally we should listen to script.realmCreated and script.realmDestroyed events to build realm info map. Then it can be safely used to return it when getRealms is called or if evaluate or callFunction is called with realmId and we have to find an associated browsing context.
So it looks like it would be better to block this bug at least on bug 1788657.

Depends on: 1788657
No longer depends on: 1788657
Whiteboard: [webdriver:m4] → [webdriver:backlog:blocked]

After the discussion, it was discovered that building the cache might require additional changes to keep it up-to-date, so it was decided to proceed with the approach of always retrieving all the realms from all browsing contexts (if a browsing context id is not provided) and build the cache later in the scope of bug 1788894.

Sasha, I just noticed the following PR from James on GitHub which includes the sandbox name into the WindowRealm info. Now I wonder if we should wait until it's merged? Otherwise we can also do a follow-up right after this patch series got landed. What's your current plan in handling that?

Flags: needinfo?(aborovova)

I would say since this bug blocks the target realm bug, which blocks finishing up the milestone, it would be better to not block it on the spec change and follow up, when the spec actually gets updated.

Flags: needinfo?(aborovova)

(In reply to Alexandra Borovova [:Sasha] from comment #9)

I would say since this bug blocks the target realm bug, which blocks finishing up the milestone, it would be better to not block it on the spec change and follow up, when the spec actually gets updated.

That sounds good! Thanks.

This bug is required to enable the script module by default. As such moving back into M4.

Whiteboard: [webdriver:backlog:blocked] → [webdriver:m4]
Attachment #9293303 - Attachment description: Bug 1766240 - [wdspec] Add a version of bidi commands that return the raw result. → Bug 1766240 - [wdspec] Add a special parameter to bidi commands to return the raw result.
Pushed by aborovova@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ffe78a5df18b
[bidi] Implement "script.getRealms" command. r=webdriver-reviewers,jdescottes,whimboo
https://hg.mozilla.org/integration/autoland/rev/dd1a5096f528
[wdspec] Add support for "script.getRealms" in wdspec tests. r=webdriver-reviewers,jgraham,whimboo
https://hg.mozilla.org/integration/autoland/rev/265621d051cb
[wdspec] Add a special parameter to bidi commands to return the raw result. r=webdriver-reviewers,jgraham,whimboo
https://hg.mozilla.org/integration/autoland/rev/c95d82c6f844
[wdspec] Add tests for "script.getRealms" command. r=webdriver-reviewers,whimboo,jdescottes
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/35866 for changes under testing/web-platform/tests
Whiteboard: [webdriver:m4] → [webdriver:m4] , [wptsync upstream]

The patches on this bug implemented the basics for this command by only supporting the type WindowRealmInfo for now.

Summary: Implement "script.getRealms" command → Implement basic "script.getRealms" command that supports WindowRealmInfo
Upstream PR merged by jgraham

Adding webdriver:relnote

Added basic support for the getRealms command of the script module, currently limited to the WindowRealmInfo type which includes window realms and sandbox realms.

Whiteboard: [webdriver:m4] , [wptsync upstream] → [webdriver:m4] , [wptsync upstream], [webdriver:relnote]
You need to log in before you can comment on or make changes to this bug.