Bug 88267 – DOMFileSystem should be structure clonable so it can be the body of a web intent.

• Component: New Bugs
• Status: RESOLVED FIXED

[Steve VanDeBogart]

DOMFileSystem should be structure clonable so it can be the body of a web intent.

[Steve VanDeBogart]

Created attachment 145652 [details]
Patch

[Steve VanDeBogart]

David, it looks like you've reviewed a lot of the file system changes.  Can you review this patch?

[WebKit Review Bot]

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.

[Steve VanDeBogart]

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.

[Kentaro Hara]

(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".

[Steve VanDeBogart]

Created attachment 145655 [details]
Patch

[WebKit Review Bot]

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.

[Steve VanDeBogart]

Created attachment 145663 [details]
Patch

[Steve VanDeBogart]

Eric requested (off bug) that the clonability not be exposed (yet) except where we need it.  Updates in most recent patch.

[Kinuko Yasuda]

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?

[Greg Billock]

Created attachment 146148 [details]
Patch

[Eric U.]

> 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.

[WebKit Review Bot]

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.

[Greg Billock]

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!

[Greg Billock]

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?

[Greg Billock]

(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.

[Greg Billock]

Created attachment 146163 [details]
Patch

[Kinuko Yasuda]

(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.

[Kinuko Yasuda]

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.

[Greg Billock]

(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.

[David Levin]

btw is this in the structure clone spec? (Has it been brought up on whatwg yet?)

[Kinuko Yasuda]

(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.

[Kinuko Yasuda]

(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.

[Greg Billock]

Created attachment 146965 [details]
Patch

[Greg Billock]

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.

[David Levin]

(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?)

[Kinuko Yasuda]

(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.

[Greg Billock]

(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.

[David Levin]

(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.

[Kinuko Yasuda]

(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.

[Greg Billock]

Created attachment 147483 [details]
Patch

[Greg Billock]

(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.

[Kinuko Yasuda]

(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)

[Greg Billock]

Can I get another look? I think this is ready to go.

[Kinuko Yasuda]

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.

[Kinuko Yasuda]

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.

[Greg Billock]

Created attachment 148356 [details]
Patch

[David Levin]

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.

[Greg Billock]

Created attachment 148453 [details]
Patch

[Greg Billock]

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.

[WebKit Review Bot]

Comment on attachment 148453 [details]
Patch

Clearing flags on attachment: 148453

Committed r120776: <http://trac.webkit.org/changeset/120776>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.