Implement Blob.close().

Implement Blob.close().

This adds support for the close() methods to Blobs,

 http://dev.w3.org/2006/webapi/FileAPI/#close-method

providing a way to explicitly release the underlying data of a Blob.

Subsequent uses of a closed Blob is required/assumed to have the
following behavior:

  - the size of a closed Blob should be zero.
  - Consumers of such a Blob should treat it as having
    size 0 (e.g., FormData.append(), XHR.send()), and not throw.
  - FileReader read operations that are underway on a Blob
    being close()d should not be impacted.
  - Subsequent FileReader read operations on a closed Blob
    should fail with InvalidStateError.
  - Slices of a closed Blob should still be usable.
  - Dereferencing a blob URL that has been closed should
    cause a network error, like a revoke.

In other words, closing a shared object is not a straightforward
matter.

R=kinuko@chromium.org,michaeln@chromium.org, jochen@chromium.org
BUG=157794
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167874

[sof, 2014-02-07 14:11:53]

When you have a spare moment, please take a look.(If there are others even
better placed to have a look, please adjust or suggest other reviewers.)

I think it's time to support this Blob API method. >=IE10 ships a prefixed
version (msClose()), but no one provides close().

[arv (Not doing code reviews), 2014-02-07 16:56:30]

Maybe send an intent to ship email about this?

[jbroman, 2014-02-07 17:24:07]

On 2014/02/07 14:11:53, sof wrote:
> When you have a spare moment, please take a look.(If there are others even
> better placed to have a look, please adjust or suggest other reviewers.)
> 
> I think it's time to support this Blob API method. >=IE10 ships a prefixed
> version (msClose()), but no one provides close().

I don't know this code well enough to properly review it, sorry.

As arv says, this should go through the launch process before landing.
http://www.chromium.org/blink#launch-process

[sof, 2014-02-07 17:38:28]

On 2014/02/07 17:24:07, jbroman wrote:
> On 2014/02/07 14:11:53, sof wrote:
> > When you have a spare moment, please take a look.(If there are others even
> > better placed to have a look, please adjust or suggest other reviewers.)
> > 
> > I think it's time to support this Blob API method. >=IE10 ships a prefixed
> > version (msClose()), but no one provides close().
> 
> I don't know this code well enough to properly review it, sorry.
> 

Thanks, just good to know.

> As arv says, this should go through the launch process before landing.
> http://www.chromium.org/blink#launch-process

Certainly, I'll send that out tonight or over the weekend.

[sof, 2014-02-07 19:35:11]

On 2014/02/07 16:56:30, arv wrote:
> Maybe send an intent to ship email about this?

Good idea;
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/jcrEI_jfYFs

[michaeln, 2014-02-11 21:03:23]

i can help review this too

[kinuko, 2014-02-12 06:11:19]

(Haven't looked into all details yet, the behavior of readers other than
FileReader concerns me a bit too)

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/Blob...
File Source/core/fileapi/Blob.idl (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/Blob...
Source/core/fileapi/Blob.idl:41: [CallWith=ExecutionContext] void close();
Depending on the spec and intent-to-ship status, probably you can start adding
this behind a flag

https://codereview.chromium.org/157363003/diff/50001/Source/core/html/PublicU...
File Source/core/html/PublicURLManager.cpp (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/html/PublicU...
Source/core/html/PublicURLManager.cpp:86: }
It might be slightly more readable if we just maintain a separate map from uuid
to URLs?

[sof, 2014-02-12 06:56:46]

https://codereview.chromium.org/157363003/diff/50001/Source/core/html/PublicU...
File Source/core/html/PublicURLManager.cpp (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/html/PublicU...
Source/core/html/PublicURLManager.cpp:86: }
On 2014/02/12 06:11:20, kinuko wrote:
> It might be slightly more readable if we just maintain a separate map from
uuid
> to URLs?

It would have to be UUID to a set of URLs; would that be cleaner (and worth the
space for what is arguably a rarer thing to do, i.e., revoke a URL by UUID)?

[kinuko, 2014-02-12 12:51:59]

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/File...
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/File...
Source/core/fileapi/File.cpp:233: m_path = String();
There're more fields in this class (e.g. m_name)-- should we also clear them?

https://codereview.chromium.org/157363003/diff/50001/Source/core/html/PublicU...
File Source/core/html/PublicURLManager.cpp (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/html/PublicU...
Source/core/html/PublicURLManager.cpp:86: }
I guessed in general this map'd be relatively small so I wouldn't worry too much
about it, but well having a separate map may make the other revoke() code a bit
complex instead?

This could be preferential, but could we do following for better readability:
- assign i->key and i->value to temp reference variables before nested loop
- remove if (removableKeys.size()) clause on line 82 as it doesn't help much
- I'd probably rename removableKeys to toBeRemoved or something more direct

[sof, 2014-02-12 12:53:45]

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/File...
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/File...
Source/core/fileapi/File.cpp:233: m_path = String();
On 2014/02/12 12:52:00, kinuko wrote:
> There're more fields in this class (e.g. m_name)-- should we also clear them?

The name of the Blob isn't currently spec'ed as going away when closed.

[kinuko, 2014-02-12 13:26:24]

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/File...
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/File...
Source/core/fileapi/File.cpp:233: m_path = String();
On 2014/02/12 12:53:46, sof wrote:
> On 2014/02/12 12:52:00, kinuko wrote:
> > There're more fields in this class (e.g. m_name)-- should we also clear
them?
> 
> The name of the Blob isn't currently spec'ed as going away when closed.

I see thanks. Could we clear m_fileSystemURL too then as it could be internally
used in a similar way as m_path?  (Also having a short comment about why we
don't clear m_name here might help avoiding similar questions)

[michaeln, 2014-02-12 20:33:04]

https://codereview.chromium.org/157363003/diff/50001/Source/core/dom/DOMURL.cpp
File Source/core/dom/DOMURL.cpp (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/dom/DOMURL.c...
Source/core/dom/DOMURL.cpp:73: String DOMURL::createPublicURL(ExecutionContext*
executionContext, URLRegistrable* registrable, const String& uuid)
Not sure we need the addition of the uuid? The balancing call to revoke all for
a registrable object could use the URLRegistrable* value as the key. Not all
registerable object types have seperate string uuids associated with them, if
the ptr value can serve as the key, they too can be revoked.

Any reason to not use the ptr value as the key? I guess internal data structs
should view the key as a void* since URLRegistrations can outlive the Blob
object.

API'wise, it might be nice to provide a balancing method to revoke all for a
registerable defined on this DOMURL class since this is where they're created.

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/Blob...
File Source/core/fileapi/Blob.cpp (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/Blob...
Source/core/fileapi/Blob.cpp:131: m_blobDataHandle =
BlobDataHandle::create(blobData.release(), 0);
nice and safe :)

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/File...
File Source/core/fileapi/FileReader.cpp (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/File...
Source/core/fileapi/FileReader.cpp:223: if (blob->hasBeenClosed()) {
The big question is where else do we have to make this test? I see there are w3c
bugs open. Behind the experimental flag until we have answers to that might be
the way to go.

[sof, 2014-02-12 22:28:26]

Thanks; sorry for not following up yet, been busy on other fronts today.

Will get to it tomorrow; some comments for now.

https://codereview.chromium.org/157363003/diff/50001/Source/core/dom/DOMURL.cpp
File Source/core/dom/DOMURL.cpp (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/dom/DOMURL.c...
Source/core/dom/DOMURL.cpp:73: String DOMURL::createPublicURL(ExecutionContext*
executionContext, URLRegistrable* registrable, const String& uuid)
On 2014/02/12 20:33:04, michaeln wrote:
> Not sure we need the addition of the uuid? The balancing call to revoke all
for
> a registrable object could use the URLRegistrable* value as the key. Not all
> registerable object types have seperate string uuids associated with them, if
> the ptr value can serve as the key, they too can be revoked.
> 
> Any reason to not use the ptr value as the key? I guess internal data structs
> should view the key as a void* since URLRegistrations can outlive the Blob
> object.
> 
> API'wise, it might be nice to provide a balancing method to revoke all for a
> registerable defined on this DOMURL class since this is where they're created.

I like the idea of using the URLRegistrable* as the unique value, will have to
think about it overnight.

There are some issues though -- when the Blobs become objects on the Oilpan
heap, you suddenly have to contend with references to GCed objects. And you have
to retire/disable a URLRegistrable* once it becomes garbage..as you don't want
to run the risk of having false matches when you do close() a Blob & pass along
its pointer value. i.e., the same pointer value could have belonged to an
earlier Blob...and you're not allowed to revoke URLs upon their associated Blob
being garbage collected.

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/File...
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/File...
Source/core/fileapi/File.cpp:233: m_path = String();
On 2014/02/12 13:26:25, kinuko wrote:
> On 2014/02/12 12:53:46, sof wrote:
> > On 2014/02/12 12:52:00, kinuko wrote:
> > > There're more fields in this class (e.g. m_name)-- should we also clear
> them?
> > 
> > The name of the Blob isn't currently spec'ed as going away when closed.
> 
> I see thanks. Could we clear m_fileSystemURL too then as it could be
internally
> used in a similar way as m_path?  (Also having a short comment about why we
> don't clear m_name here might help avoiding similar questions)

Yes, better do both.

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/File...
File Source/core/fileapi/FileReader.cpp (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/File...
Source/core/fileapi/FileReader.cpp:223: if (blob->hasBeenClosed()) {
On 2014/02/12 20:33:04, michaeln wrote:
> The big question is where else do we have to make this test? I see there are
w3c
> bugs open. Behind the experimental flag until we have answers to that might be
> the way to go.

Yes, that is a question still on the table. I don't see anyone arguing for eager
exception reporting over closed Blobs in other contexts than here (e.g.,
XHR.send(Blob)), but the spec might still change in this regard.

[michaeln, 2014-02-12 22:34:08]

https://codereview.chromium.org/157363003/diff/50001/Source/core/dom/DOMURL.cpp
File Source/core/dom/DOMURL.cpp (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/dom/DOMURL.c...
Source/core/dom/DOMURL.cpp:73: String DOMURL::createPublicURL(ExecutionContext*
executionContext, URLRegistrable* registrable, const String& uuid)
> There are some issues though -- when the Blobs become objects on the Oilpan
> heap, you suddenly have to contend with references to GCed objects. And you
have
> to retire/disable a URLRegistrable* once it becomes garbage..as you don't want
> to run the risk of having false matches when you do close() a Blob & pass
along
> its pointer value. i.e., the same pointer value could have belonged to an
> earlier Blob...and you're not allowed to revoke URLs upon their associated
Blob
> being garbage collected.

Right, that's sounds like a very good reason to not use the ptr value as the
key.

[sof, 2014-02-13 09:15:09]

On 2014/02/12 22:34:08, michaeln wrote:
>
https://codereview.chromium.org/157363003/diff/50001/Source/core/dom/DOMURL.cpp
> File Source/core/dom/DOMURL.cpp (right):
> 
>
https://codereview.chromium.org/157363003/diff/50001/Source/core/dom/DOMURL.c...
> Source/core/dom/DOMURL.cpp:73: String
DOMURL::createPublicURL(ExecutionContext*
> executionContext, URLRegistrable* registrable, const String& uuid)
> > There are some issues though -- when the Blobs become objects on the Oilpan
> > heap, you suddenly have to contend with references to GCed objects. And you
> have
> > to retire/disable a URLRegistrable* once it becomes garbage..as you don't
want
> > to run the risk of having false matches when you do close() a Blob & pass
> along
> > its pointer value. i.e., the same pointer value could have belonged to an
> > earlier Blob...and you're not allowed to revoke URLs upon their associated
> Blob
> > being garbage collected.
> 
> Right, that's sounds like a very good reason to not use the ptr value as the
> key.

ok, let's not pursue that here.

I've added a revoke method over UUIDs to DOMURL to address the issue about
asymmetry. Switched Blob.close() to use it instead, tidier that way.

[sof, 2014-02-13 09:59:56]

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/Blob...
File Source/core/fileapi/Blob.idl (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/Blob...
Source/core/fileapi/Blob.idl:41: [CallWith=ExecutionContext] void close();
On 2014/02/12 06:11:20, kinuko wrote:
> Depending on the spec and intent-to-ship status, probably you can start adding
> this behind a flag

Alright, done.

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/File...
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/fileapi/File...
Source/core/fileapi/File.cpp:233: m_path = String();
On 2014/02/12 22:28:26, sof wrote:
> On 2014/02/12 13:26:25, kinuko wrote:
> > On 2014/02/12 12:53:46, sof wrote:
> > > On 2014/02/12 12:52:00, kinuko wrote:
> > > > There're more fields in this class (e.g. m_name)-- should we also clear
> > them?
> > > 
> > > The name of the Blob isn't currently spec'ed as going away when closed.
> > 
> > I see thanks. Could we clear m_fileSystemURL too then as it could be
> internally
> > used in a similar way as m_path?  (Also having a short comment about why we
> > don't clear m_name here might help avoiding similar questions)
> 
> Yes, better do both.

Done now.

https://codereview.chromium.org/157363003/diff/50001/Source/core/html/PublicU...
File Source/core/html/PublicURLManager.cpp (right):

https://codereview.chromium.org/157363003/diff/50001/Source/core/html/PublicU...
Source/core/html/PublicURLManager.cpp:86: }
On 2014/02/12 12:52:00, kinuko wrote:
> I guessed in general this map'd be relatively small so I wouldn't worry too
much
> about it, but well having a separate map may make the other revoke() code a
bit
> complex instead?
> 
> This could be preferential, but could we do following for better readability:
> - assign i->key and i->value to temp reference variables before nested loop
> - remove if (removableKeys.size()) clause on line 82 as it doesn't help much
> - I'd probably rename removableKeys to toBeRemoved or something more direct

Thanks, I've tried to improve readability - please take another look?

Also noticed that this revocation method didn't retire the URLs from the
memory cache(like revoking by URL does), so added that also.

[sof, 2014-02-18 18:43:58]

If there's more to attend to here, please let me know.

[michaeln, 2014-02-18 18:59:53]

https://codereview.chromium.org/157363003/diff/470001/Source/core/fileapi/Blo...
File Source/core/fileapi/Blob.cpp (right):

https://codereview.chromium.org/157363003/diff/470001/Source/core/fileapi/Blo...
Source/core/fileapi/Blob.cpp:128: // read operations which will throw.
Can you add a TODO (or is it FIXME in blink) comment here about how the behavior
of other blob consumers is not yet specified and include a link to a crbug
(which links to the w3c bug).

https://codereview.chromium.org/157363003/diff/470001/Source/core/fileapi/Fil...
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/157363003/diff/470001/Source/core/fileapi/Fil...
Source/core/fileapi/File.cpp:218: if (!getFileMetadata(m_path, metadata)) {
Is getFileMetadata() safe to call with an empty path, or maybe this is
unreachable for those that have been closed? The method is in the public section
which makes me wonder.

https://codereview.chromium.org/157363003/diff/470001/Source/core/fileapi/File.h
File Source/core/fileapi/File.h (right):

https://codereview.chromium.org/157363003/diff/470001/Source/core/fileapi/Fil...
Source/core/fileapi/File.h:79: KURL fileSystemURL() const {
ASSERT(m_hasBackingFile); return m_fileSystemURL; }
That assertion looks like a landmine in a world with blobs that get closed.

https://codereview.chromium.org/157363003/diff/470001/Source/core/fileapi/Fil...
Source/core/fileapi/File.h:98: const String& path() const {
ASSERT(m_hasBackingFile); return m_path; }
Ditto landmind.

[sof, 2014-02-18 19:12:47]

https://codereview.chromium.org/157363003/diff/470001/Source/core/fileapi/File.h
File Source/core/fileapi/File.h (right):

https://codereview.chromium.org/157363003/diff/470001/Source/core/fileapi/Fil...
Source/core/fileapi/File.h:79: KURL fileSystemURL() const {
ASSERT(m_hasBackingFile); return m_fileSystemURL; }
On 2014/02/18 18:59:54, michaeln wrote:
> That assertion looks like a landmine in a world with blobs that get closed.

Yes, I'll see what I can do. The landmine is already present with being able to
create File objects from blobBits using the File constructor:

  var noBackingFile = new File(["foo"], {..});

[michaeln, 2014-02-19 01:39:24]

> Yes, I'll see what I can do. The landmine is already present with being able
to
> create File objects from blobBits using the File constructor:
> 
>   var noBackingFile = new File(["foo"], {..});

Do we know why is that assertion there? What depends on those values being not
empty? I wish those assertions were at the callsites that need these values to
not be empty rather then in the getters :(

[kinuko, 2014-02-19 07:10:50]

On 2014/02/19 01:39:24, michaeln wrote:
> > Yes, I'll see what I can do. The landmine is already present with being able
> to
> > create File objects from blobBits using the File constructor:
> > 
> >   var noBackingFile = new File(["foo"], {..});
> 
> Do we know why is that assertion there? What depends on those values being not
> empty? I wish those assertions were at the callsites that need these values to
> not be empty rather then in the getters :(

In most callsites we check hasBackingFile() before referring path() but not
always (I thought we always do so but codesearch said no). One easiest fix would
be to make it return "" if hasBackingFile is false?

[sof, 2014-02-19 11:47:38]

https://codereview.chromium.org/157363003/diff/470001/Source/core/fileapi/Blo...
File Source/core/fileapi/Blob.cpp (right):

https://codereview.chromium.org/157363003/diff/470001/Source/core/fileapi/Blo...
Source/core/fileapi/Blob.cpp:128: // read operations which will throw.
On 2014/02/18 18:59:54, michaeln wrote:
> Can you add a TODO (or is it FIXME in blink) comment here about how the
behavior
> of other blob consumers is not yet specified and include a link to a crbug
> (which links to the w3c bug).

Good idea, done + created http://crbug.com/344820

https://codereview.chromium.org/157363003/diff/470001/Source/core/fileapi/Fil...
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/157363003/diff/470001/Source/core/fileapi/Fil...
Source/core/fileapi/File.cpp:218: if (!getFileMetadata(m_path, metadata)) {
On 2014/02/18 18:59:54, michaeln wrote:
> Is getFileMetadata() safe to call with an empty path, or maybe this is
> unreachable for those that have been closed? The method is in the public
section
> which makes me wonder.
> 

It would be relying on whatever behavior that method provides for empty paths,
it's reachable. I've made this (and other) calls conditional on there being a
backing file; please check if that looks better/safer.

https://codereview.chromium.org/157363003/diff/470001/Source/core/fileapi/File.h
File Source/core/fileapi/File.h (right):

https://codereview.chromium.org/157363003/diff/470001/Source/core/fileapi/Fil...
Source/core/fileapi/File.h:98: const String& path() const {
ASSERT(m_hasBackingFile); return m_path; }
On 2014/02/18 18:59:54, michaeln wrote:
> Ditto landmind.

I checked the call sites & they do check backing file status, so this looks like
a defensive measure should that slip.

I've kept the assert, but tweaked it slightly.

[sof, 2014-02-19 11:53:08]

On 2014/02/19 07:10:50, kinuko wrote:
> On 2014/02/19 01:39:24, michaeln wrote:
> > > Yes, I'll see what I can do. The landmine is already present with being
able
> > to
> > > create File objects from blobBits using the File constructor:
> > > 
> > >   var noBackingFile = new File(["foo"], {..});
> > 
> > Do we know why is that assertion there? What depends on those values being
not
> > empty? I wish those assertions were at the callsites that need these values
to
> > not be empty rather then in the getters :(
> 
> In most callsites we check hasBackingFile() before referring path() but not
> always (I thought we always do so but codesearch said no). One easiest fix
would
> be to make it return "" if hasBackingFile is false?

And it would be reasonable to have it do so too, I think.

It is/was already the case that it would return an empty string in the
non-backing file case. I've tried to make the assert more precise&clear.

[michaeln, 2014-02-20 21:50:20]

i haven't look closely at the tests, but the cpp changes do lgtm now

[kinuko, 2014-02-21 12:26:16]

Sorry for slow review, this almost looks good. I have a few more questions..

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
File LayoutTests/fast/files/blob-close-read.html (right):

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-close-read.html:20: blob = buildBlob(blobContents);
nit: now that it's very easy to create a blob by Blob ctor there seem to be few
reason to use this (old) helper buildBlob method...

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-close-read.html:34: sliced = blob.slice(2);
nit: can we verify shouldBe("sliced.size", "14") before close too?

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
File LayoutTests/fast/files/blob-close-revoke.html (right):

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-close-revoke.html:45: // Make them all inaccessible.
We don't seem checking if all of them become inaccessible?  (This subtest itself
looks good, btw)

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-close-revoke.html:58: shouldBeTrue("blobURL2.length
> 0");
I've briefly checked the spec but I'm fully not sure what'd be the expected
behavior for creating object URL for a closed Blob... is this what spec says or
based on your interpretation / assumption?
http://dev.w3.org/2006/webapi/FileAPI/#dfn-close

[sof, 2014-02-21 12:36:45]

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
File LayoutTests/fast/files/blob-close-revoke.html (right):

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-close-revoke.html:58: shouldBeTrue("blobURL2.length
> 0");
On 2014/02/21 12:26:17, kinuko wrote:
> I've briefly checked the spec but I'm fully not sure what'd be the expected
> behavior for creating object URL for a closed Blob... is this what spec says
or
> based on your interpretation / assumption?
> http://dev.w3.org/2006/webapi/FileAPI/#dfn-close

That's correct, not much to go on there yet. The behavior is based on what I
thought made sense + what was confirmed in
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24576#c3  

(That bug is linked to from http://crbug.com/344820 )

[kinuko, 2014-02-21 14:11:17]

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
File LayoutTests/fast/files/blob-close-revoke.html (right):

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-close-revoke.html:58: shouldBeTrue("blobURL2.length
> 0");
On 2014/02/21 12:36:45, sof wrote:
> On 2014/02/21 12:26:17, kinuko wrote:
> > I've briefly checked the spec but I'm fully not sure what'd be the expected
> > behavior for creating object URL for a closed Blob... is this what spec says
> or
> > based on your interpretation / assumption?
> > http://dev.w3.org/2006/webapi/FileAPI/#dfn-close
> 
> That's correct, not much to go on there yet. The behavior is based on what I
> thought made sense + what was confirmed in
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=24576#c3  
> (That bug is linked to from http://crbug.com/344820 )

I see, thanks for the pointers. So let me make sure... line 58 verifies it
returns a URL that appears valid (vs. returning an empty URL which indicates
invalid blob) but reading the URL should fail, and the latter is not tested in
this test.  Is that right?   What does line 57 verify?  (I can understand the
line should pass in our implementation but it feels unclear if it needs to be
satisfied in a test)

[sof, 2014-02-21 14:56:56]

Thanks very much, updated the tests based on your feedback.

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
File LayoutTests/fast/files/blob-close-read.html (right):

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-close-read.html:20: blob = buildBlob(blobContents);
On 2014/02/21 12:26:17, kinuko wrote:
> nit: now that it's very easy to create a blob by Blob ctor there seem to be
few
> reason to use this (old) helper buildBlob method...

ok, switched over to create it directly instead. (Was just trying remain
consistent with other tests.)

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-close-read.html:34: sliced = blob.slice(2);
On 2014/02/21 12:26:17, kinuko wrote:
> nit: can we verify shouldBe("sliced.size", "14") before close too?

Done.

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
File LayoutTests/fast/files/blob-close-revoke.html (right):

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-close-revoke.html:45: // Make them all inaccessible.
On 2014/02/21 12:26:17, kinuko wrote:
> We don't seem checking if all of them become inaccessible?  (This subtest
itself
> looks good, btw)

Good idea; I've kept this test over 'img', but created another one over Workers
that check that all become inaccessible after close().

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-close-revoke.html:58: shouldBeTrue("blobURL2.length
> 0");
On 2014/02/21 14:11:18, kinuko wrote:
> On 2014/02/21 12:36:45, sof wrote:
> > On 2014/02/21 12:26:17, kinuko wrote:
> > > I've briefly checked the spec but I'm fully not sure what'd be the
expected
> > > behavior for creating object URL for a closed Blob... is this what spec
says
> > or
> > > based on your interpretation / assumption?
> > > http://dev.w3.org/2006/webapi/FileAPI/#dfn-close
> > 
> > That's correct, not much to go on there yet. The behavior is based on what I
> > thought made sense + what was confirmed in
> > https://www.w3.org/Bugs/Public/show_bug.cgi?id=24576#c3  
> > (That bug is linked to from http://crbug.com/344820 )
> 
> I see, thanks for the pointers. So let me make sure... line 58 verifies it
> returns a URL that appears valid (vs. returning an empty URL which indicates
> invalid blob) but reading the URL should fail, and the latter is not tested in
> this test.  Is that right?   What does line 57 verify?  (I can understand the
> line should pass in our implementation but it feels unclear if it needs to be
> satisfied in a test)

Correct, this only tests re-registration after close (see the referred to bug
for motivation on why this is expected to be legal, and not throw or something
else.)

Line 57 checks that you can create a new URL (and a different one) on a closed
Blob. Line 58 is a continuation of that, checking that it is not
null/undefined/empty string. It was the best/only thing I could think of to
check for a valid, but otherwise opaque Blob URL string.

Does that clarify purpose?

[kinuko, 2014-02-21 15:40:48]

Thanks, lgtm (modulo a few optional nits)

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
File LayoutTests/fast/files/blob-close-revoke.html (right):

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-close-revoke.html:58: shouldBeTrue("blobURL2.length
> 0");
On 2014/02/21 14:56:57, sof wrote:
> On 2014/02/21 14:11:18, kinuko wrote:
> > On 2014/02/21 12:36:45, sof wrote:
> > > On 2014/02/21 12:26:17, kinuko wrote:
> > > > I've briefly checked the spec but I'm fully not sure what'd be the
> expected
> > > > behavior for creating object URL for a closed Blob... is this what spec
> says
> > > or
> > > > based on your interpretation / assumption?
> > > > http://dev.w3.org/2006/webapi/FileAPI/#dfn-close
> > > 
> > > That's correct, not much to go on there yet. The behavior is based on what
I
> > > thought made sense + what was confirmed in
> > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=24576#c3  
> > > (That bug is linked to from http://crbug.com/344820 )
> > 
> > I see, thanks for the pointers. So let me make sure... line 58 verifies it
> > returns a URL that appears valid (vs. returning an empty URL which indicates
> > invalid blob) but reading the URL should fail, and the latter is not tested
in
> > this test.  Is that right?   What does line 57 verify?  (I can understand
the
> > line should pass in our implementation but it feels unclear if it needs to
be
> > satisfied in a test)
> 
> Correct, this only tests re-registration after close (see the referred to bug
> for motivation on why this is expected to be legal, and not throw or something
> else.)
> 
> Line 57 checks that you can create a new URL (and a different one) on a closed
> Blob. Line 58 is a continuation of that, checking that it is not
> null/undefined/empty string. It was the best/only thing I could think of to
> check for a valid, but otherwise opaque Blob URL string.
> 
> Does that clarify purpose?

Yes thanks. Could we add a brief comment for these two lines then? (Like:
"Verifies createObjectURL() still works and returns valid (non-empty) URL
without throwing an error until the caller dereferences it.", and maybe a link
to the bug)

https://codereview.chromium.org/157363003/diff/880001/LayoutTests/fast/files/...
File LayoutTests/fast/files/blob-close-revoke.html (right):

https://codereview.chromium.org/157363003/diff/880001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-close-revoke.html:51: // Now check that URLs have
become now inaccessible.
(optional) Really a nit-picky comment, but having this comment in a failure
(unexpected) handler feels a bit weird.
Also since this is for initial event handler, rearranging these handlers in a
following order may look more natural:

onNonClosedMessage()
onNonClosedError()
onClosedMessage()
onClosedError()

[sof, 2014-02-21 16:02:43]

Updated with another patchset.

The previous set wasn't liked by the trybots, suddenly showing the new tests
here as failing, while they pass locally. Had some upload trouble with that set,
so re-trying with this one instead.

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
File LayoutTests/fast/files/blob-close-revoke.html (right):

https://codereview.chromium.org/157363003/diff/710001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-close-revoke.html:58: shouldBeTrue("blobURL2.length
> 0");
On 2014/02/21 15:40:49, kinuko wrote:
> On 2014/02/21 14:56:57, sof wrote:
> > On 2014/02/21 14:11:18, kinuko wrote:
> > > On 2014/02/21 12:36:45, sof wrote:
> > > > On 2014/02/21 12:26:17, kinuko wrote:
> > > > > I've briefly checked the spec but I'm fully not sure what'd be the
> > expected
> > > > > behavior for creating object URL for a closed Blob... is this what
spec
> > says
> > > > or
> > > > > based on your interpretation / assumption?
> > > > > http://dev.w3.org/2006/webapi/FileAPI/#dfn-close
> > > > 
> > > > That's correct, not much to go on there yet. The behavior is based on
what
> I
> > > > thought made sense + what was confirmed in
> > > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=24576#c3  
> > > > (That bug is linked to from http://crbug.com/344820 )
> > > 
> > > I see, thanks for the pointers. So let me make sure... line 58 verifies it
> > > returns a URL that appears valid (vs. returning an empty URL which
indicates
> > > invalid blob) but reading the URL should fail, and the latter is not
tested
> in
> > > this test.  Is that right?   What does line 57 verify?  (I can understand
> the
> > > line should pass in our implementation but it feels unclear if it needs to
> be
> > > satisfied in a test)
> > 
> > Correct, this only tests re-registration after close (see the referred to
bug
> > for motivation on why this is expected to be legal, and not throw or
something
> > else.)
> > 
> > Line 57 checks that you can create a new URL (and a different one) on a
closed
> > Blob. Line 58 is a continuation of that, checking that it is not
> > null/undefined/empty string. It was the best/only thing I could think of to
> > check for a valid, but otherwise opaque Blob URL string.
> > 
> > Does that clarify purpose?
> 
> Yes thanks. Could we add a brief comment for these two lines then? (Like:
> "Verifies createObjectURL() still works and returns valid (non-empty) URL
> without throwing an error until the caller dereferences it.", and maybe a link
> to the bug)

Done.

https://codereview.chromium.org/157363003/diff/880001/LayoutTests/fast/files/...
File LayoutTests/fast/files/blob-close-revoke.html (right):

https://codereview.chromium.org/157363003/diff/880001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-close-revoke.html:51: // Now check that URLs have
become now inaccessible.
On 2014/02/21 15:40:49, kinuko wrote:
> (optional) Really a nit-picky comment, but having this comment in a failure
> (unexpected) handler feels a bit weird.
> Also since this is for initial event handler, rearranging these handlers in a
> following order may look more natural:
> 
> onNonClosedMessage()
> onNonClosedError()
> onClosedMessage()
> onClosedError()

Done that + added a comment above them explaining the test pattern (check that
it initially works to load a Blob URL, then not after calling close().)

[sof, 2014-02-21 16:06:38]

jochen: adding a new experimental feature for this method; ok to do so?

[jochen (gone - plz use gerrit), 2014-02-24 11:57:25]

can you please reupload?

[sof, 2014-02-24 12:33:37]

On 2014/02/24 11:57:25, jochen wrote:
> can you please reupload?

Sorry; done, rebasing at the same time.

(current presubmit handling of trybot runs prevents me testing this fully.)

[jochen (gone - plz use gerrit), 2014-02-25 19:31:07]

lgtm

[sof, 2014-02-25 21:58:40]

Thanks everyone; let's give this one a go.

[sof, 2014-02-25 21:58:49]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-02-25 21:59:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/157363003/1130001

[commit-bot: I haz the power, 2014-02-25 23:09:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/157363003/1130001

[commit-bot: I haz the power, 2014-02-25 23:27:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/157363003/1130001

[Paweł Hajdan Jr., 2014-02-26 05:45:28]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-26 06:01:21]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 06:06:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/157363003/1130001

[commit-bot: I haz the power, 2014-02-26 09:14:24]

Change committed as 167874