Closed Bug 953425 Opened 6 years ago Closed 6 years ago

DOMFileMetadataParameters and LockedFile cleanup

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

Attachments

(4 files, 1 obsolete file)

Want to move these out of bug 860731 so I can land them before figuring out the TypeError stuff.
This also helps with my goal of killing XPConnect dictionary support.
Attachment #8351719 - Flags: review?(Jan.Varga)
This is necessary to use FileRequest in IDL.
Attachment #8355083 - Flags: review?(Jan.Varga)
Summary: DOMFileMetadataParameters cleanup → DOMFileMetadataParameters and LockedFile cleanup
Comment on attachment 8351718 [details] [diff] [review]
Part a: Store bools directly in MetadataParameters

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

Looks good
Attachment #8351718 - Flags: review?(Jan.Varga) → review+
Attachment #8351719 - Flags: review?(Jan.Varga) → review+
Comment on attachment 8355083 [details] [diff] [review]
Part c: Use DOMFileRequest in LockedFile

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

Hm, I wonder why we didn't just add DOMFileRequest::Create()
(In reply to Jan Varga [:janv] from comment #5)
> Comment on attachment 8355083 [details] [diff] [review]
> Part c: Use DOMFileRequest in LockedFile
> 
> Review of attachment 8355083 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hm, I wonder why we didn't just add DOMFileRequest::Create()

Ms2ger ?
Flags: needinfo?(Ms2ger)
No idea.
Flags: needinfo?(Ms2ger)
Comment on attachment 8355083 [details] [diff] [review]
Part c: Use DOMFileRequest in LockedFile

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

::: dom/file/LockedFile.cpp
@@ +422,4 @@
>  LockedFile::GenerateFileRequest()
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +  return FileRequest::Create(GetOwner(), this, true).downcast<DOMFileRequest>();

I'll send an additional patch to add DOMFileRequest::Create(), I don't know why we didn't add it before.

::: dom/file/LockedFile.h
@@ +23,2 @@
>  class FileHandle;
>  class FileRequest;

Nit: |class FileRequest;| is now not needed
Attachment #8355083 - Flags: review?(Jan.Varga) → review+
Attachment #8357064 - Flags: review?(Ms2ger)
Comment on attachment 8357064 [details] [diff] [review]
Part d: Add DOMFileRequest::Create()

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

LGTM. I guess I'll land it with my patches?

::: dom/file/DOMFileRequest.cpp
@@ +19,5 @@
> +// static
> +already_AddRefed<DOMFileRequest>
> +DOMFileRequest::Create(nsPIDOMWindow* aOwner, LockedFile* aLockedFile)
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Feel free to make this MOZ_ASSERT.

::: dom/file/FileHandle.cpp
@@ +148,5 @@
>      return nullptr;
>    }
>  
>    nsRefPtr<FileRequest> request =
> +    FileRequest::Create(GetOwner(), lockedFile);

Check if it fits in 80 columns now.
Attachment #8357064 - Flags: review?(Ms2ger) → review+
patch for checkin, feel free to push it
Attachment #8357064 - Attachment is obsolete: true
Attachment #8357813 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/37c564f32e26
https://hg.mozilla.org/mozilla-central/rev/4a49a222793f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Blocks: 992888
No longer blocks: 992888
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.