Skip to content

refactor(core): scope JS resources to the webview#9272

Merged
lucasfernog merged 14 commits intodevfrom
refactor/resources-scope
Apr 2, 2024
Merged

refactor(core): scope JS resources to the webview#9272
lucasfernog merged 14 commits intodevfrom
refactor/resources-scope

Conversation

@amrbashir
Copy link
Member

No description provided.

@amrbashir amrbashir requested a review from a team as a code owner March 26, 2024 00:41
@lucasfernog
Copy link
Member

lucasfernog commented Mar 27, 2024

I think it's easier if we added a state that is specific to a window/webview so each target has its own resource table.

Currently there's a StateManager in the manager, but we can easily include one in the window or webview too (WebviewWindow can just use the window one).

plus we should still use random resource IDs

@amrbashir
Copy link
Member Author

I think it's easier if we added a state that is specific to a window/webview so each target has its own resource table.

Currently there's a StateManager in the manager, but we can easily include one in the window or webview too (WebviewWindow can just use the window one).

yeah this sounds better, however I still would like to keep a global resources table so I guess I will just remove Manager::resources_table and add a resources_table method on Window/Webview/WebviewWindow/App/AppHandle types

plus we should still use random resource IDs

if the resources are scoped, we don't have to use random ids, is there any other reason to use random ids?

@lucasfernog
Copy link
Member

we can check with the auditors later but I feel it's safer with random IDs. cc @chippers @tweidinger

@amrbashir
Copy link
Member Author

amrbashir commented Mar 28, 2024

I did confirm with the auditors before opening the PR, they still need to re-test though

@tweidinger
Copy link
Contributor

if the resources are scoped, we don't have to use random ids, is there any other reason to use random ids?

Hardening in depth and reducing risk. I would strongly suggest to have it randomized as we don't know all the special use cases in finished apps.

@lucasfernog
Copy link
Member

doesn't cost much to randomize it (we should already have a dependency on getrandom somewhere)

@amrbashir
Copy link
Member Author

amrbashir commented Apr 1, 2024

we should already have a dependency on getrandom somewhere

totally forgot we already have a dependency on it, pushed the random changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants