-
Notifications
You must be signed in to change notification settings - Fork 170
Allow to share resources, expose more debug info #519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's implement this using traits instead; it will give us more flexibility to e.g. use an on-disk cache in brave-core and have better devtools diagnostics in the future.
More or less like this:
Dropping support for individual
add_resource
calls is ok with me.Engine
can keep the existinguse_resources
method to create anInMemoryResourceStorageBackend
with the provided resources, as well as a new methoduse_resources_backend
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The particular task I'm trying to solve it sharing the impl between the engines.
It's sounds like you suggest changing the interfaces in another place.
Right now we have only one
ResourceStorage
impl and no plans to support another. I have no idea what to put intoInMemoryResourceStorageBackend
. Can you elaborate on this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InMemoryResourceStorageBackend
is identical to the current implementation. What I propose with theResourceStorageBackend
trait is that we can use it to define a new brave-core specific implementation that supports sharing between engines and (eventually) on-disk caching. We can put the implementation of that in brave-core'sadblock
binding layer since we will need tighter integration with Chromium's FS operations; it doesn't need to live inadblock-rust
itself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current conception assumes that the browser should just make
ResourceStorage
and pass it to the both engines.I don't have such plans right now. Also, on-disk representation takes more memory because of base64: an extra work is needed to process it or change the backend code to storage. If you have a conception how to deal with it, it would be nice to see a PR.
I would say we first need to focus on 20MB+ flatbuffer caching rather that relatively small resources.
In conclusion, I don't want to make multiple
get_resource
.The only thing I'm generalizing here is getting the current
ResourceStorage
impl.If you really want it, we can do it via traits:
ResourceStorageGetter
with two implsSimpleResourceStorage
andSharableResourceStorageGetter
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I made a dedicated PR with the part with extra debug info: #527
At least we can merge this part and continue discussion about this.
I've also added the test to show how the resources can be shared from the b-c perspective.