Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14317004:
Add ContentBrowserClient::GetAdditionalFileSystemMountPointProviders() (Closed)
|
Created:
7 years, 8 months ago by kinuko Modified:
7 years, 8 months ago Reviewers:
jam CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, kinuko+watch, tzik+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd ContentBrowserClient::GetAdditionalFileSystemMountPointProviders()
So that we can instantiate and add chrome-specific FileSystem modules
in its implementation (i.e. ChromeContentBrowserClient).
(Brief bigger plan: Brief change plan (internal): https://docs.google.com/a/google.com/document/d/1XEtX0OO_RIA_c0KuKvd9yqK5l3XtsTulwXs3c-mAQ1o/view)
BUG=175936
Patch Set 1 : #
Total comments: 1
Patch Set 2 : AdditionalFSMountPointProvider -> AdditionalFSMountPointProviders #
Messages
Total messages: 10 (0 generated)
Hi Jam, I want to add GetAdditionalFileSystemMountPointProviders method to ContentBrowserClient so that chrome layer modules can easily plug their own FS modules into content layer. Can you review? Thanks!
https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... content/public/browser/content_browser_client.h:505: virtual void GetAdditionalFileSystemMountPointProvider( why is this a ScopedVector instead of passing in a std::vector like the other methods in this class? nit: GetAdditionalFileSystemMountPointProviders
2013/04/19 1:16 <jam@chromium.org>: > > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > File content/public/browser/content_browser_client.h (right): > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > content/public/browser/content_browser_client.h:505: virtual void > GetAdditionalFileSystemMountPointProvider( > why is this a ScopedVector instead of passing in a std::vector like the > other methods in this class? I wanted to make it clear that the pointers returned by this method will be owned by the caller. I could change the code with explicit comment about ownership if this doesn't fit with other code here. > nit: GetAdditionalFileSystemMountPointProviders > > https://codereview.chromium.org/14317004/
On 2013/04/19 01:11:45, kinuko wrote: > 2013/04/19 1:16 <jam@chromium.org>: > > > > > > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > > File content/public/browser/content_browser_client.h (right): > > > > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > > content/public/browser/content_browser_client.h:505: virtual void > > GetAdditionalFileSystemMountPointProvider( > > why is this a ScopedVector instead of passing in a std::vector like the > > other methods in this class? > > I wanted to make it clear that the pointers returned by this method will be > owned by the caller. I could change the code with explicit comment about > ownership if this doesn't fit with other code here. > > > nit: GetAdditionalFileSystemMountPointProviders > > > > https://codereview.chromium.org/14317004/ Do you plan on changing the interface so that the providers (which as I understand them ought to basically be per-filesystem-type factories that produce AsyncFileUtils for particular instances of those filesystem types) remain owned by the ContentBrowserClient? That architecture would make the most sense to me. It'd make the factories centrally registered, and then the per-instance objects they make be owned by the filesystem api code. Ultimately, it'd be nice to encapsulate that so the filesystem api code only dealt with one FileSystemMountPointProvider, which then internally dealt with all the registered providers. That gives maximal separation between the filesystem api code which maps that API onto the JS world, and the implementation specifics needed to massage various data sources to conform to the API. My sense is that would better fit the overall mission of the webkit/* layer.
On 2013/04/19 16:30:15, Greg Billock wrote: > On 2013/04/19 01:11:45, kinuko wrote: > > 2013/04/19 1:16 <jam@chromium.org>: > > > > > > > > > > > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > > > File content/public/browser/content_browser_client.h (right): > > > > > > > > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > > > content/public/browser/content_browser_client.h:505: virtual void > > > GetAdditionalFileSystemMountPointProvider( > > > why is this a ScopedVector instead of passing in a std::vector like the > > > other methods in this class? > > > > I wanted to make it clear that the pointers returned by this method will be > > owned by the caller. I could change the code with explicit comment about > > ownership if this doesn't fit with other code here. > > > > > nit: GetAdditionalFileSystemMountPointProviders > > > > > > https://codereview.chromium.org/14317004/ > > Do you plan on changing the interface so that the providers (which as I > understand them ought to basically be per-filesystem-type factories that produce > AsyncFileUtils for particular instances of those filesystem types) remain owned > by the ContentBrowserClient? That architecture would make the most sense to me. > It'd make the factories centrally registered, and then the per-instance objects > they make be owned by the filesystem api code. > > Ultimately, it'd be nice to encapsulate that so the filesystem api code only > dealt with one FileSystemMountPointProvider, which then internally dealt with > all the registered providers. That gives maximal separation between the > filesystem api code which maps that API onto the JS world, and the > implementation specifics needed to massage various data sources to conform to > the API. My sense is that would better fit the overall mission of the webkit/* > layer. Based on this patch I was able to move all of webkit/fileapi/media into chrome/browser/media_galleries/fileapi. I had to change the signature to: virtual void GetAdditionalFileSystemMountPointProviders( const base::FilePath& profile_path, ScopedVector<fileapi::FileSystemMountPointProvider>* additional_providers) {} as MediaFileSystemMountPointProvider required a profile_path argument.
On 2013/04/19 16:30:15, Greg Billock wrote: > On 2013/04/19 01:11:45, kinuko wrote: > > 2013/04/19 1:16 <jam@chromium.org>: > > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > > > File content/public/browser/content_browser_client.h (right): > > > > > > > > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > > > content/public/browser/content_browser_client.h:505: virtual void > > > GetAdditionalFileSystemMountPointProvider( > > > why is this a ScopedVector instead of passing in a std::vector like the > > > other methods in this class? > > > > I wanted to make it clear that the pointers returned by this method will be > > owned by the caller. I could change the code with explicit comment about > > ownership if this doesn't fit with other code here. > > > > > nit: GetAdditionalFileSystemMountPointProviders > > > > > > https://codereview.chromium.org/14317004/ > > Do you plan on changing the interface so that the providers (which as I > understand them ought to basically be per-filesystem-type factories that produce > AsyncFileUtils for particular instances of those filesystem types) remain owned > by the ContentBrowserClient? That architecture would make the most sense to me. > It'd make the factories centrally registered, and then the per-instance objects > they make be owned by the filesystem api code. As for the ownership: I doubt ContentBrowserClient is supposed to 'own' some modules. Do you mean handing the ownership of providers to FileAPI module isn't really convenient for FS module implementors, or are you suggesting that we should have a separate central registration module in addition to FileSystemContext, and each FS module should just call the registration layer to register its own MountPointProvider (or its factory class)? I'm slightly concerned with the initialization order and dependency issues and the current design is trying to avoid having those implicit dependencies. (I can drop this design though if your team's task priorities would be ok) As for the 'providers', MountPointProviders are also responsible for creating module-specific FileStreamReade/FileStreamWriter, providing permission policy and doing some other misc module-specific tasks. (Though in MediaGalleries case we've only focused on Async/Sync FileUtils) > Ultimately, it'd be nice to encapsulate that so the filesystem api code only > dealt with one FileSystemMountPointProvider, which then internally dealt with > all the registered providers. That gives maximal separation between the > filesystem api code which maps that API onto the JS world, and the > implementation specifics needed to massage various data sources to conform to > the API. My sense is that would better fit the overall mission of the webkit/* > layer. If we could cleanly separate module registration and dispatch part from the current filesystem api code that'd be nice, but my current feeling is we have a few more steps before we could do that. Some FS modules (especially the regular ones) are more intricate into the filesystem api code and we'll need some more cleanups.
On 2013/04/20 00:56:45, tommycli wrote: > On 2013/04/19 16:30:15, Greg Billock wrote: > > On 2013/04/19 01:11:45, kinuko wrote: > > > 2013/04/19 1:16 <jam@chromium.org>: > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > > > > File content/public/browser/content_browser_client.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > > > > content/public/browser/content_browser_client.h:505: virtual void > > > > GetAdditionalFileSystemMountPointProvider( > > > > why is this a ScopedVector instead of passing in a std::vector like the > > > > other methods in this class? > > > > > > I wanted to make it clear that the pointers returned by this method will be > > > owned by the caller. I could change the code with explicit comment about > > > ownership if this doesn't fit with other code here. > > > > > > > nit: GetAdditionalFileSystemMountPointProviders > > > > > > > > https://codereview.chromium.org/14317004/ > > > > Do you plan on changing the interface so that the providers (which as I > > understand them ought to basically be per-filesystem-type factories that > produce > > AsyncFileUtils for particular instances of those filesystem types) remain > owned > > by the ContentBrowserClient? That architecture would make the most sense to > me. > > It'd make the factories centrally registered, and then the per-instance > objects > > they make be owned by the filesystem api code. > > > > Ultimately, it'd be nice to encapsulate that so the filesystem api code only > > dealt with one FileSystemMountPointProvider, which then internally dealt with > > all the registered providers. That gives maximal separation between the > > filesystem api code which maps that API onto the JS world, and the > > implementation specifics needed to massage various data sources to conform to > > the API. My sense is that would better fit the overall mission of the webkit/* > > layer. > > Based on this patch I was able to move all of webkit/fileapi/media into > chrome/browser/media_galleries/fileapi. That sounds nice, thanks for doing the massive moving! > I had to change the signature to: > > virtual void GetAdditionalFileSystemMountPointProviders( > const base::FilePath& profile_path, > ScopedVector<fileapi::FileSystemMountPointProvider>* > additional_providers) {} > > as MediaFileSystemMountPointProvider required a profile_path argument. 'profile' is probably not a resident of content layer, I guess we'd better use 'partition_path' (i.e. storage partition path) or something?
On 2013/04/22 15:48:19, kinuko wrote: > On 2013/04/20 00:56:45, tommycli wrote: > > On 2013/04/19 16:30:15, Greg Billock wrote: > > > On 2013/04/19 01:11:45, kinuko wrote: > > > > 2013/04/19 1:16 <jam@chromium.org>: > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > > > > > File content/public/browser/content_browser_client.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > > > > > content/public/browser/content_browser_client.h:505: virtual void > > > > > GetAdditionalFileSystemMountPointProvider( > > > > > why is this a ScopedVector instead of passing in a std::vector like the > > > > > other methods in this class? > > > > > > > > I wanted to make it clear that the pointers returned by this method will > be > > > > owned by the caller. I could change the code with explicit comment about > > > > ownership if this doesn't fit with other code here. > > > > > > > > > nit: GetAdditionalFileSystemMountPointProviders > > > > > > > > > > https://codereview.chromium.org/14317004/ > > > > > > Do you plan on changing the interface so that the providers (which as I > > > understand them ought to basically be per-filesystem-type factories that > > produce > > > AsyncFileUtils for particular instances of those filesystem types) remain > > owned > > > by the ContentBrowserClient? That architecture would make the most sense to > > me. > > > It'd make the factories centrally registered, and then the per-instance > > objects > > > they make be owned by the filesystem api code. > > > > > > Ultimately, it'd be nice to encapsulate that so the filesystem api code only > > > dealt with one FileSystemMountPointProvider, which then internally dealt > with > > > all the registered providers. That gives maximal separation between the > > > filesystem api code which maps that API onto the JS world, and the > > > implementation specifics needed to massage various data sources to conform > to > > > the API. My sense is that would better fit the overall mission of the > webkit/* > > > layer. > > > > Based on this patch I was able to move all of webkit/fileapi/media into > > chrome/browser/media_galleries/fileapi. > > That sounds nice, thanks for doing the massive moving! > > > I had to change the signature to: > > > > virtual void GetAdditionalFileSystemMountPointProviders( > > const base::FilePath& profile_path, > > ScopedVector<fileapi::FileSystemMountPointProvider>* > > additional_providers) {} > > > > as MediaFileSystemMountPointProvider required a profile_path argument. > > 'profile' is probably not a resident of content layer, I guess we'd better use > 'partition_path' (i.e. storage partition path) or something? In the previous, profile_path was the same as partition_path. I have no problem changing the name of the argument.
On 2013/04/20 00:56:45, tommycli wrote: > On 2013/04/19 16:30:15, Greg Billock wrote: > > On 2013/04/19 01:11:45, kinuko wrote: > > > 2013/04/19 1:16 <jam@chromium.org>: > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > > > > File content/public/browser/content_browser_client.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > > > > content/public/browser/content_browser_client.h:505: virtual void > > > > GetAdditionalFileSystemMountPointProvider( > > > > why is this a ScopedVector instead of passing in a std::vector like the > > > > other methods in this class? > > > > > > I wanted to make it clear that the pointers returned by this method will be > > > owned by the caller. I could change the code with explicit comment about > > > ownership if this doesn't fit with other code here. > > > > > > > nit: GetAdditionalFileSystemMountPointProviders > > > > > > > > https://codereview.chromium.org/14317004/ > > > > Do you plan on changing the interface so that the providers (which as I > > understand them ought to basically be per-filesystem-type factories that > produce > > AsyncFileUtils for particular instances of those filesystem types) remain > owned > > by the ContentBrowserClient? That architecture would make the most sense to > me. > > It'd make the factories centrally registered, and then the per-instance > objects > > they make be owned by the filesystem api code. > > > > Ultimately, it'd be nice to encapsulate that so the filesystem api code only > > dealt with one FileSystemMountPointProvider, which then internally dealt with > > all the registered providers. That gives maximal separation between the > > filesystem api code which maps that API onto the JS world, and the > > implementation specifics needed to massage various data sources to conform to > > the API. My sense is that would better fit the overall mission of the webkit/* > > layer. > > Based on this patch I was able to move all of webkit/fileapi/media into > chrome/browser/media_galleries/fileapi. > > I had to change the signature to: > > virtual void GetAdditionalFileSystemMountPointProviders( > const base::FilePath& profile_path, > ScopedVector<fileapi::FileSystemMountPointProvider>* > additional_providers) {} > > as MediaFileSystemMountPointProvider required a profile_path argument. what does ChromeContentBrowserClient implementation look like? if the API is really keyed off the profile, it seems that this should be a method on BrowserContext instead.
On 2013/04/22 17:39:13, jam wrote: > On 2013/04/20 00:56:45, tommycli wrote: > > On 2013/04/19 16:30:15, Greg Billock wrote: > > > On 2013/04/19 01:11:45, kinuko wrote: > > > > 2013/04/19 1:16 <jam@chromium.org>: > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > > > > > File content/public/browser/content_browser_client.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/14317004/diff/5003/content/public/browser/con... > > > > > content/public/browser/content_browser_client.h:505: virtual void > > > > > GetAdditionalFileSystemMountPointProvider( > > > > > why is this a ScopedVector instead of passing in a std::vector like the > > > > > other methods in this class? > > > > > > > > I wanted to make it clear that the pointers returned by this method will > be > > > > owned by the caller. I could change the code with explicit comment about > > > > ownership if this doesn't fit with other code here. > > > > > > > > > nit: GetAdditionalFileSystemMountPointProviders > > > > > > > > > > https://codereview.chromium.org/14317004/ > > > > > > Do you plan on changing the interface so that the providers (which as I > > > understand them ought to basically be per-filesystem-type factories that > > produce > > > AsyncFileUtils for particular instances of those filesystem types) remain > > owned > > > by the ContentBrowserClient? That architecture would make the most sense to > > me. > > > It'd make the factories centrally registered, and then the per-instance > > objects > > > they make be owned by the filesystem api code. > > > > > > Ultimately, it'd be nice to encapsulate that so the filesystem api code only > > > dealt with one FileSystemMountPointProvider, which then internally dealt > with > > > all the registered providers. That gives maximal separation between the > > > filesystem api code which maps that API onto the JS world, and the > > > implementation specifics needed to massage various data sources to conform > to > > > the API. My sense is that would better fit the overall mission of the > webkit/* > > > layer. > > > > Based on this patch I was able to move all of webkit/fileapi/media into > > chrome/browser/media_galleries/fileapi. > > > > I had to change the signature to: > > > > virtual void GetAdditionalFileSystemMountPointProviders( > > const base::FilePath& profile_path, > > ScopedVector<fileapi::FileSystemMountPointProvider>* > > additional_providers) {} > > > > as MediaFileSystemMountPointProvider required a profile_path argument. > > what does ChromeContentBrowserClient implementation look like? if the API is > really keyed off the profile, it seems that this should be a method on > BrowserContext instead. Oh- having this in BrowserContext might make things simpler, but then we'll need to change the callsites of CreateFileSystem, which is called from StoragePartitionImpl::Create where we have a comment like this: "// TODO(ajwong): Break the direct dependency on |context|. We only need 3 pieces of info from it." So looks it's not expected to have more dependencies to BrowserContext in storage clients code. ChromeContentBrowserClient implementation can be found in: https://codereview.chromium.org/14247034/diff/10002/chrome/browser/chrome_con... I'm going to close this issue and just cc you (jam@) to the tommycli's new CL which subsumes this CL. Thanks! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
