Add the new FileSystem domain for the DevTools Protocol. [chromium/src : main]
Etienne Noël (Gerrit)
Attention is currently required from: Etienne Noël.
Etienne Noël uploaded patch set #2 to this change.
Add the new FileSystem domain for the DevTools Protocol.
This CL adds the new FileSystem domain required for
the Origin Private File System in DevTools new feature.
Change-Id: I3e7fe86192df507b7cdf313e819f93a677b2388b
---
M headless/BUILD.gn
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/renderer/core/inspector/BUILD.gn
M third_party/blink/renderer/core/inspector/inspector_protocol_config.json
4 files changed, 71 insertions(+), 0 deletions(-)
To view, visit change 4860661. To unsubscribe, or for help writing mail filters, visit settings.
Etienne Noël (Gerrit)
Attention is currently required from: Andrey Kosyakov, Danil Somsikov.
Etienne Noël would like Danil Somsikov and Andrey Kosyakov to review this change.
Add the new FileSystem domain for the DevTools Protocol.
This CL adds the new FileSystem domain required for
the Origin Private File System in DevTools new feature.
Change-Id: I3e7fe86192df507b7cdf313e819f93a677b2388b
---
M headless/BUILD.gn
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/renderer/core/inspector/BUILD.gn
M third_party/blink/renderer/core/inspector/inspector_protocol_config.json
4 files changed, 71 insertions(+), 0 deletions(-)
To view, visit change 4860661. To unsubscribe, or for help writing mail filters, visit settings.
Andrey Kosyakov (Gerrit)
Attention is currently required from: Danil Somsikov, Etienne Noël.
5 comments:
Patchset:
No major issues with this, but let's see the implementation too!
File third_party/blink/public/devtools_protocol/browser_protocol.pdl:
Patch Set #2, Line 4228:
depends on RuntimeDoes it?
Patch Set #2, Line 4252:
string bucketIdAre both storageKey abd bucketId always required?
Patch Set #2, Line 4266:
array of string pathsIs this the same as `pathComponents` above?
File third_party/blink/renderer/core/inspector/inspector_protocol_config.json:
Please fix formatting
To view, visit change 4860661. To unsubscribe, or for help writing mail filters, visit settings.
Etienne Noël (Gerrit)
Attention is currently required from: Andrey Kosyakov, Danil Somsikov.
1 comment:
Patchset:
No major issues with this, but let's see the implementation too!
Perfect, the implementation will take a bit of time but will keep you posted. Thank you.
To view, visit change 4860661. To unsubscribe, or for help writing mail filters, visit settings.
Etienne Noël (Gerrit)
Attention is currently required from: Andrey Kosyakov, Danil Somsikov.
Etienne Noël uploaded patch set #3 to this change.
Add the new FileSystem domain for the DevTools Protocol.
This CL adds the new FileSystem domain required for
the Origin Private File System in DevTools new feature.
Change-Id: I3e7fe86192df507b7cdf313e819f93a677b2388b
---
M headless/BUILD.gn
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/renderer/core/inspector/BUILD.gn
M third_party/blink/renderer/core/inspector/inspector_protocol_config.json
M third_party/blink/renderer/modules/file_system_access/BUILD.gn
M third_party/blink/renderer/modules/file_system_access/DEPS
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.h
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h
M third_party/blink/renderer/modules/modules_initializer.cc
11 files changed, 860 insertions(+), 3 deletions(-)
To view, visit change 4860661. To unsubscribe, or for help writing mail filters, visit settings.
Etienne Noël (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ayu Ishii, Danil Somsikov.
Etienne Noël would like Ayu Ishii to review this change.
Add the new FileSystem domain for the DevTools Protocol.
This CL adds the new FileSystem domain required for
the Origin Private File System in DevTools new feature.
Change-Id: I3e7fe86192df507b7cdf313e819f93a677b2388b
---
M headless/BUILD.gn
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/renderer/core/inspector/BUILD.gn
M third_party/blink/renderer/core/inspector/inspector_protocol_config.json
M third_party/blink/renderer/modules/file_system_access/BUILD.gn
M third_party/blink/renderer/modules/file_system_access/DEPS
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.h
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h
M third_party/blink/renderer/modules/modules_initializer.cc
11 files changed, 860 insertions(+), 3 deletions(-)
To view, visit change 4860661. To unsubscribe, or for help writing mail filters, visit settings.
Etienne Noël (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ayu Ishii, Danil Somsikov.
4 comments:
Patchset:
I've fixed the comments, but this is still WIP. I've decided to reduce the number of methods in the `browser_protocol.pdl` for now and will iterate over time after this one is merged or else this CL will become monstrous.
I have implemented the Agent with the `getDirectory` method and have handled the errors.
I have tested locally but I am not ready to commit or open a CL on the JS side yet.
Questions for reviewers:
- I'm managing one instance of a class manually since I'm iterating over an AsyncPromiseIterator, it needs to survive. I haven't found a better way to do this, but maybe you will
- Are agent classes tested somewhere other than in the JS code?
- I am using crdtp::DispatchResponse to pass along success or errors but I'm getting a warning. Looking at the intent of the classes, I still think that coupling it to this class makes a lot of sense since in the end, that is what we will be returning and this Agent class will only be used for DevTools.
File third_party/blink/public/devtools_protocol/browser_protocol.pdl:
Patch Set #2, Line 4228:
depends on RuntimeDoes it?
Done
Patch Set #2, Line 4252:
string bucketIdAre both storageKey abd bucketId always required?
Yes.
File third_party/blink/renderer/core/inspector/inspector_protocol_config.json:
Please fix formatting
Done
To view, visit change 4860661. To unsubscribe, or for help writing mail filters, visit settings.
Etienne Noël (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ayu Ishii, Danil Somsikov.
1 comment:
File third_party/blink/public/devtools_protocol/browser_protocol.pdl:
Patch Set #2, Line 4266:
array of string paths
Is this the same as `pathComponents` above?
Done
To view, visit change 4860661. To unsubscribe, or for help writing mail filters, visit settings.
Etienne Noël (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ayu Ishii, Danil Somsikov.
Etienne Noël uploaded patch set #4 to this change.
Add the new FileSystem domain for the DevTools Protocol.
This CL adds the new FileSystem domain required for
the Origin Private File System in DevTools new feature.
Change-Id: I3e7fe86192df507b7cdf313e819f93a677b2388b
---
M headless/BUILD.gn
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/renderer/core/inspector/BUILD.gn
M third_party/blink/renderer/core/inspector/inspector_protocol_config.json
M third_party/blink/renderer/modules/file_system_access/BUILD.gn
M third_party/blink/renderer/modules/file_system_access/DEPS
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.h
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h
M third_party/blink/renderer/modules/modules_initializer.cc
11 files changed, 860 insertions(+), 3 deletions(-)
To view, visit change 4860661. To unsubscribe, or for help writing mail filters, visit settings.
Etienne Noël (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ayu Ishii, Danil Somsikov, Etienne Noël.
Etienne Noël uploaded patch set #5 to this change.
Add the new FileSystem domain for the DevTools Protocol.
This CL adds the new FileSystem domain required for
the Origin Private File System in DevTools new feature.
Change-Id: I3e7fe86192df507b7cdf313e819f93a677b2388b
---
M headless/BUILD.gn
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/renderer/core/inspector/BUILD.gn
M third_party/blink/renderer/core/inspector/inspector_protocol_config.json
M third_party/blink/renderer/modules/file_system_access/BUILD.gn
M third_party/blink/renderer/modules/file_system_access/DEPS
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.h
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h
M third_party/blink/renderer/modules/modules_initializer.cc
11 files changed, 861 insertions(+), 3 deletions(-)
To view, visit change 4860661. To unsubscribe, or for help writing mail filters, visit settings.
Ayu Ishii (Gerrit)
Attention is currently required from: Andrey Kosyakov, Danil Somsikov, Etienne Noël.
5 comments:
Patchset:
Hi! Sorry for the delay, I see what you mean now about dealing with promises. I added a super high level question on if we can avoid dealing with promises in the file builder code.
Commit Message:
Link to crbug & design doc?
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc:
Patch Set #5, Line 87:
promiseLooks like having to deal with these promises are creating a lot of complexity. Why not connect directly connect to the mojo remote so you don't have to deal with the promises, similar to the cache storage agent?[1]
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h:
Patch Set #3, Line 95:
BucketFileSystemBuilderClass comment on what it does
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.cc:
Patch Set #5, Line 160:
ScriptPromiseNote to self: do these all need to be wrapped in promises?
To view, visit change 4860661. To unsubscribe, or for help writing mail filters, visit settings.
Andrey Kosyakov (Gerrit)
Attention is currently required from: Ayu Ishii, Danil Somsikov, Etienne Noël.
1 comment:
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc:
Patch Set #5, Line 87:
promise
Looks like having to deal with these promises are creating a lot of complexity. […]
My understanding is that this is goes through a thin proxy and ends up being a mojo request to the browser? Why do we implement the agent in the renderer in the first place? Is there any renderer-specific state that it uses? Can we do it in the browser right away? See content/browser/devtools/protocol/storage_handler.cc for an example.
To view, visit change 4860661. To unsubscribe, or for help writing mail filters, visit settings.
Etienne Noël (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ayu Ishii, Danil Somsikov.
1 comment:
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc:
Patch Set #5, Line 87:
promise
My understanding is that this is goes through a thin proxy and ends up being a mojo request to the b […]
The main issue I would have with doing it in the Browser is that a lot of logic (security and business logic) would have to be reimplemented. There were a lot of discussions with security regarding what can be passed through a mojo call and they didn't want a full path to be passable for example. I have put together a new CL that avoids using promises and directly uses the mojo endpoints. It is obviously still complicated but I think it's a step in the right direction (https://chromium-review.googlesource.com/c/chromium/src/+/4970306)
It's still in draft so that's why I haven't tagged anyone other than Ayu yet, but feel free to take a look nonetheless.
To view, visit change 4860661. To unsubscribe, or for help writing mail filters, visit settings.
Etienne Noël (Gerrit)
Attention is currently required from: Etienne Noël.
Etienne Noël uploaded patch set #2 to this change.
Add the new FileSystem domain for the DevTools Protocol.
This CL adds the new FileSystem domain required for
the Origin Private File System in DevTools new feature.
Design doc: https://docs.google.com/document/d/1NLIVz0viaXbX_OrbmjY42ph1IcZmLJfkTBYlma0ZuLk/edit
Bug:1258806
Change-Id: Ia4a512e4c1d0b1cb44be785598df082b9aef5865
---
M headless/BUILD.gn
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/renderer/core/inspector/BUILD.gn
M third_party/blink/renderer/core/inspector/inspector_protocol_config.json
M third_party/blink/renderer/modules/buckets/storage_bucket.cc
M third_party/blink/renderer/modules/buckets/storage_bucket.h
M third_party/blink/renderer/modules/file_system_access/BUILD.gn
M third_party/blink/renderer/modules/file_system_access/DEPS
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.h
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h
M third_party/blink/renderer/modules/file_system_access/storage_manager_file_system_access.cc
M third_party/blink/renderer/modules/file_system_access/storage_manager_file_system_access.h
M third_party/blink/renderer/modules/modules_initializer.cc
15 files changed, 778 insertions(+), 28 deletions(-)
To view, visit change 4970306. To unsubscribe, or for help writing mail filters, visit settings.
Etienne Noël (Gerrit)
Etienne Noël uploaded patch set #3 to this change.
Add the new FileSystem domain for the DevTools Protocol.
This CL adds the new FileSystem domain required for
the Origin Private File System in DevTools new feature.
Design doc: https://docs.google.com/document/d/1NLIVz0viaXbX_OrbmjY42ph1IcZmLJfkTBYlma0ZuLk/edit
Bug:1258806
Change-Id: Ia4a512e4c1d0b1cb44be785598df082b9aef5865
---
M headless/BUILD.gn
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/renderer/core/inspector/BUILD.gn
M third_party/blink/renderer/core/inspector/inspector_protocol_config.json
M third_party/blink/renderer/modules/buckets/storage_bucket.cc
M third_party/blink/renderer/modules/buckets/storage_bucket.h
M third_party/blink/renderer/modules/file_system_access/BUILD.gn
M third_party/blink/renderer/modules/file_system_access/DEPS
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.h
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h
M third_party/blink/renderer/modules/file_system_access/storage_manager_file_system_access.cc
M third_party/blink/renderer/modules/file_system_access/storage_manager_file_system_access.h
M third_party/blink/renderer/modules/modules_initializer.cc
15 files changed, 829 insertions(+), 28 deletions(-)
To view, visit change 4970306. To unsubscribe, or for help writing mail filters, visit settings.
Etienne Noël (Gerrit)
Etienne Noël uploaded patch set #4 to this change.
Add the new FileSystem domain for the DevTools Protocol.
This CL adds the new FileSystem domain required for
the Origin Private File System in DevTools new feature.
Design doc: https://docs.google.com/document/d/1NLIVz0viaXbX_OrbmjY42ph1IcZmLJfkTBYlma0ZuLk/edit
Bug:1258806
Change-Id: Ia4a512e4c1d0b1cb44be785598df082b9aef5865
---
M headless/BUILD.gn
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/renderer/core/inspector/BUILD.gn
M third_party/blink/renderer/core/inspector/inspector_protocol_config.json
M third_party/blink/renderer/modules/buckets/storage_bucket.cc
M third_party/blink/renderer/modules/buckets/storage_bucket.h
M third_party/blink/renderer/modules/file_system_access/BUILD.gn
M third_party/blink/renderer/modules/file_system_access/DEPS
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.h
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h
M third_party/blink/renderer/modules/file_system_access/storage_manager_file_system_access.cc
M third_party/blink/renderer/modules/file_system_access/storage_manager_file_system_access.h
M third_party/blink/renderer/modules/modules_initializer.cc
15 files changed, 831 insertions(+), 28 deletions(-)
To view, visit change 4970306. To unsubscribe, or for help writing mail filters, visit settings.
Ayu Ishii (Gerrit)
Attention is currently required from: Etienne Noël.
18 comments:
Patchset:
Sorry for the delay, here are some comments from a partial review.
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.h:
// This method implements the protocol::FileSystem::Metainfo which directly
// receives and returns the response to DevTools. The response to the promise
// is provided through the callback parameter.
// It takes in a BucketFileSystemLocator which specifies a storage key, a
// storage bucket and a path component array (it can receive a nested path (in
// the case of a refresh action)). It's an array where each element is a
// nested directory level.
// This method gets the Root handle of the OPFS (for a
// specific storage key and bucket), finds the specified path by iterating
// from the root and then recursively iterates inside the specified directory
// to retrieve its nested files and directories.I think some of specific implementation details can be removed. Generally what I'm looking for here are things that simply answer, "what does it do?" "when should I call this?" type details. Especially since this `getDirectory` method (while it has the same name) works differently from the actual API itself. What do you think about something like...
```
This method is called by DevTools to find the specified path within the OPFS
given a `BucketFileSystemLocator`, and retrieve all nested files and directories
to build the complete directory tree, which is then returned to the callback.
```
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc:
Patch Set #3, Line 71:
GetRootDirectory`GetRootDirectoryHandle`
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc:
If there are still path_components in the array, it means we haven't yet
// reached the requested directory and must continue to recursively retrieve
// the handle for it.It does stink to have to go the browser process every time just to ignore irrelevant paths. Have we considered creating a special mojo method just for DevTools so we don't have to do this?
Patch Set #4, Line 142:
BucketFileSystemBuilderCan you add back the comment about lifecycle? I think that was useful here.
Patch Set #4, Line 173:
path_componentsCurious when this might be called with a path? Seems like we build the entire directory tree anyway. Or is it used to refresh parts of the directory tree when files are created?
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h:
Patch Set #4, Line 58:
BuildDirectoryWhat about `BuildDirectoryTree`?
Patch Set #4, Line 67:
BuildFileCurious if you think this needs to be a `static` method or anticipate this needing to be called like `BuildDirectory`? It seems like all the files will be created already with `BuildDirectory`.
Patch Set #4, Line 69:
Better to call `BuildDirectory` than to manually instantiate.Should this be private?
// Implements the FileSystemAccessDirectoryEntriesListener::DidReadDirectory
// method.Typically methods overridden are specified by adding the class above the methods.
`// mojom::blink::FileSystemAccessDirectoryEntriesListener`
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h:
Patch Set #2, Line 72:
TraceDitto Add `// GarbageCollected` above.
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h:
Patch Set #1, Line 2:
CopyrightRemove extra line above
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.cc:
Patch Set #1, Line 1:
#include "third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h"Copyright above
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.cc:
Patch Set #4, Line 119:
iterator_callbackHow about `barrier_callback`? It helps to know that this the callback that needs to run to eventually trigger the `completion_callback_`
Patch Set #4, Line 135:
BuildDirectoryDo you think we can do this without recursion? Recursion is often hard to follow and can lead to scary bugs.
Patch Set #4, Line 159:
callbackMaybe `barrier_callback` to differentiate from other callbacks? Also this should be `base::OnceClosure` as I think this should be running multiple times once it arrives here.
Patch Set #4, Line 182:
base::RepeatingClosure callbackDitto `base::OnceClosure barrier_callback`
Patch Set #4, Line 204:
DidIterateDirectoryHow about something like `DidCompleteBuildDirectory`? To make it more obvious that this is the final step.
To view, visit change 4970306. To unsubscribe, or for help writing mail filters, visit settings.
Etienne Noël (Gerrit)
Attention is currently required from: Etienne Noël.
Etienne Noël uploaded patch set #5 to this change.
Add the new FileSystem domain for the DevTools Protocol.
This CL adds the new FileSystem domain required for
the Origin Private File System in DevTools new feature.
Design doc: https://docs.google.com/document/d/1NLIVz0viaXbX_OrbmjY42ph1IcZmLJfkTBYlma0ZuLk/edit
Bug:1258806
Change-Id: Ia4a512e4c1d0b1cb44be785598df082b9aef5865
---
M headless/BUILD.gn
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/renderer/core/inspector/BUILD.gn
M third_party/blink/renderer/core/inspector/inspector_protocol_config.json
M third_party/blink/renderer/modules/buckets/storage_bucket.cc
M third_party/blink/renderer/modules/buckets/storage_bucket.h
M third_party/blink/renderer/modules/file_system_access/BUILD.gn
M third_party/blink/renderer/modules/file_system_access/DEPS
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.h
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.cc
A third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h
M third_party/blink/renderer/modules/file_system_access/storage_manager_file_system_access.cc
M third_party/blink/renderer/modules/file_system_access/storage_manager_file_system_access.h
M third_party/blink/renderer/modules/modules_initializer.cc
15 files changed, 828 insertions(+), 28 deletions(-)
To view, visit change 4970306. To unsubscribe, or for help writing mail filters, visit settings.
Etienne Noël (Gerrit)
Attention is currently required from: Ayu Ishii.
16 comments:
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.h:
// This method implements the protocol::FileSystem::Metainfo which directly
// receives and returns the response to DevTools. The response to the promise
// is provided through the callback parameter.
// It takes in a BucketFileSystemLocator which specifies a storage key, a
// storage bucket and a path component array (it can receive a nested path (in
// the case of a refresh action)). It's an array where each element is a
// nested directory level.
// This method gets the Root handle of the OPFS (for a
// specific storage key and bucket), finds the specified path by iterating
// from the root and then recursively iterates inside the specified directory
// to retrieve its nested files and directories.
I think some of specific implementation details can be removed. […]
Done
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc:
Patch Set #3, Line 71:
GetRootDirectory`GetRootDirectoryHandle`
Done
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc:
Patch Set #4, Line 142:
BucketFileSystemBuilder
Can you add back the comment about lifecycle? I think that was useful here.
With this refactor, the instantiation of the BucketFileSystemBuilder is hidden to anyone else calling it. Is it useful to still mention that the BuildDirectory instantiates the Builder using `MakeGarbageCollected`?
Patch Set #4, Line 173:
path_components
Curious when this might be called with a path? Seems like we build the entire directory tree anyway. […]
Yes, it's used to refresh parts of the directory tree. Eventually, this method will support a parameter that specifies how many nesting levels should be returned to avoid building the entire tree.
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h:
Patch Set #4, Line 58:
BuildDirectoryWhat about `BuildDirectoryTree`?
Done
Patch Set #4, Line 67:
BuildFile
Curious if you think this needs to be a `static` method or anticipate this needing to be called like […]
This will eventually be used to refresh a file so I think it makes sense to leave it as is.
Patch Set #4, Line 69:
Better to call `BuildDirectory` than to manually instantiate.Should this be private?
I don't think I can make it private since it's the `std::make_unique` library that is invoking the constructor in the `BuildDirectoryTree` method.
// Implements the FileSystemAccessDirectoryEntriesListener::DidReadDirectory
// method.
Typically methods overridden are specified by adding the class above the methods. […]
Done
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h:
Patch Set #2, Line 72:
TraceDitto Add `// GarbageCollected` above.
Done
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h:
Patch Set #1, Line 2:
CopyrightRemove extra line above
Done
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.cc:
Patch Set #1, Line 1:
#include "third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h"Copyright above
Done
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.cc:
Patch Set #4, Line 119:
iterator_callback
How about `barrier_callback`? It helps to know that this the callback that needs to run to eventuall […]
Done
Patch Set #4, Line 133:
recurisvely"recurisvely" is a possible misspelling of "recursively".
Please fix.
Patch Set #4, Line 159:
callback
Maybe `barrier_callback` to differentiate from other callbacks? Also this should be `base::OnceClosu […]
Done
Patch Set #4, Line 182:
base::RepeatingClosure callbackDitto `base::OnceClosure barrier_callback`
Done
Patch Set #4, Line 204:
DidIterateDirectory
How about something like `DidCompleteBuildDirectory`? To make it more obvious that this is the final […]
Done
To view, visit change 4970306. To unsubscribe, or for help writing mail filters, visit settings.
Andrey Kosyakov (Gerrit)
Attention is currently required from: Ayu Ishii, Danil Somsikov, Etienne Noël.
1 comment:
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc:
Patch Set #5, Line 87:
promise
The main issue I would have with doing it in the Browser is that a lot of logic (security and busine […]
As for the security requirements, the CDP client is considered trusted and can already do a lot, at least within given browser context (e.g. navigate to any origin, then evaluate things in page), so in terms of security, it's a bit more relaxed compared to the page-side view. And anyway, I would expect all those checks to be on the browser side anyway, is that not so?
As for the business logic, I didn't see much in the renderer, it appeared that it goes more or less straight to the mojo call to browser, am I missing something?
To view, visit change 4860661. To unsubscribe, or for help writing mail filters, visit settings.
Ayu Ishii (Gerrit)
Attention is currently required from: Etienne Noël.
38 comments:
Patchset:
Sorry for the super delayed review 😞
Some questions and a lots of stylistic feedback.
Thanks for the iterations so far!
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.h:
Patch Set #5, Line 8:
#include <unordered_map>Separate different include groups by adding a blank line below.
go/cstyle#Names_and_Order_of_Includes
This private method returns the root directory handle of the OPFS for a
// specific storage key and bucket.Some details can be omitted here.
```
This method retrieves the root directory handle for the OPFS.
```
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_agent.cc:
Patch Set #5, Line 73:
path_componentsUse backticks "\`" to surround `path_components`. Ditto with comment below.
Can we update to `directory_name`?
Patch Set #5, Line 126:
path_componentsSurround with backticks.
I think we can remove the variable. Maybe just...
`/*directory_name=*/""`Patch Set #5, Line 189:
storageKey`storage_key`, we use snake case instead of camel case for variable names. Ditto for all below.
go/cstyle#Variable_Names
Patch Set #5, Line 189:
Stringconst ref? here and other storage key parameters
Patch Set #5, Line 223:
storage_keyconst?
Patch Set #5, Line 225:
storage_keyI notice this doesn't take into account bucket info. What happens if this is called with the same storage key but different bucket name?
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h:
Patch Set #5, Line 6:
#define THIRD_PARTY_BLINK_RENDERER_MODULES_FILE_SYSTEM_ACCESS_BUCKET_FILE_SYSTEM_BUILDER_H_Space below.
Patch Set #5, Line 7:
#include <deque>Separate and group includes by includes types.
go/cstyle#Names_and_Order_of_Includes
#include "third_party/blink/renderer/bindings/core/v8/async_iterable.h"
#include "third_party/blink/renderer/bindings/core/v8/iterable.h"Lets clean up some includes we're not using, here and others.
End sentence with period.
End sentence with period.
Patch Set #5, Line 48:
FileSystemAccessDirectoryEntriesListenerI think you can remove this detail, since we can see that right below.
This means that for each entry in
// the directory, the `DidReadDirectory` method is being called.This too, the override method comments helps see this.
Patch Set #5, Line 69:
Better to callI think it's better to be decisive here, and say that this should only be called by `BuildDirectoryTree`, if that is how it is intended to be used.
Patch Set #5, Line 88:
nested_directoriesSurround `nested_directories_` with backticks "`" and add underscore at the end.
Patch Set #5, Line 90:
DidBuildDirectoryMaybe something like `DidBuildChildDirectory` or `DidBuildNestedDirectory`?
Patch Set #5, Line 102:
nested_files and nested_directoriesUse backticks and match variable name `nested_files_` `nested_directories_`.
Patch Set #5, Line 106:
storage_key_const?
Patch Set #5, Line 107:
Stringconst?
Patch Set #5, Line 107:
name_How about something like `directory_name_` or `base_directory_name_`?
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.h:
Patch Set #4, Line 69:
Better to call `BuildDirectory` than to manually instantiate.I don't think I can make it private since it's the `std::make_unique` library that is invoking the c […]
Acknowledged
File third_party/blink/renderer/modules/file_system_access/bucket_file_system_builder.cc:
Patch Set #5, Line 6:
memorySeparate and group includes by includes types.
go/cstyle#Names_and_Order_of_Includes
String storage_key,
String name,const?
Patch Set #5, Line 29:
nullptr`/*directory=*/nullptr` for here, and others.
`needed`? or remove
String storage_key,
String namconst?
`if`
This method only starts mapping the entries into
// `protocol::FileSystem::File` and `protocol::FileSystem::Directory` once the
// directory has been iterated over and the entries saved in here. Therefore,
// is more are expected, return and wait for a signal that no more entriesHow about...
```
Wait until all entries are iterated over and saved before converting entries
into `protocol::FileSystem::File` and `protocol::FileSystem::Directory`.
```Patch Set #5, Line 163:
RepeatingClosure`base::OnceClosure` as this should only be called once at this point.
Patch Set #5, Line 166:
resultWhat scenario would you expect `result` to be empty?
Patch Set #5, Line 171:
nullptr`/*directory=*/nullptr` here and others below.
Patch Set #5, Line 186:
RepeatingClosure`base::OnceClosure` as this should only be called once at this point.
`/*message=*/""`
To view, visit change 4970306. To unsubscribe, or for help writing mail filters, visit settings.
Etienne Noël (Gerrit)
Etienne Noël added 30 comments![]( "Open in Gerrit")
I know it's been a long while, thank you so much for the thorough review. I've cleared most of the nits, will finish the rest soon.
I will experiment with removing the recursion using a stack and see if that makes sense, then we can decide which approach works best.
// This private method returns the root directory handle of the OPFS for a
// specific storage key and bucket.Some details can be omitted here.
```
This method retrieves the root directory handle for the OPFS.
```
Done
Separate different include groups by adding a blank line below.
go/cstyle#Names_and_Order_of_Includes
Done
// The path_components contains the path of the directory that must be build andUse backticks "\`" to surround `path_components`. Ditto with comment below.
Done
Can we update to `directory_name`?
Done
// Recursively call this method until path_components becomesEtienne NoëlSurround with backticks.
Done
I think we can remove the variable. Maybe just...
`/*directory_name=*/""`
It needs to be in a variable to compile.
ScriptState* BucketFileSystemAgent::GetScriptState(String storageKey) {`storage_key`, we use snake case instead of camel case for variable names. Ditto for all below.
go/cstyle#Variable_Names
Done
ScriptState* BucketFileSystemAgent::GetScriptState(String storageKey) {const ref? here and other storage key parameters
Done
String storage_key = file_system_locator->getStorageKey();Etienne Noëlconst?
Done
How about something like `directory_name_` or `base_directory_name_`?
Done
// nested_files and nested_directories are fully created. It builds theUse backticks and match variable name `nested_files_` `nested_directories_`.
Done
// nested_directories encountered. It's passed as a callback toSurround `nested_directories_` with backticks "`" and add underscore at the end.
Done
// Better to call `BuildDirectoryTree` than to manually instantiate.I think it's better to be decisive here, and say that this should only be called by `BuildDirectoryTree`, if that is how it is intended to be used.
Done
// FileSystemAccessDirectoryEntriesListener. This means that for each entry in
// the directory, the `DidReadDirectory` method is being called.This too, the override method comments helps see this.
Done
// FileSystemAccessDirectoryEntriesListener. This means that for each entry inI think you can remove this detail, since we can see that right below.
Done
// `protocol::FileSystem::Directory` from a `FileSystemDirectoryHandle`Etienne NoëlEnd sentence with period.
Done
// `protocol::FileSystem::File` from a `FileSystemFileHandle`Etienne NoëlEnd sentence with period.
Done
Separate and group includes by includes types.
go/cstyle#Names_and_Order_of_Includes
Done
#define THIRD_PARTY_BLINK_RENDERER_MODULES_FILE_SYSTEM_ACCESS_BUCKET_FILE_SYSTEM_BUILDER_H_Etienne NoëlSpace below.
Done
Separate and group includes by includes types.
go/cstyle#Names_and_Order_of_Includes
Done
`/*directory=*/nullptr` for here, and others.
Done
// Some need info is only available via the blob. Retrieve it and build theEtienne Noël`needed`? or remove
Done
// is more are expected, return and wait for a signal that no more entriesEtienne Noël`if`
Done
// This method only starts mapping the entries into
// `protocol::FileSystem::File` and `protocol::FileSystem::Directory` once the
// directory has been iterated over and the entries saved in here. Therefore,
// is more are expected, return and wait for a signal that no more entriesHow about...
```
Wait until all entries are iterated over and saved before converting entries
into `protocol::FileSystem::File` and `protocol::FileSystem::Directory`.
```
Done
`/*directory=*/nullptr` here and others below.
- Ayu Ishii
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Noël (Gerrit)
Etienne Noël added 9 comments![]( "Open in Gerrit")
Curious when this might be called with a path? Seems like we build the entire directory tree anyway. Or is it used to refresh parts of the directory tree when files are created?
Yes, it's used to refresh parts of the directory tree. Eventually, this method will support a parameter that specifies how many nesting levels should be returned to avoid building the entire tree.
Done
auto it = handles_.find(storage_key);Etienne NoëlI notice this doesn't take into account bucket info. What happens if this is called with the same storage key but different bucket name?
Very good point. Since the lifecycle of the `BucketFileSystemAgent` is tied to a DevToolsSession, it's very probable that this list will contain the handles of multiple buckets. I have updated the hash key to be `${storage_key}:${bucket_name}`. If you have a better format for a key, please don't hesitate to tell me!
String storage_key_;Etienne Noëlconst?
Done
Maybe something like `DidBuildChildDirectory` or `DidBuildNestedDirectory`?
Great idea, thank you!
#include "third_party/blink/renderer/bindings/core/v8/async_iterable.h"
#include "third_party/blink/renderer/bindings/core/v8/iterable.h"Etienne NoëlLets clean up some includes we're not using, here and others.
Done
static void BuildFile(FileSystemFileHandle* handle, FileCallback callback);Curious if you think this needs to be a `static` method or anticipate this needing to be called like `BuildDirectory`? It seems like all the files will be created already with `BuildDirectory`.
This will eventually be used to refresh a file so I think it makes sense to leave it as is.
Done
base::RepeatingClosure barrier_callback,Etienne Noël`base::OnceClosure` as this should only be called once at this point.
Done
What scenario would you expect `result` to be empty?
I don't think the way this code is written that it can receive a null. However, it feels unintuitive for me to assume a `nullptr` will never be passed? Should I still trust that it will never be and not do the check?
base::RepeatingClosure barrier_callback,Etienne Noël`base::OnceClosure` as this should only be called once at this point.
Done
Related details
- Ayu Ishii
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Noël (Gerrit)
Etienne Noël added 1 comment![]( "Open in Gerrit")
// If there are still path_components in the array, it means we haven't yet
// reached the requested directory and must continue to recursively retrieve
// the handle for it.It does stink to have to go the browser process every time just to ignore irrelevant paths. Have we considered creating a special mojo method just for DevTools so we don't have to do this?
I've tried something here: https://chromium-review.googlesource.com/c/chromium/src/+/5552231
The relevant files:
- This file implements the logic in `FileSystemAccessManagerImpl` to retrieve a nested path on retrieval of the sandbox FileSystem.
- https://chromium-review.googlesource.com/c/chromium/src/+/5552231/4/content/browser/file_system_access/file_system_access_manager_impl.cc
- In these, a new method to retrieve a nested path is added to the `.mojom` files
- https://chromium-review.googlesource.com/c/chromium/src/+/5552231/4/third_party/blink/public/mojom/buckets/bucket_manager_host.mojom
- https://chromium-review.googlesource.com/c/chromium/src/+/5552231/4/third_party/blink/public/mojom/file_system_access/file_system_access_manager.mojom
The rest of the files are mainly just making sure everything compiles.
Let me know if you think this is a better way, if it's in line with what you had in mind and if that's better than what I'm doing in this CL.
Etienne Noël (Gerrit)
Etienne Noël added 6 comments![]( "Open in Gerrit")
@ay...@chromium.org This is ready for a second round of review. I'm missing tests on the `BucketFileSystemBuilder`. Since this will be mainly used for DevTools and never anything else, I wonder if it would be better (read here much easier to do) to cover this class using DevTools tests (testing that the devtools feature actually works).
The issue that I see with unit testing this class, is the complexity around the setup. We need to setup a bucket, OPFS, create nested files and folders to then test that the tree is properly built. This is doable but wonder if it's worth it and wanted to hear your thoughts!
// If there are still path_components in the array, it means we haven't yet
// reached the requested directory and must continue to recursively retrieve
// the handle for it.Etienne NoëlIt does stink to have to go the browser process every time just to ignore irrelevant paths. Have we considered creating a special mojo method just for DevTools so we don't have to do this?
I've tried something here: https://chromium-review.googlesource.com/c/chromium/src/+/5552231
The relevant files:
- This file implements the logic in `FileSystemAccessManagerImpl` to retrieve a nested path on retrieval of the sandbox FileSystem.
- https://chromium-review.googlesource.com/c/chromium/src/+/5552231/4/content/browser/file_system_access/file_system_access_manager_impl.cc
- In these, a new method to retrieve a nested path is added to the `.mojom` files
- https://chromium-review.googlesource.com/c/chromium/src/+/5552231/4/third_party/blink/public/mojom/buckets/bucket_manager_host.mojom
- https://chromium-review.googlesource.com/c/chromium/src/+/5552231/4/third_party/blink/public/mojom/file_system_access/file_system_access_manager.mojomThe rest of the files are mainly just making sure everything compiles.
Let me know if you think this is a better way, if it's in line with what you had in mind and if that's better than what I'm doing in this CL.
Done, we no longer iterate from the root just to get the requested directory.
BucketFileSystemBuilder::BuildDirectory(Etienne NoëlCan you add back the comment about lifecycle? I think that was useful here.
With this refactor, the instantiation of the BucketFileSystemBuilder is hidden to anyone else calling it. Is it useful to still mention that the BuildDirectory instantiates the Builder using `MakeGarbageCollected`?
With the latest refactor, this is no longer needed.
storage_key, root, std::move(path_components),Etienne NoëlI think we can remove the variable. Maybe just...
`/*directory_name=*/""`
It needs to be in a variable to compile.
Done
auto it = handles_.find(storage_key);Etienne NoëlI notice this doesn't take into account bucket info. What happens if this is called with the same storage key but different bucket name?
Very good point. Since the lifecycle of the `BucketFileSystemAgent` is tied to a DevToolsSession, it's very probable that this list will contain the handles of multiple buckets. I have updated the hash key to be `${storage_key}:${bucket_name}`. If you have a better format for a key, please don't hesitate to tell me!
No longer relevant since we don't cache handles (for now).
Do you think we can do this without recursion? Recursion is often hard to follow and can lead to scary bugs.
As discussed, I've removed the recursion and am only retrieving one level at a time.
Related details
- Ayu Ishii
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ayu Ishii (Gerrit)
Ayu Ishii added 18 comments![]( "Open in Gerrit")
Thanks for the updates, this looks great! Mostly minor comments, and suggestion on testing. Let me know if you have any question!
@ay...@chromium.org This is ready for a second round of review. I'm missing tests on the `BucketFileSystemBuilder`. Since this will be mainly used for DevTools and never anything else, I wonder if it would be better (read here much easier to do) to cover this class using DevTools tests (testing that the devtools feature actually works).
The issue that I see with unit testing this class, is the complexity around the setup. We need to setup a bucket, OPFS, create nested files and folders to then test that the tree is properly built. This is doable but wonder if it's worth it and wanted to hear your thoughts!
Looks like folks typically write web_tests for inspector agent changes. I presume you can use the js APIs to setup which hopefully would be easier, but I haven't looked into this deeply. WDYT?
https://crsrc.org/c/third_party/blink/web_tests/http/tests/inspector-protocol/
# Files that are directly nested under this directorynit: Terminate with period. Ditto for other comments that are sentences.
go/cstyle#Punctuation,_Spelling_and_Grammar
// Copyright 2023 The Chromium Authorsnit: 2024 😜 ditto for others
if (!error) {Maybe this error null check should come first, so you don't have to check twice?
```
if (!error) {
return crdtp::DispatchResponse::InternalError();
}
if (error->status == mojom::blink::FileSystemAccessStatus::kOk) {
return crdtp::DispatchResponse::Success();
}
return crdtp::DispatchResponse::ServerError(error->message.Utf8());
```
nit: Lets remove some of these blank lines for readability. Maybe something like...
go/cstyle#Vertical_Whitespace
```
String storage_key = file_system_locator->getStorageKey();
StorageBucket* storage_bucket =
GetStorageBucket(storage_key, file_system_locator->getBucketName());
if (storage_bucket == nullptr) {
callback->sendFailure(
protocol::Response::InvalidRequest("Storage Bucket not found."));
return;
}
WTF::Vector<WTF::String> path_components = WTF::Vector<WTF::String>();
for (const auto& component : *file_system_locator->getPathComponents()) {
path_components.push_back(component);
}
String directory_name = path_components.empty() ? "" : path_components.back();
storage_bucket->GetDirectoryForDevTools(
GetExecutionContext(storage_key), path_components,
WTF::BindOnce(&BucketFileSystemAgent::DidGetDirectoryHandle,
WrapPersistent(this),
WrapWeakPersistent(GetExecutionContext(storage_key)),
storage_key, directory_name, std::move(callback)));
```
CHECK(storage_bucket_manager);Can this be nullptr? Looks like it just create one if it there isn't one already?
ScriptState* BucketFileSystemAgent::GetScriptState(const String& storage_key) {This is only called in once place, maybe we can inline? Also should we be checking for nullptr here?
nit: I think we can return the these white spaces before `return` in these methods.
go/cstyle#Vertical_Whitespace
CHECK(frame);Should this be crashing if they fail? I think there might be edge case that are valid scenarios of when a document might go away, but wouldn't necessarily want to crash. May want to fail/error gracefully.
typedef base::OnceCallback<void(Can we use `using` here?
go/cstyle#Aliases
if (!handle) {I feel like we should do this before it gets here, maybe right after its been retrieve. WDYT about doing this in `DidGetDirectoryHandle`? And `CHECK(handle)` here.
return;Should the callback still return here? Can the caller still be waiting?
((FileSystemFileHandle*)entry)I've never seen this pattern for down casting before. Can this be `To<FileSystemFileHandle>(entry)->`?
BucketFileSystemBuilder::BuildDirectory(Etienne NoëlDo you think we can do this without recursion? Recursion is often hard to follow and can lead to scary bugs.
As discussed, I've removed the recursion and am only retrieving one level at a time.
Done
if (!result || !directory) {Etienne NoëlWhat scenario would you expect `result` to be empty?
I don't think the way this code is written that it can receive a null. However, it feels unintuitive for me to assume a `nullptr` will never be passed? Should I still trust that it will never be and not do the check?
Done
nit: Remove vertical line, ditto for other applicable areas.
Related details
- Etienne Noël
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Noël (Gerrit)
Etienne Noël added 16 comments![]( "Open in Gerrit")
Ayu Ishii@ay...@chromium.org This is ready for a second round of review. I'm missing tests on the `BucketFileSystemBuilder`. Since this will be mainly used for DevTools and never anything else, I wonder if it would be better (read here much easier to do) to cover this class using DevTools tests (testing that the devtools feature actually works).
The issue that I see with unit testing this class, is the complexity around the setup. We need to setup a bucket, OPFS, create nested files and folders to then test that the tree is properly built. This is doable but wonder if it's worth it and wanted to hear your thoughts!
Looks like folks typically write web_tests for inspector agent changes. I presume you can use the js APIs to setup which hopefully would be easier, but I haven't looked into this deeply. WDYT?
https://crsrc.org/c/third_party/blink/web_tests/http/tests/inspector-protocol/
That's exactly what I was looking for. I will add those. I feel this will be much easier, thanks!
nit: Terminate with period. Ditto for other comments that are sentences.
go/cstyle#Punctuation,_Spelling_and_Grammar
Sorry, it doesn't stick in my head. I'm adding this to the list of things to triple check when submitting a CL. Thanks.
// Copyright 2023 The Chromium Authorsnit: 2024 😜 ditto for others
LOL thanks haha
if (!error) {Maybe this error null check should come first, so you don't have to check twice?
```
if (!error) {
return crdtp::DispatchResponse::InternalError();
}
if (error->status == mojom::blink::FileSystemAccessStatus::kOk) {
return crdtp::DispatchResponse::Success();
}
return crdtp::DispatchResponse::ServerError(error->message.Utf8());
```
Done
nit: Lets remove some of these blank lines for readability. Maybe something like...
go/cstyle#Vertical_Whitespace```
String storage_key = file_system_locator->getStorageKey();
StorageBucket* storage_bucket =
GetStorageBucket(storage_key, file_system_locator->getBucketName());
if (storage_bucket == nullptr) {
callback->sendFailure(
protocol::Response::InvalidRequest("Storage Bucket not found."));
return;
}WTF::Vector<WTF::String> path_components = WTF::Vector<WTF::String>();
for (const auto& component : *file_system_locator->getPathComponents()) {
path_components.push_back(component);
}String directory_name = path_components.empty() ? "" : path_components.back();
storage_bucket->GetDirectoryForDevTools(
GetExecutionContext(storage_key), path_components,
WTF::BindOnce(&BucketFileSystemAgent::DidGetDirectoryHandle,
WrapPersistent(this),
WrapWeakPersistent(GetExecutionContext(storage_key)),
storage_key, directory_name, std::move(callback)));
```
Done
CHECK(storage_bucket_manager);Can this be nullptr? Looks like it just create one if it there isn't one already?
How should I think about such scenarios. My thinking is that if this code changes in the future, I prefer to put a check to ensure that this doesn't turn into a `nullptr`. However, we can also say that this is the responsibility of the person that will modify `StorageBucketManager::storageBuckets(*navigator)`.
In chromium's codebase, how should I balance these two things? I always prefer to be cautious but also want to follow chromium's mentality.
ScriptState* BucketFileSystemAgent::GetScriptState(const String& storage_key) {This is only called in once place, maybe we can inline? Also should we be checking for nullptr here?
Done
nit: I think we can return the these white spaces before `return` in these methods.
go/cstyle#Vertical_Whitespace
Done
Should this be crashing if they fail? I think there might be edge case that are valid scenarios of when a document might go away, but wouldn't necessarily want to crash. May want to fail/error gracefully.
I've removed the methods. I retrieving these elements, checking for nullptr and gracefully returning an error.
Can we use `using` here?
go/cstyle#Aliases
Done
Done
I feel like we should do this before it gets here, maybe right after its been retrieve. WDYT about doing this in `DidGetDirectoryHandle`? And `CHECK(handle)` here.
Nice yes that's clean I like it.
return;Should the callback still return here? Can the caller still be waiting?
For my own understanding, can you trigger a callback when the execution context is destroyed? I thought this would mean that the callback would no longer be executable.
I've never seen this pattern for down casting before. Can this be `To<FileSystemFileHandle>(entry)->`?
Done
BucketFileSystemBuilder::BuildDirectory(Etienne NoëlDo you think we can do this without recursion? Recursion is often hard to follow and can lead to scary bugs.
As discussed, I've removed the recursion and am only retrieving one level at a time.
Done
nit: Remove vertical line, ditto for other applicable areas.
I think I've found them all. Is there a link to the code styling for new lines?
Related details
- Ayu Ishii
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ayu Ishii (Gerrit)
Ayu Ishii added 3 comments![]( "Open in Gerrit")
CHECK(storage_bucket_manager);Etienne NoëlCan this be nullptr? Looks like it just create one if it there isn't one already?
How should I think about such scenarios. My thinking is that if this code changes in the future, I prefer to put a check to ensure that this doesn't turn into a `nullptr`. However, we can also say that this is the responsibility of the person that will modify `StorageBucketManager::storageBuckets(*navigator)`.
In chromium's codebase, how should I balance these two things? I always prefer to be cautious but also want to follow chromium's mentality.
Good question. Specifically `storageBuckets` needs to operate in a way that it will always return because it is web exposed. Otherwise there could be a scenario where a StorageBucket object won't return when the JS API is called.
Not very helpful, but for me I think of this case by case. I don't think it is very reasonable to need to check every return value, so I typically look into the method and see how it is implemented, and evaluate if it should CHECK or fail gracefully and return. Others might have other better methods though.
return;Etienne NoëlShould the callback still return here? Can the caller still be waiting?
For my own understanding, can you trigger a callback when the execution context is destroyed? I thought this would mean that the callback would no longer be executable.
Yeah, good question. I'm not sure about the answer. But the execution context is for the frame for the StorageKey, and the callback is from the DevTools protocol which I assume wouldn't have the same exact context as the StorageKey (not sure) so I'm not sure if those would be destroyed at the same time.
Etienne Noëlnit: Remove vertical line, ditto for other applicable areas.
I think I've found them all. Is there a link to the code styling for new lines?
- Etienne Noël
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Noël (Gerrit)
Etienne Noël added 4 comments![]( "Open in Gerrit")
Ayu Ishii@ay...@chromium.org This is ready for a second round of review. I'm missing tests on the `BucketFileSystemBuilder`. Since this will be mainly used for DevTools and never anything else, I wonder if it would be better (read here much easier to do) to cover this class using DevTools tests (testing that the devtools feature actually works).
The issue that I see with unit testing this class, is the complexity around the setup. We need to setup a bucket, OPFS, create nested files and folders to then test that the tree is properly built. This is doable but wonder if it's worth it and wanted to hear your thoughts!
Etienne NoëlLooks like folks typically write web_tests for inspector agent changes. I presume you can use the js APIs to setup which hopefully would be easier, but I haven't looked into this deeply. WDYT?
https://crsrc.org/c/third_party/blink/web_tests/http/tests/inspector-protocol/
That's exactly what I was looking for. I will add those. I feel this will be much easier, thanks!
Done
@ds...@chromium.org When you get a moment, can you review some of the file system files please? Ayu is OOO
CHECK(storage_bucket_manager);Etienne NoëlCan this be nullptr? Looks like it just create one if it there isn't one already?
Ayu IshiiHow should I think about such scenarios. My thinking is that if this code changes in the future, I prefer to put a check to ensure that this doesn't turn into a `nullptr`. However, we can also say that this is the responsibility of the person that will modify `StorageBucketManager::storageBuckets(*navigator)`.
In chromium's codebase, how should I balance these two things? I always prefer to be cautious but also want to follow chromium's mentality.
Good question. Specifically `storageBuckets` needs to operate in a way that it will always return because it is web exposed. Otherwise there could be a scenario where a StorageBucket object won't return when the JS API is called.
Not very helpful, but for me I think of this case by case. I don't think it is very reasonable to need to check every return value, so I typically look into the method and see how it is implemented, and evaluate if it should CHECK or fail gracefully and return. Others might have other better methods though.
Done
return;Etienne NoëlShould the callback still return here? Can the caller still be waiting?
Ayu IshiiFor my own understanding, can you trigger a callback when the execution context is destroyed? I thought this would mean that the callback would no longer be executable.
Yeah, good question. I'm not sure about the answer. But the execution context is for the frame for the StorageKey, and the callback is from the DevTools protocol which I assume wouldn't have the same exact context as the StorageKey (not sure) so I'm not sure if those would be destroyed at the same time.
- Ayu Ishii
- Daseul Lee
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Noël (Gerrit)
Etienne Noël added 1 comment![]( "Open in Gerrit")
@ca...@chromium.org I've finally had some time to continue the work on this (previous CL that you reviewed was this one: https://chromium-review.googlesource.com/c/chromium/src/+/4860661).
If you can review this one and let me know what you think, that would be great! Thank you.
Related details
- Andrey Kosyakov
- Ayu Ishii
- Daseul Lee
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Noël (Gerrit)
Etienne Noël added 1 comment![]( "Open in Gerrit")
@ca...@chromium.org I've finally had some time to continue the work on this (previous CL that you reviewed was this one: https://chromium-review.googlesource.com/c/chromium/src/+/4860661).
If you can review this one and let me know what you think, that would be great! Thank you.
Forgot to add: This is doing less back and forth (between renderer and browser) and directly calling the Mojo endpoint (which makes more sense).
Daseul Lee (Gerrit)
Daseul Lee added 6 comments![]( "Open in Gerrit")
WTF::Vector<WTF::String> path_components = WTF::Vector<WTF::String>();
for (const auto& component : *file_system_locator->getPathComponents()) {
path_components.push_back(component);
}Shall we move this below all the failure handling code? so that we don't have to construct the vector until it's necessary
Also, just curious - we cannot just return `file_system_locator->getPathComponents()` because it's different data types?
callback->sendFailure(protocol::Response::ServerError("No handle."));nit: is this for failing to retrieve the root directory handle? if so, would be useful to specify something like "No root handle", to differentiate this from failing to retrieve child directories or files (below).
DirectoryCallback completion_callback_;Is `completion_callback_` expected to be invoked only once? If so, I wonder if this class's constructor should be made private, or provide other way to prevent re-using the object state.
}The `else` case shouldn't really happen, but wonder if it's worth putting a CHECK() here since the barrier callback will hang for not executing `file_system_handle_queue_->size()` times otherwise.
base::File::Error::FILE_ERROR_FAILED, "Invalid state Error."),nit: provide a specific error message, or omit it since it's the same name as the FileSystemAccessStatus anyways
if (!result || !file) {
std::move(completion_callback_)
.Run(mojom::blink::FileSystemAccessError::New(
mojom::blink::FileSystemAccessStatus::kInvalidState,
base::File::Error::FILE_ERROR_FAILED, "Invalid state Error."),
/*std::unique_ptr<protocol::FileSystem::Directory>*/ nullptr);
return;
}
if (result->status != mojom::blink::FileSystemAccessStatus::kOk) {
std::move(completion_callback_)
.Run(std::move(result),
/*std::unique_ptr<protocol::FileSystem::Directory>*/ nullptr);
return;
}In either case, should the barrier_callback be cancelled, since it would continue waiting?
Related details
- Andrey Kosyakov
- Ayu Ishii
- Etienne Noël
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Noël (Gerrit)
Etienne Noël voted and added 6 comments![]( "Open in Gerrit")
Votes added by Etienne Noël
| Commit-Queue | +1 |
6 comments
WTF::Vector<WTF::String> path_components = WTF::Vector<WTF::String>();
for (const auto& component : *file_system_locator->getPathComponents()) {
path_components.push_back(component);
}Shall we move this below all the failure handling code? so that we don't have to construct the vector until it's necessary
Also, just curious - we cannot just return `file_system_locator->getPathComponents()` because it's different data types?
`getPathComponents` is of type `protocol::Array<String>`which is `std::vector<WTF:String>` and I haven't seen an automatic conversion to `WTF::Vector`.
Good catch on the doing it later on part. Thanks!
callback->sendFailure(protocol::Response::ServerError("No handle."));nit: is this for failing to retrieve the root directory handle? if so, would be useful to specify something like "No root handle", to differentiate this from failing to retrieve child directories or files (below).
Not necessarily. It can be for either the root handle or a nested handle.
DirectoryCallback completion_callback_;Is `completion_callback_` expected to be invoked only once? If so, I wonder if this class's constructor should be made private, or provide other way to prevent re-using the object state.
I don't know how to make the constructor private since I'm creating the `BucketFileSystemBuilder` using `MakeGarbageCollected`, this needs to be a friend of `BucketFileSystemBuilder`, but that would defeat the purpose of making the constructor private.
The idea of this class is that you use the static method `BuildDirectoryTree` every time you need to retrieve something. Since this is a once callback, it will fail trying to use it again so I wonder if I should actually do anything more to prevent using it? WDYT?
The `else` case shouldn't really happen, but wonder if it's worth putting a CHECK() here since the barrier callback will hang for not executing `file_system_handle_queue_->size()` times otherwise.
If we ever introduce another type, you are right that this would suddenly start failing. I agree with adding a `CHECK`.
base::File::Error::FILE_ERROR_FAILED, "Invalid state Error."),nit: provide a specific error message, or omit it since it's the same name as the FileSystemAccessStatus anyways
I've added something, let me know if you think this is clear enough.
if (!result || !file) {
std::move(completion_callback_)
.Run(mojom::blink::FileSystemAccessError::New(
mojom::blink::FileSystemAccessStatus::kInvalidState,
base::File::Error::FILE_ERROR_FAILED, "Invalid state Error."),
/*std::unique_ptr<protocol::FileSystem::Directory>*/ nullptr);
return;
}
if (result->status != mojom::blink::FileSystemAccessStatus::kOk) {
std::move(completion_callback_)
.Run(std::move(result),
/*std::unique_ptr<protocol::FileSystem::Directory>*/ nullptr);
return;
}In either case, should the barrier_callback be cancelled, since it would continue waiting?
I was under the impression that because this `barrier_callback` is held by the `BucketFileSystemBuilder`, which gets garbage collected after the `completion_callback` is executed, that the `barrier_callback` would be automatically canceled. Is there something I'm not seeing?
Related details
- Andrey Kosyakov
- Ayu Ishii
- Daseul Lee
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daseul Lee (Gerrit)
Daseul Lee voted and added 5 comments![]( "Open in Gerrit")
Votes added by Daseul Lee
| Code-Review | +1 |
5 comments
DirectoryCallback completion_callback_;Etienne NoëlIs `completion_callback_` expected to be invoked only once? If so, I wonder if this class's constructor should be made private, or provide other way to prevent re-using the object state.
I don't know how to make the constructor private since I'm creating the `BucketFileSystemBuilder` using `MakeGarbageCollected`, this needs to be a friend of `BucketFileSystemBuilder`, but that would defeat the purpose of making the constructor private.
The idea of this class is that you use the static method `BuildDirectoryTree` every time you need to retrieve something. Since this is a once callback, it will fail trying to use it again so I wonder if I should actually do anything more to prevent using it? WDYT?
Gotcha. It seems okay to leave as then.
Etienne NoëlThe `else` case shouldn't really happen, but wonder if it's worth putting a CHECK() here since the barrier callback will hang for not executing `file_system_handle_queue_->size()` times otherwise.
If we ever introduce another type, you are right that this would suddenly start failing. I agree with adding a `CHECK`.
Sorry, I should have said `NOTREACHED_NORETURN();`
base::File::Error::FILE_ERROR_FAILED, "Invalid state Error."),Etienne Noëlnit: provide a specific error message, or omit it since it's the same name as the FileSystemAccessStatus anyways
I've added something, let me know if you think this is clear enough.
Hmm, what about "Failed to retrieve blob info"?
if (!result || !file) {
std::move(completion_callback_)
.Run(mojom::blink::FileSystemAccessError::New(
mojom::blink::FileSystemAccessStatus::kInvalidState,
base::File::Error::FILE_ERROR_FAILED, "Invalid state Error."),
/*std::unique_ptr<protocol::FileSystem::Directory>*/ nullptr);
return;
}
if (result->status != mojom::blink::FileSystemAccessStatus::kOk) {
std::move(completion_callback_)
.Run(std::move(result),
/*std::unique_ptr<protocol::FileSystem::Directory>*/ nullptr);
return;
}Etienne NoëlIn either case, should the barrier_callback be cancelled, since it would continue waiting?
I was under the impression that because this `barrier_callback` is held by the `BucketFileSystemBuilder`, which gets garbage collected after the `completion_callback` is executed, that the `barrier_callback` would be automatically canceled. Is there something I'm not seeing?
- Andrey Kosyakov
- Ayu Ishii
- Etienne Noël
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Noël (Gerrit)
Etienne Noël added 2 comments![]( "Open in Gerrit")
Etienne NoëlThe `else` case shouldn't really happen, but wonder if it's worth putting a CHECK() here since the barrier callback will hang for not executing `file_system_handle_queue_->size()` times otherwise.
Daseul LeeIf we ever introduce another type, you are right that this would suddenly start failing. I agree with adding a `CHECK`.
Sorry, I should have said `NOTREACHED_NORETURN();`
Done
base::File::Error::FILE_ERROR_FAILED, "Invalid state Error."),Etienne Noëlnit: provide a specific error message, or omit it since it's the same name as the FileSystemAccessStatus anyways
Daseul LeeI've added something, let me know if you think this is clear enough.
Hmm, what about "Failed to retrieve blob info"?
Done
Related details
- Andrey Kosyakov
- Ayu Ishii
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Noël (Gerrit)
Etienne Noël added 1 comment![]( "Open in Gerrit")
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
Bug:1258806is the bug number wrong? it links to https://g-issues.chromium.org/issues/40797264 for me
Related details
- Andrey Kosyakov
- Ayu Ishii
- Etienne Noël
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 5 comments![]( "Open in Gerrit")
sorry for the unsolicited review, saw a notification in the mailbox and it looked interesting (left some suggestions).
number sizeCould you please add a comment about the units for the size (bytes?)?
string mimeTypenit: mimeTypes has been renamed to mediaTypes in https://www.iana.org/assignments/media-types/media-types.xhtml so we might prefer a new name, I wonder if mime types is used in the filesystem spec?
# Bucket Namenit: could you please add some information about what a bucket is and how to find one to call this API?
Etienne Noël (Gerrit)
Etienne Noël added 6 comments![]( "Open in Gerrit")
is the bug number wrong? it links to https://g-issues.chromium.org/issues/40797264 for me
Thanks, fixed it.
Could you please add a comment about the units for the size (bytes?)?
Done
string mimeTypenit: mimeTypes has been renamed to mediaTypes in https://www.iana.org/assignments/media-types/media-types.xhtml so we might prefer a new name, I wonder if mime types is used in the filesystem spec?
In the spec, the attribute is "type" and MIME Type is mentioned. I can put `type` to align with the existing naming?
Oh yes good catch!
# Bucket Namenit: could you please add some information about what a bucket is and how to find one to call this API?
I'm not sure I get this question. The latest name of this API, is the Bucket File System, meaning that you kind of need to know what a bucket is to use it. I'm not sure exactly what I should be describing aside from linking to the API. Should I describe that if you don't pass anything, you will retrieve the default bucket automatically?
Related details
- Alex Rudenko
- Andrey Kosyakov
- Ayu Ishii
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 3 comments![]( "Open in Gerrit")
Bug:1258806Etienne Noëlis the bug number wrong? it links to https://g-issues.chromium.org/issues/40797264 for me
Thanks, fixed it.
I think the bug link still does not work for me, still links to the bindOnce issue.
string mimeTypeEtienne Noëlnit: mimeTypes has been renamed to mediaTypes in https://www.iana.org/assignments/media-types/media-types.xhtml so we might prefer a new name, I wonder if mime types is used in the filesystem spec?
In the spec, the attribute is "type" and MIME Type is mentioned. I can put `type` to align with the existing naming?
aligning with the spec terms sounds good to me!
# Bucket NameEtienne Noëlnit: could you please add some information about what a bucket is and how to find one to call this API?
I'm not sure I get this question. The latest name of this API, is the Bucket File System, meaning that you kind of need to know what a bucket is to use it. I'm not sure exactly what I should be describing aside from linking to the API. Should I describe that if you don't pass anything, you will retrieve the default bucket automatically?
My question comes from my unfamiliarity with the API, is it this spec that defines this https://fs.spec.whatwg.org/#sandboxed-filesystem (I see that getDirectory does not accept a bucket name in the web-facing API)? In general, users will discover the filesystem domain through the documentation generated from this definition (e.g., PWA domain https://chromedevtools.github.io/devtools-protocol/tot/PWA/) so it'd be nice to include links to the concepts that might be unknown.
Related details
- Andrey Kosyakov
- Ayu Ishii
- Etienne Noël
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 2 comments![]( "Open in Gerrit")
protocol LGTM % comments
await dp.Runtime.enable();
dp.Runtime.onConsoleAPICalled(e => { testRunner.log(e.params.args[0].value); });it looks like these two lines are not needed for the test?