Last Comment Bug 669433 - Clean up File implementations
: Clean up File implementations
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Jonas Sicking (:sicking)
:
Mentors:
Depends on:
Blocks: 669437
  Show dependency treegraph
 
Reported: 2011-07-05 12:51 PDT by Jonas Sicking (:sicking)
Modified: 2011-07-12 03:55 PDT (History)
4 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Create nsDOMFileBase (32.60 KB, patch)
2011-07-05 13:00 PDT, Jonas Sicking (:sicking)
khuey: review+
Details | Diff | Review
Part 2: Move functions to be grouped with their class (6.26 KB, patch)
2011-07-05 13:03 PDT, Jonas Sicking (:sicking)
khuey: review+
Details | Diff | Review
Part 3: Make first argument to Blob.slice optional (5.25 KB, patch)
2011-07-05 13:04 PDT, Jonas Sicking (:sicking)
khuey: review+
Details | Diff | Review

Description Jonas Sicking (:sicking) 2011-07-05 12:51:55 PDT
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) 2011-07-05 13:00:12 PDT
Created attachment 544033 [details] [diff] [review]
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) 2011-07-05 13:03:00 PDT
Created attachment 544036 [details] [diff] [review]
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) 2011-07-05 13:04:05 PDT
Created attachment 544037 [details] [diff] [review]
Part 3: Make first argument to Blob.slice optional

To match spec and Array.slice/String.slice
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-07-07 09:53:08 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-07-07 09:53:34 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-07-07 09:56:17 PDT
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 2011-07-07 10:06:59 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-07-07 10:32:15 PDT
(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) 2011-07-11 17:18:46 PDT
> - 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.

Note You need to log in before you can comment on or make changes to this bug.