669433 - Clean up File implementations

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Currently the various classes that we use to implement Files are a bit of a mess. One problem is that the base class used by all File implementations were written for OS-file-backed Files. This causes subclasses that use other storage mechanisms to override the base class in awkward ways.

Another problem is that the base class uses a single bool to represent two concepts: Blob vs. File and backed-by-whole-file vs. backed-by-part-of-file.

Patches coming up which creates a dedicated base class which provides the base functionality to implement Blob and File. Subclasses can then inherit that an implement their own storage strategies.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 1: Create nsDOMFileBase

This is somewhat messy since the current nsDOMFile class gets spread out over two classes: nsDOMFileBase and nsDOMFileFile.

I tried to avoid reshuffling functions in this patch as to make it easier to read the actual changes. Separate patch coming up which moves functions so they are grouped with their class.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 2: Move functions to be grouped with their class

This just moves functions around into a more logical order. No code changes.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 3: Make first argument to Blob.slice optional

To match spec and Array.slice/String.slice

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 544033 [details] [diff] [review]
Part 1: Create nsDOMFileBase

Review of attachment 544033 [details] [diff] [review]:
-----------------------------------------------------------------

Overall comments:

- Add a method to nsDOMFileBase called IsSizeUnknown (or whatever) that checks mSize == PR_UINT64_MAX so we don't have to do that manually in a bunch of places.
- Move the constructor/CreateSlice implementations out of nsDOMFile.h and into the .cpp, and add comments for what each constructor is used for on the ones that are missing.

Other than that, it looks great.  I assume it passes tests too ;-)

::: content/base/public/nsDOMFile.h
@@ +120,2 @@
>  
> +class nsDOMFileFile : public nsDOMFileBase,

I'm not thrilled by the name, but I don't have any better ideas.

@@ +184,5 @@
> +  CreateSlice(PRUint64 aStart, PRUint64 aLength, const nsAString& aContentType)
> +  {
> +    nsDOMFileFile* t = new nsDOMFileFile(this, aStart, aLength, aContentType);
> +    NS_ADDREF(t);
> +    return t;

Use an nsCOMPtr here for consistency (I'm actually surprised this works at all).

@@ +191,2 @@
>    nsCOMPtr<nsIFile> mFile;
> +  bool mWholeFile;

I don't understand in what situations mWholeFile and mIsFile have different values ....

::: content/base/src/nsDOMFile.cpp
@@ +226,4 @@
>  {
> +  if (mContentType.IsVoid()) {
> +    NS_ASSERTION(mWholeFile,
> +                 "Should only use lazy ContentType when using the whole file");

Is this really true?  What happens if I pass in a null content type for slice?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 544036 [details] [diff] [review]
Part 2: Move functions to be grouped with their class

/me assumes Jonas can copy/paste correctly.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 544037 [details] [diff] [review]
Part 3: Make first argument to Blob.slice optional

Review of attachment 544037 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: content/base/public/nsIDOMFile.idl
@@ +57,5 @@
>    // The caller is responsible for releasing the internalUrl from the
>    // moz-filedata: protocol handler
>    [noscript] DOMString getInternalUrl(in nsIPrincipal principal);
>  
> +  [optional_argc] nsIDOMBlob mozSlice([optional] in long long start,

Hmm, I suppose we don't need to change the IID for this since this is binary compatible?

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

If you're going to rename stuff, may I suggest namespaces?

(In reply to comment #6)
> Comment on attachment 544037 [details] [diff] [review] [review]
> Hmm, I suppose we don't need to change the IID for this since this is binary
> compatible?

Surely it isn't, we're changing the number of arguments

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #7)
> If you're going to rename stuff, may I suggest namespaces?

I could go either way here.  Jonas?

> (In reply to comment #6)
> > Comment on attachment 544037 [details] [diff] [review] [review] [review]
> > Hmm, I suppose we don't need to change the IID for this since this is binary
> > compatible?
> 
> Surely it isn't, we're changing the number of arguments

No, we're not ...

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> - Add a method to nsDOMFileBase called IsSizeUnknown (or whatever) that
> checks mSize == PR_UINT64_MAX so we don't have to do that manually in a
> bunch of places.

Done

> - Move the constructor/CreateSlice implementations out of nsDOMFile.h and
> into the .cpp, and add comments for what each constructor is used for on the
> ones that are missing.

Move CreateSlize but not the ctors. The ctors do actually inline which is nice, and they also serve as documentation which is really needed here.

> @@ +184,5 @@
> > +  CreateSlice(PRUint64 aStart, PRUint64 aLength, const nsAString& aContentType)
> > +  {
> > +    nsDOMFileFile* t = new nsDOMFileFile(this, aStart, aLength, aContentType);
> > +    NS_ADDREF(t);
> > +    return t;
> 
> Use an nsCOMPtr here for consistency (I'm actually surprised this works at
> all).

Not sure why you think that wouldn't work, but I've changed it.

> @@ +191,2 @@
> >    nsCOMPtr<nsIFile> mFile;
> > +  bool mWholeFile;
> 
> I don't understand in what situations mWholeFile and mIsFile have different
> values ....

For example for the blobs returned by XHR. And likely also in the future once we store Blobs in IndexedDB.

> ::: content/base/src/nsDOMFile.cpp
> @@ +226,4 @@
> >  {
> > +  if (mContentType.IsVoid()) {
> > +    NS_ASSERTION(mWholeFile,
> > +                 "Should only use lazy ContentType when using the whole file");
> 
> Is this really true?  What happens if I pass in a null content type for
> slice?

Then we set mContentType to the empty string. This is intentional for other reasons too since we want .type returning "" and not null.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/df4de9835c78
http://hg.mozilla.org/integration/mozilla-inbound/rev/3265ea092f0f
http://hg.mozilla.org/integration/mozilla-inbound/rev/ab1f4bc0b70a

Comment 11

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/df4de9835c78
http://hg.mozilla.org/mozilla-central/rev/3265ea092f0f
http://hg.mozilla.org/mozilla-central/rev/ab1f4bc0b70a