Closed
Bug 1278939
Opened 9 years ago
Closed 9 years ago
DataTransfer::mozGetDataAt("application/x-moz-file", 0) returns File versus nsIFile
Categories
(Core :: DOM: Copy & Paste and Drag & Drop, defect)
Core
DOM: Copy & Paste and Drag & Drop
Tracking
()
RESOLVED
FIXED
mozilla50
| Tracking | Status | |
|---|---|---|
| firefox48 | --- | unaffected |
| firefox49 | --- | unaffected |
| firefox50 | --- | fixed |
People
(Reporter: enndeakin, Assigned: nika)
References
Details
(Keywords: regression, Whiteboard: btpp-active)
Attachments
(2 files, 2 obsolete files)
|
1.34 KB,
application/vnd.mozilla.xul+xml
|
Details | |
|
18.61 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
Bug 906420 made this return DOM File objects rather than nsIFile objects. Since existing code expects these to be nsIFiles, various code doesn't work.
One case where this doesn't work:
1. Open data:text/html,<body contenteditable> in non-e10s window
2. Drag image to the content area
Expected: the image to appear
Actual: nothing happens
|
Reporter | |
Comment 1•9 years ago
|
||
Assuming this is a regression from 906420.
|
||
Updated•9 years ago
|
status-firefox48:
--- → unaffected
status-firefox49:
--- → unaffected
status-firefox50:
--- → affected
|
Assignee | |
Comment 2•9 years ago
|
||
This also fixes some issues with the original patch related to hiding strings when dragging file data when in e10s mode, as well as preventing crashes when the clipboard changes between the data transfer being created, and used.
Attachment #8764969 -
Flags: review?(enndeakin)
|
|
Assignee | |
Updated•9 years ago
|
Assignee: nobody → michael
Flags: needinfo?(michael)
|
||
Updated•9 years ago
|
Whiteboard: btpp-active
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8764969 -
Attachment is obsolete: true
Attachment #8764969 -
Flags: review?(enndeakin)
|
|
Assignee | |
Comment 4•9 years ago
|
||
As you requested, I've rebased the patches onto master. If you want, you can also just pull the entire tree from github here: https://github.com/mystor/mozilla-central/commits/bug906420
|
|
Reporter | |
Comment 5•9 years ago
|
||
Comment on attachment 8766013 [details] [diff] [review]
Store nsIFile entries as nsIFile, but continue to produce dom::File objects from relevant APIs
> }
>- } else if (nsCOMPtr<BlobImpl> blobImpl = do_QueryInterface(aSupports)) {
>- MOZ_ASSERT(blobImpl->IsFile());
>- file = File::Create(GetParentObject(), blobImpl);
>+ MOZ_ASSERT(mFileCache, "File should be created successfully");
Should this be an assertion? Is it possible for this to fail from normal use?
>+
>+ // File cache for nsIFile application/x-moz-file entries.
>+ RefPtr<File> mFileCache;
Since this only supports one file, it would be clearer if this was called mCachedFile (and the comment updated).
I get an assertion on the testcase I am about to attach:
Assertion failure: oldKind == Kind() (Kind should not have changed after setting data), at /builds/moz2/working/dom/events/DataTransferItem.cpp:223
#01: mozilla::dom::DataTransferItem::FillInExternalData()[/builds/moz2/working/output/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3a98775]
Otherwise I did some extensive testing and don't see other issues, (although I did find some unrelated to this patch).
Attachment #8766013 -
Flags: review?(enndeakin) → review-
|
|
Reporter | |
Comment 6•9 years ago
|
||
Cut and paste between the two areas caused the assertion mentioned in the previous comment. Needs to be privileged to cause this.
|
|
Assignee | |
Comment 7•9 years ago
|
||
(In reply to Neil Deakin from comment #6)
> Created attachment 8766432 [details]
> dragorclip-simple.xul
>
> Cut and paste between the two areas caused the assertion mentioned in the
> previous comment. Needs to be privileged to cause this.
I believe that the problem in this case is that you are trying to store an nsIFile on a clipboard dataTransfer.
This means that we put the nsIFile into a transferrable, which gets stored, and tell the OS that we have some data of type x-moz-file.
When we paste, we look at the MIME type, and see an x-moz-file, which we think means that we have a file, so our type predictor adds an DataTransferItem of type FILE. Later, when we try to get the actual data from the OS, the OS calls back to us. We extract the nsIFile from the transfer, and then try to get out some primitive data from it, with nsPrimitiveHelpers::CreateDataFromPrimitive, which I believe fails, and thus the data never gets sent back. This means that we didn't get a file, so the type changes.
If you think this should not be an assertion, I'm fine with that, but I'm not sure we can make it work with nsIFiles easily. What do you think?
Flags: needinfo?(enndeakin)
|
|
Reporter | |
Comment 8•9 years ago
|
||
Shouldn't the data from GetTransferData be null in that case?
Flags: needinfo?(enndeakin)
|
|
Assignee | |
Comment 9•9 years ago
|
||
This patch no longer fails when we get data of an unexpected type from the OS, and also renames mFileCache to mCachedFile.
Attachment #8767707 -
Flags: review?(enndeakin)
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8766013 -
Attachment is obsolete: true
|
|
Reporter | |
Updated•9 years ago
|
Attachment #8767707 -
Flags: review?(enndeakin) → review+
|
||
Comment 10•9 years ago
|
||
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d0aef4586bd
Store nsIFile entries as nsIFile, but continue to produce dom::File objects from relevant APIs, r=enndeakin
|
||
Comment 11•9 years ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
|
||
Updated•9 years ago
|
Version: unspecified → Trunk
You need to log in
before you can comment on or make changes to this bug.
Description
•