DOMFileSystem should be structure clonable so it can be the body of a web intent.
Created attachment 145652 [details] Patch
David, it looks like you've reviewed a lot of the file system changes. Can you review this patch?
Attachment 145652 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/v8/SerializedScriptValue.cpp:59: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
It looks like the style bot has a false positive, or do we really want headers in ASCII order? I couldn't find an existing bug for it.
(In reply to comment #4) > It looks like the style bot has a false positive, or do we really want headers in ASCII order? I couldn't find an existing bug for it. WebKit expects the ASCII order. "V8DOMFileSystem.h" should be before "V8DataView.h".
Created attachment 145655 [details] Patch
Attachment 145655 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 145663 [details] Patch
Eric requested (off bug) that the clonability not be exposed (yet) except where we need it. Updates in most recent patch.
Comment on attachment 145663 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145663&action=review > Source/WebCore/ChangeLog:7 > + Could you add an (elaborated) comment about how this should or should NOT work and why we are adding this? > Source/WebCore/Modules/filesystem/DOMFileSystemBase.h:78 > + void makeClonable() { m_clonable = true; } Please comment what this flag does and what the default value is. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:191 > + DOMFileSystemTag = 'd', // type:int32_t, name:WebCoreString, url:WebCoreString -> FileSystem (ref) Please surround filesystem related feature with ifdef ENABLE(FILE_SYSTEM) > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1772 > + RefPtr<DOMFileSystem> fs = DOMFileSystem::create(getScriptExecutionContext(), name, static_cast<WebCore::FileSystemType>(type), KURL(ParsedURLString, url), PlatformSupport::createAsyncFileSystem()); PlatformSupport::createAsyncFileSystem() is only defined and available for chromium port. Please add this code only for PLATFORM(CHROMIUM), or it might be nicer to have a new specialized DOMFileSystem::create method for deserialization case and implement it in DOMFileSystemChromium.cpp. Another question is do we want to make the file system cloneable if it's made by deserialization?
Created attachment 146148 [details] Patch
> Another question is do we want to make the file system cloneable if it's made by deserialization? Given that we only have a single use case that we want cloneable [respond with cloned FS from a web intent], probably not. It's not that we want specific filesystems that are cloneable--it's that we want some filesystems to be sendable via web intent, but not otherwise cloned. I'm not sure if there's a more specific way to apply that restriction, though.
Attachment 146148 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/v8/SerializedScriptValue.cpp:59: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Hi, reviewers, Steve is on parental leave (congrats!) and asked me to take over. This latest patch is me applying the above in my client. Responding to comments now. Thanks!
Comment on attachment 145663 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145663&action=review >> Source/WebCore/ChangeLog:7 >> + > > Could you add an (elaborated) comment about how this should or should NOT work and why we are adding this? Added some more explanation here. >> Source/WebCore/Modules/filesystem/DOMFileSystemBase.h:78 >> + void makeClonable() { m_clonable = true; } > > Please comment what this flag does and what the default value is. Added. >> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:191 >> + DOMFileSystemTag = 'd', // type:int32_t, name:WebCoreString, url:WebCoreString -> FileSystem (ref) > > Please surround filesystem related feature with ifdef ENABLE(FILE_SYSTEM) Done. I notice that the IntArrayTag below is also 'd'. Is that a problem? Or is that a sub-type that won't conflict? >> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1772 >> + RefPtr<DOMFileSystem> fs = DOMFileSystem::create(getScriptExecutionContext(), name, static_cast<WebCore::FileSystemType>(type), KURL(ParsedURLString, url), PlatformSupport::createAsyncFileSystem()); > > PlatformSupport::createAsyncFileSystem() is only defined and available for chromium port. Please add this code only for PLATFORM(CHROMIUM), or it might be nicer to have a new specialized DOMFileSystem::create method for deserialization case and implement it in DOMFileSystemChromium.cpp. > > Another question is do we want to make the file system cloneable if it's made by deserialization? I think we just want single-shot serialization for this use case. PLATFORM(CHROMIUM) is an option, but would it be better to add this method to PlatformSupport.h on ports, or as you say, to DOMFileSystem itself?
(In reply to comment #12) > > Another question is do we want to make the file system cloneable if it's made by deserialization? > > Given that we only have a single use case that we want cloneable [respond with cloned FS from a web intent], probably not. It's not that we want specific filesystems that are cloneable--it's that we want some filesystems to be sendable via web intent, but not otherwise cloned. I'm not sure if there's a more specific way to apply that restriction, though. I think makeClonable is about as tight as we can make it -- it'll require case-specific and explicit marking by internal code to make the object serializable.
Created attachment 146163 [details] Patch
(In reply to comment #12) > PLATFORM(CHROMIUM) is an option, but would it be better to add this method to PlatformSupport.h on ports, or as you say, to DOMFileSystem itself? I prefer having a constructor for deserialization (like File::create does), though PLATFORM(CHROMIUM) would be probably ok (I searched for other callsites of PlatformSupport::foo under v8/ and they seem to be just guarded with PLATFORM(CHROMIUM)). PlatformSupport.h is not a common header across ports (in my understanding) so adding the method to the header doesn't sound really right to me. > (In reply to comment #12) > > > Another question is do we want to make the file system cloneable if it's made by deserialization? > > > > Given that we only have a single use case that we want cloneable [respond with cloned FS from a web intent], probably not. It's not that we want specific filesystems that are cloneable--it's that we want some filesystems to be sendable via web intent, but not otherwise cloned. I'm not sure if there's a more specific way to apply that restriction, though. > > I think makeClonable is about as tight as we can make it -- it'll require case-specific and explicit marking by internal code to make the object serializable. Ok, then let's just leave it.
Comment on attachment 146163 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146163&action=review > Source/WebCore/ChangeLog:13 > + Could we add a test which makes sure it's not serializable in regular cases? Otherwise please add a comment why this patch doesn't have tests.
(In reply to comment #18) > (In reply to comment #12) > > PLATFORM(CHROMIUM) is an option, but would it be better to add this method to PlatformSupport.h on ports, or as you say, to DOMFileSystem itself? > > I prefer having a constructor for deserialization (like File::create does), though PLATFORM(CHROMIUM) would be probably ok (I searched for other callsites of PlatformSupport::foo under v8/ and they seem to be just guarded with PLATFORM(CHROMIUM)). > > PlatformSupport.h is not a common header across ports (in my understanding) so adding the method to the header doesn't sound really right to me. Snooping around, it looks like the right thing to do here is call AsyncFileSystem::create(), which then makes the platform-specific version. I see implementations for gtk and blackberry ports. It looks like the chromium port needs to conform to that API. I can see what it'll take to do that. > > > (In reply to comment #12) > > > > Another question is do we want to make the file system cloneable if it's made by deserialization? > > > > > > Given that we only have a single use case that we want cloneable [respond with cloned FS from a web intent], probably not. It's not that we want specific filesystems that are cloneable--it's that we want some filesystems to be sendable via web intent, but not otherwise cloned. I'm not sure if there's a more specific way to apply that restriction, though. > > > > I think makeClonable is about as tight as we can make it -- it'll require case-specific and explicit marking by internal code to make the object serializable. > > Ok, then let's just leave it.
btw is this in the structure clone spec? (Has it been brought up on whatwg yet?)
(In reply to comment #21) > btw is this in the structure clone spec? (Has it been brought up on whatwg yet?) Not yet, though we plan to do so eventually. This patch isn't adding fully functional structured cloning, but just implementing primitive serialize/deserialize for experimental use case (in our case for Web intents), which is disabled by default unless setClonable() is explicitly called.
(In reply to comment #20) > (In reply to comment #18) > > (In reply to comment #12) > > > PLATFORM(CHROMIUM) is an option, but would it be better to add this method to PlatformSupport.h on ports, or as you say, to DOMFileSystem itself? > > > > I prefer having a constructor for deserialization (like File::create does), though PLATFORM(CHROMIUM) would be probably ok (I searched for other callsites of PlatformSupport::foo under v8/ and they seem to be just guarded with PLATFORM(CHROMIUM)). > > > > PlatformSupport.h is not a common header across ports (in my understanding) so adding the method to the header doesn't sound really right to me. > > Snooping around, it looks like the right thing to do here is call AsyncFileSystem::create(), which then makes the platform-specific version. I see implementations for gtk and blackberry ports. It looks like the chromium port needs to conform to that API. I can see what it'll take to do that. Ah that's true, we used to need a specialized constructor which takes more arguments for chromium but it's no longer the case... we can directly define and create AsyncFileSystem::create now. > > > > > (In reply to comment #12) > > > > > Another question is do we want to make the file system cloneable if it's made by deserialization? > > > > > > > > Given that we only have a single use case that we want cloneable [respond with cloned FS from a web intent], probably not. It's not that we want specific filesystems that are cloneable--it's that we want some filesystems to be sendable via web intent, but not otherwise cloned. I'm not sure if there's a more specific way to apply that restriction, though. > > > > > > I think makeClonable is about as tight as we can make it -- it'll require case-specific and explicit marking by internal code to make the object serializable. > > > > Ok, then let's just leave it.
Created attachment 146965 [details] Patch
Done now (wasn't too hard at all :-)) Also added the test. (In reply to comment #23) > (In reply to comment #20) > > (In reply to comment #18) > > > (In reply to comment #12) > > > > PLATFORM(CHROMIUM) is an option, but would it be better to add this method to PlatformSupport.h on ports, or as you say, to DOMFileSystem itself? > > > > > > I prefer having a constructor for deserialization (like File::create does), though PLATFORM(CHROMIUM) would be probably ok (I searched for other callsites of PlatformSupport::foo under v8/ and they seem to be just guarded with PLATFORM(CHROMIUM)). > > > > > > PlatformSupport.h is not a common header across ports (in my understanding) so adding the method to the header doesn't sound really right to me. > > > > Snooping around, it looks like the right thing to do here is call AsyncFileSystem::create(), which then makes the platform-specific version. I see implementations for gtk and blackberry ports. It looks like the chromium port needs to conform to that API. I can see what it'll take to do that. > > Ah that's true, we used to need a specialized constructor which takes more arguments for chromium but it's no longer the case... we can directly define and create AsyncFileSystem::create now. > > > > > > > > (In reply to comment #12) > > > > > > Another question is do we want to make the file system cloneable if it's made by deserialization? > > > > > > > > > > Given that we only have a single use case that we want cloneable [respond with cloned FS from a web intent], probably not. It's not that we want specific filesystems that are cloneable--it's that we want some filesystems to be sendable via web intent, but not otherwise cloned. I'm not sure if there's a more specific way to apply that restriction, though. > > > > > > > > I think makeClonable is about as tight as we can make it -- it'll require case-specific and explicit marking by internal code to make the object serializable. > > > > > > Ok, then let's just leave it.
(In reply to comment #22) > (In reply to comment #21) > > btw is this in the structure clone spec? (Has it been brought up on whatwg yet?) > > Not yet, though we plan to do so eventually. This patch isn't adding fully functional structured cloning, but just implementing primitive serialize/deserialize for experimental use case (in our case for Web intents), which is disabled by default unless setClonable() is explicitly called. This seems very confusing. Sometimes DOMFileSystem is cloneable and sometimes it isn't. And if you get one that had been cloned but try to send it in a message, then it won't be cloneable again because the cloning process doesn't set that flag again. (I'm feel a little slow for not understand this, so hopefully you'll help me understand the situation better.) Does it mean to make sense to make DOMFileSystem cloneable with respect to Web Workers? If so, why not just proposal it in that context alone? (If not, how does this interact with Web Workers?)
(In reply to comment #26) > (In reply to comment #22) > > (In reply to comment #21) > > > btw is this in the structure clone spec? (Has it been brought up on whatwg yet?) > > > > Not yet, though we plan to do so eventually. This patch isn't adding fully functional structured cloning, but just implementing primitive serialize/deserialize for experimental use case (in our case for Web intents), which is disabled by default unless setClonable() is explicitly called. > > This seems very confusing. Sometimes DOMFileSystem is cloneable and sometimes it isn't. And if you get one that had been cloned but try to send it in a message, then it won't be cloneable again because the cloning process doesn't set that flag again. > (I'm feel a little slow for not understand this, so hopefully you'll help me understand the situation better.) > Does it mean to make sense to make DOMFileSystem cloneable with respect to Web Workers? If so, why not just proposal it in that context alone? (If not, how does this interact with Web Workers?) They are very reasonable questions, as I had the same. In my understanding this patch wouldn't make sending or cloning a file system work at all in js code. It must be always disabled until we make a valid proposal. As for how the flag is being used I think Steve or Greg can explain better.
(In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #22) > > > (In reply to comment #21) > > > > btw is this in the structure clone spec? (Has it been brought up on whatwg yet?) > > > > > > Not yet, though we plan to do so eventually. This patch isn't adding fully functional structured cloning, but just implementing primitive serialize/deserialize for experimental use case (in our case for Web intents), which is disabled by default unless setClonable() is explicitly called. > > > > This seems very confusing. Sometimes DOMFileSystem is cloneable and sometimes it isn't. And if you get one that had been cloned but try to send it in a message, then it won't be cloneable again because the cloning process doesn't set that flag again. > > (I'm feel a little slow for not understand this, so hopefully you'll help me understand the situation better.) > > Does it mean to make sense to make DOMFileSystem cloneable with respect to Web Workers? If so, why not just proposal it in that context alone? (If not, how does this interact with Web Workers?) > > They are very reasonable questions, as I had the same. In my understanding this patch wouldn't make sending or cloning a file system work at all in js code. It must be always disabled until we make a valid proposal. > As for how the flag is being used I think Steve or Greg can explain better. Agreed. These are good questions. As the test shows, these objects won't be cloneable when created through JS. They'll need to be specially empowered for serializability through browser intervention. The immediate use case for this is being able to pass the filesystem through a web intents payload, which is received as a structured clone input. This also allows the objects to be passed through a browser-initiated message event, which also seems really convenient.
(In reply to comment #28) > (In reply to comment #27) > > (In reply to comment #26) > > > (In reply to comment #22) > > > > (In reply to comment #21) > > > > > btw is this in the structure clone spec? (Has it been brought up on whatwg yet?) > > > > > > > > Not yet, though we plan to do so eventually. This patch isn't adding fully functional structured cloning, but just implementing primitive serialize/deserialize for experimental use case (in our case for Web intents), which is disabled by default unless setClonable() is explicitly called. > > > > > > This seems very confusing. Sometimes DOMFileSystem is cloneable and sometimes it isn't. And if you get one that had been cloned but try to send it in a message, then it won't be cloneable again because the cloning process doesn't set that flag again. > > > (I'm feel a little slow for not understand this, so hopefully you'll help me understand the situation better.) > > > Does it mean to make sense to make DOMFileSystem cloneable with respect to Web Workers? If so, why not just proposal it in that context alone? (If not, how does this interact with Web Workers?) > > > > They are very reasonable questions, as I had the same. In my understanding this patch wouldn't make sending or cloning a file system work at all in js code. It must be always disabled until we make a valid proposal. > > As for how the flag is being used I think Steve or Greg can explain better. > > Agreed. These are good questions. As the test shows, these objects won't be cloneable when created through JS. They'll need to be specially empowered for serializability through browser intervention. > > The immediate use case for this is being able to pass the filesystem through a web intents payload, which is received as a structured clone input. This also allows the objects to be passed through a browser-initiated message event, which also seems really convenient. hm. I guess this is ok.
(In reply to comment #25) > Done now (wasn't too hard at all :-)) > > Also added the test. The test looks good to me, please update LayoutTests/ChangeLog as well and also add the test to Skipped/TestExpectations for the platforms that don't support file system API.
Created attachment 147483 [details] Patch
(In reply to comment #30) > (In reply to comment #25) > > Done now (wasn't too hard at all :-)) > > > > Also added the test. > > The test looks good to me, please update LayoutTests/ChangeLog as well and also add the test to Skipped/TestExpectations for the platforms that don't support file system API. I checked the expectations files -- It looks like they already have categorical fast/filesystem exceptions. Added the changelog.
(In reply to comment #32) > (In reply to comment #30) > > (In reply to comment #25) > > > Done now (wasn't too hard at all :-)) > > > > > > Also added the test. > > > > The test looks good to me, please update LayoutTests/ChangeLog as well and also add the test to Skipped/TestExpectations for the platforms that don't support file system API. > > I checked the expectations files -- It looks like they already have categorical fast/filesystem exceptions. Added the changelog. (In reply to comment #32) > (In reply to comment #30) > > (In reply to comment #25) > > > Done now (wasn't too hard at all :-)) > > > > > > Also added the test. > > > > The test looks good to me, please update LayoutTests/ChangeLog as well and also add the test to Skipped/TestExpectations for the platforms that don't support file system API. > > I checked the expectations files -- It looks like they already have categorical fast/filesystem exceptions. Added the changelog. Thanks! The change looks good to me (if it looks good to real reviewers)
Can I get another look? I think this is ready to go.
Comment on attachment 147483 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147483&action=review > Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:98 > +} I've added this code in another patch (I needed it for other change) so you may want to remove it before landing or cq may fail.
David, would you be able to take another look? (In reply to comment #35) > (From update of attachment 147483 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147483&action=review > > > Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:98 > > +} > > I've added this code in another patch (I needed it for other change) so you may want to remove it before landing or cq may fail.
Created attachment 148356 [details] Patch
Comment on attachment 148356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148356&action=review > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:34 > +#include "AsyncFileSystem.h" This include appears misplaced (of course so are both of the <wtf/... includes) > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:58 > +#if ENABLE(FILE_SYSTEM) if shouldn't be intermingled with other include's. It should be below all of them.
Created attachment 148453 [details] Patch
Comment on attachment 148356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148356&action=review >> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:34 >> +#include "AsyncFileSystem.h" > > This include appears misplaced (of course so are both of the <wtf/... includes) The linter was complaining to me about this line until I moved it here. Is the real error that the wtf includes go lower? I'll move them and see if it is happy. >> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:58 >> +#if ENABLE(FILE_SYSTEM) > > if shouldn't be intermingled with other include's. It should be below all of them. Done.
Comment on attachment 148453 [details] Patch Clearing flags on attachment: 148453 Committed r120776: <http://trac.webkit.org/changeset/120776>
All reviewed patches have been landed. Closing bug.