1163388 - Get rid of nsIDOMFile

Status: RESOLVED FIXED

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch 1 - make nsIDOMFile an empty interface

Comment 2

[Andrea Marchesini [:baku]]

Attachment: patch 2 - get rid nsIDOMFile

Comment 3

[(no longer active)]

Comment on attachment 8605824 [details] [diff] [review]
patch 2 - get rid nsIDOMFile

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

Minusing because of the nsFilePickerProxy.cpp comment.

::: dom/base/nsIDOMFile.idl
@@ -58,5 @@
> -// nsIDOMBlob to Blob safely.  Our chain is:
> -//  - Blob -> nsIDOMBlob
> -//  - File -> nsIDOMFile and Blob
> -[scriptable, builtinclass, uuid(26e292a6-f5aa-4560-b523-ae22a4c7dfca)]
> -interface nsIDOMFile : nsISupports

It would be nice if you renamed this file as nsIDOMBlob.idl (perhaps in a follow-up.)

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1448,2 @@
>     */
> +  nsISupports wrapDOMFile(in nsIFile aFile);

Please rev the uuid of this interface.

::: dom/interfaces/html/nsIDOMHTMLCanvasElement.idl
@@ +37,5 @@
>    // Valid calls are
>    // mozGetAsFile(name);              -- defaults to image/png
>    // mozGetAsFile(name, type);        -- uses given type
> +  // The return value is a File object.
> +  nsISupports mozGetAsFile(in DOMString name, [optional] in DOMString type);

Same.

::: widget/nsFilePickerProxy.cpp
@@ +193,5 @@
>  
>  NS_IMETHODIMP
>  nsFilePickerProxy::GetDomfiles(nsISimpleEnumerator** aDomfiles)
>  {
> +  // TODO

Hmm, you're breaking this method, right?  I don't understand what makes that OK.  :-)

::: widget/nsIFilePicker.idl
@@ +146,4 @@
>    *
>    * @return Returns the file currently selected as File
>    */
> +  readonly attribute nsISupports domfile;

Please rev the uuid of this interface as well.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch 2 - get rid nsIDOMFile

Sorry, completely forgot about that TODO.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch 3 - make nsIDOMBlob an empty interface

Comment 6

[(no longer active)]

Comment on attachment 8605844 [details] [diff] [review]
patch 2 - get rid nsIDOMFile

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

::: widget/nsFilePickerProxy.cpp
@@ +197,5 @@
> +{
> +public:
> +  NS_DECL_ISUPPORTS
> +
> +  SimpleEnumerator(nsTArray<nsRefPtr<File>>& aFiles)

Please make this explicit.

Also shouldn't the argument be const nsTArray&?

Comment 7

[(no longer active)]

Comment on attachment 8605867 [details] [diff] [review]
patch 3 - make nsIDOMBlob an empty interface

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

r=me if the below are intentional.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +2635,5 @@
> +    if (NS_WARN_IF(rv.Failed())) {
> +      rv.SuppressException();
> +      nsCOMPtr<nsIRunnable> event =
> +        new PostErrorEvent(mRequest.forget(), POST_ERROR_EVENT_UNKNOWN);
> +      return NS_DispatchToMainThread(event);

This behavior change is intentional, right?

::: dom/filesystem/CreateFileTask.cpp
@@ +41,5 @@
>    if (aBlobData) {
>      if (FileSystemUtils::IsParentProcess()) {
> +      aBlobData->GetInternalStream(getter_AddRefs(mBlobStream), aRv);
> +      if (NS_WARN_IF(aRv.Failed())) {
> +        return;

Is this correct?  This can leave the object half-initialized.

Comment 8

[Andrea Marchesini [:baku]]

> >      if (FileSystemUtils::IsParentProcess()) {
> > +      aBlobData->GetInternalStream(getter_AddRefs(mBlobStream), aRv);
> > +      if (NS_WARN_IF(aRv.Failed())) {
> > +        return;
> 
> Is this correct?  This can leave the object half-initialized.

No, but the bug is in the current implementation. We should have a Init() method that can return errors instead having all of this code in the CTOR. I'll revert my code and file a follow up.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbee8cb95f97
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6bf6230c4a5

Comment 10

[Andrea Marchesini [:baku]]

I'm landing the last patch separately because it introduces a memory leak.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83525eb46832

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bbee8cb95f97
https://hg.mozilla.org/mozilla-central/rev/a6bf6230c4a5

Comment 12

[Andrea Marchesini [:baku]]

I'll work on the last patch in a separate bug because it's not related to nsIDOMFile.

Comment 13

[[e-mail]]

Comment on attachment 8605844 [details] [diff] [review]
patch 2 - get rid nsIDOMFile

>-  return NS_NewArrayEnumerator(aDomfiles, mDomfiles);
>+  nsRefPtr<SimpleEnumerator> enumerator = new SimpleEnumerator(mDomfiles);
Rather than rolling your own enumerator, could you not make mDomFiles an nsCOMArray<nsISupports>?

Comment 14

[Andrea Marchesini [:baku]]

> >-  return NS_NewArrayEnumerator(aDomfiles, mDomfiles);
> >+  nsRefPtr<SimpleEnumerator> enumerator = new SimpleEnumerator(mDomfiles);
> Rather than rolling your own enumerator, could you not make mDomFiles an
> nsCOMArray<nsISupports>?

More than this would be nice to have a template enumerator that take a nsTArray<nsRefPtr<T>>.
Otherwise all the code has to deal with a generic nsCOMArray<nsISupports> and this doesn't make the code readable.

Comment 15

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.