Closed Bug 1159401 Opened 4 years ago Closed 4 years ago

Make dom::File sane to use from c++

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: bzbarsky, Assigned: baku)

References

Details

Attachments

(1 file, 3 obsolete files)

Right now, if I have a dom::File there are some methods that are footguns to call, because they will assert if !IsFile().

We should do one of two possible things here, imo:

1)  Remove those asserts, just returns some sort of default values (empty
    strings, invalid dates, whatever).  We may want to rename the class to Blob
    in that case anyway.
2)  Have separate C++ classes for Blob and File, put the relevant methods on File
    only, have a ToFile() on Blob that asserts IsFile() and returns File* (or
    returns null if not IsFile(), or something).

Either one would be better than what we have right now, where a File in the C++ might just mean Blob.

Thoughts?
What about this: instead removing these asserts, we can move them from  the FileImpl to the dom::File class. From C++ usually it's better to use FileImpl and leave File just for the JS DOM.

What you suggest as section options, is actually FileImpl.
I think doing 1) would be the simplest.

I don't see a purpose to these assertions anyway since both Blobs and Files may or may not actually be backed by an OS-file, so whether functions like GetMozFullPath can return a "real value" is independent of if it's a File or a Blob object.
> we can move them from  the FileImpl to the dom::File class

That doesn't solve my problem, which is that I have an array of dom::File, some of which are JS File and some of which are JS Blob and the code right now assumes they're all JS File and calls these methods.

> From C++ usually it's better to use FileImpl

All the stuff around file inputs is working with an array of File.

> What you suggest as section options

I'm not sure what you mean here....
2) seems to obviously be the correct design
Assignee: nobody → amarchesini
Attached patch blob.patch (obsolete) — Splinter Review
Attachment #8599498 - Flags: review?(bzbarsky)
Attached patch blob.patch (obsolete) — Splinter Review
Attachment #8599498 - Attachment is obsolete: true
Attachment #8599498 - Flags: review?(bzbarsky)
Attachment #8599896 - Flags: review?(bzbarsky)
Comment on attachment 8599896 [details] [diff] [review]
blob.patch

> ArchiveRequest::GetFileResult(JSContext* aCx,
>+      if (!GetOrCreateDOMReflector(aCx, file, aValue)) {

Could probably use ToJSValue here too.  This comes up multiple places in the patch.

>+Blob::Create(nsISupports* aParent, const nsAString& aContentType,
>+                uint64_t aStart, uint64_t aLength)

Fix indent?

>+    nsCOMPtr<nsIDOMBlob> domBlob = this;
>+    nsCOMPtr<nsIDOMFile> domFile = do_QueryInterface(domBlob);

Should just be able to do_QueryObject(this).

But also, can we not just have a protected virtual method to answer the "am I actually a File" question instead of relying on the much slower QI?

>+Blob::ToFile()

Seems like this could up front check IsFile() and return nullptr if not, then outdent the rest of the logic.

>+Blob::GetLastModifiedDate(ErrorResult& aRv)

I'm not sure I follow why this is on Blob, not File.  It ends up asserting mIsFile, no?  And in IDL terms it's on File, not on Blob.

>+Blob::GetLastModified(ErrorResult& aRv)

Likewise.

>+File::Create(nsISupports* aParent, FileImpl* aImpl)

Are there any actual consumers that would expect this to return null?  If not, can we MOZ_CRASH if !aImpl->IsFile()?  I guess there's nsFilePickerProxy, but we could just make that check IsFile() on the FileImpl, right?

>+  already_AddRefed<File> ToFile();

This needs some documentation.

>+  already_AddRefed<File> ToFile(const nsAString& aName) const;

And this.  Esp. given how much the differ from each other!

>+  static File*
>+  Create(nsISupports* aParent, FileImpl* aImpl);

Should document the constraints on FileImpl.

>+  // File constructor should never be used directly. Use Blob instead.

Blob constructor should never be used either.  Do you mean use Blob::Create?  Or File::Create?  Or something else?

>+nsDOMFileReader::ReadAsArrayBuffer(nsIDOMBlob* aBlob, JSContext* aCx)
...
>+  nsRefPtr<Blob> blob = static_cast<Blob*>(aBlob);

So I _think_ this is a problem.

The issue is that File now multiply-inherits from nsIDOMBlob with your changes: one path via Blob and one path via nsIDOMFile.  So if aBlob is actually a File, and that's a pointer to the nsIDOMBlob version that came from nsIDOMFile (which is NOT the canonical nsIDOMBlob* for a File, of course), this cast will do the wrong thing.

We need to either QI to nsIDOMBlob in all places where we do casts like this, or get rid of the multiple inheritance by making nsIDOMFile no longer inherit from nsIDOMBlob.  I would prefer the latter, I think; it's way less surprising.  Might want a comment in the IDL explaining why the inheritance is not there, of course.

>+nsFormData::Append(const nsAString& aName, Blob& aBlob,
>+  nsRefPtr<Blob> blob = CreateNewFileInstance(aBlob, aFilename);

The whole point of CreateNewFileInstance is to return a File.  So why are we assigning it into a Blob?  We should be assigning into a File, and AddNameFilePair should take a File* for the second arg.  And then it won't need the weird ToFile and assert.  In fact, you could just leave the old code for it in the header, with aBlob renamed to aFile.

>+++ b/dom/events/DataTransfer.cpp
>+          File::Create(GetParentObject(),
>+                       static_cast<FileImpl*>(fileImpl.get()));

Why is that static_cast, or the .get() call needed?  Seems like you should be able to just pass fileImpl here.

>+++ b/dom/filesystem/FileSystemTaskBase.cpp
>+  {
>+    ErrorResult rv;
>+    aFile->GetSize(rv);
>+  }

We should either SuppressException() or assert that there is no exception or something here.

>+    aFile->GetLastModified(rv);

Likewise.

And maybe document that we're just calling these for the side-effects?  Because it looks pretty confusing as it is.

> nsFSMultipartFormData::AddNameFilePair(const nsAString& aName,
>     // Since Bug 1127150, any Blob received from FormData must be a File

Can we just leave this function taking a File*?  That will make it clearer what's going on, and when that invariant changes the compiler will tell us that we need to change this function accordingly.

Same for the other AddNameFilePair impls.

>@@ -385,18 +385,19 @@ MmsMessage::GetData(ContentParent* aPare
>+      if (rv.Failed()) {
>         NS_WARNING("Failed to get last modified date!");

rv.SuppressException()?

We should consider a followup to rename FileImpl to BlobImpl...

r=me with the above nits fixed (esp. the nsIDOM* inheritance thing).  Thank you for doing this!
Attachment #8599896 - Flags: review?(bzbarsky) → review+
> We need to either QI to nsIDOMBlob in all places where we do casts like
> this, or get rid of the multiple inheritance by making nsIDOMFile no longer
> inherit from nsIDOMBlob.  I would prefer the latter, I think; it's way less
> surprising.  Might want a comment in the IDL explaining why the inheritance
> is not there, of course.


What about if we get rid of nsIDOMFile completely? I see just Firefox OS emulator addon using nsIDOMFile. Do you think it's doable?
Flags: needinfo?(bzbarsky)
> What about if we get rid of nsIDOMFile completely?

Sounds lovely to me, esp. if we can do it as a followup instead of blocking this patch on it.
Flags: needinfo?(bzbarsky)
Blocks: 1163387
Blocks: 1163388
Attached patch patch 2 (obsolete) — Splinter Review
It seems that I don't have to include all the members of nsIDOMBlob in nsIDOMFile, but tell me if I'm wrong.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eab217e34c1b
Attachment #8603822 - Flags: review?(bzbarsky)
Comment on attachment 8603822 [details] [diff] [review]
patch 2

r=me if you mention in the comment that we want to avoid multiple inheritance so we can downcast from nsIDOMBlob to Blob safely.
Attachment #8603822 - Flags: review?(bzbarsky) → review+
The first patch here introduced some -Winconsistent-missing-override build warnings (in clang 3.6+) for nsFormSubmission.cpp -- specifically, for the class "nsFSURLEncoded" methods SupportsIsindexSubmission() & AddIsindex().

(Those methods (and their overriding status) predated this patch.  I think the warnings appeared just now because the patch marked a *different* overridding method as 'override', which made the lack-of-override-keyword on these other methods appear 'inconsistent', hence -Winconsistent-missing-override.)

Pushed a patch with rs=ehsan (granted in bug 1126447 comment 2):
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eec0168c916
And because I pushed that backout before I had realized there was a followup, here's a backout of the followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/0c80b9af0593
(When this re-lands, ideally it'd be great to have the 'override' followup just merged into the first patch here, so that every intermediate cset builds cleanly with clang > 3.6.)
Attached patch blob.patchSplinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3bf5ee35d40

single patch.
Attachment #8599896 - Attachment is obsolete: true
Attachment #8603822 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f1dd5e362984
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1168346
No longer depends on: 1168346
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.