Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[DnD] dataTransfer.items undefined in Firefox (implement DataTransferItem and DataTransferItemList)

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM: Events
RESOLVED FIXED
4 years ago
5 months ago

People

(Reporter: yugang, Assigned: mystor)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla50
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(URL)

Attachments

(5 attachments, 16 obsolete attachments)

1.27 KB, text/html
Details
11.72 KB, text/plain
Details
107.79 KB, patch
Details | Diff | Splinter Review
11.91 KB, patch
Details | Diff | Splinter Review
24.75 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
Created attachment 791811 [details]
DnD_drag_files_length_tests.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.95 Safari/537.36

Steps to reproduce:

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0

Steps to reproduce:

Drag and Drop Spec Info: http://www.w3.org/TR/2011/WD-html5-20110525/dnd.html#the-datatransfer-interface

- Test case: See attachment


Actual results:

Always report undefined error when using "dataTransfer.items"


Expected results:

dataTransfer.item should be defined in Firefox

Updated

4 years ago
Attachment #791811 - Attachment mime type: text/plain → text/html

Comment 1

4 years ago
I don't see this error in the console, only this message when I'm loading your testcase:
ReferenceError: setup is not defined @ https://bug906420.bugzilla.mozilla.org/attachment.cgi?id=791811:21
Component: Untriaged → DOM: Events
Product: Firefox → Core

Comment 2

3 years ago
According to http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/events/nsIDOMDataTransfer.idl mozilla does not have the "items" property

Checking on the browser console yields similar results:

new DataTransfer("", "").items -> undefined
inspecting DataTransfer.prototype -> no items property

.items seems to be a newer version of the API, supporting the transfer of multiple objects out of the box (instead of the non-standard .mozSetDataAt methods) and also supports adding File objects.

DataTransferItem and DataTransferItemList also aren't implemented.

Updated

3 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 23 Branch → Trunk
The WG spec for this is here: http://www.whatwg.org/specs/web-apps/current-work/multipage/interaction.html#the-datatransferitemlist-interface it looks like it's not implemented yet.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

3 years ago
Blocks: 803014
Created attachment 8513216 [details] [diff] [review]
WIP

This patch should pass tests but needs a lot of cleanup and some serious tests. I'll try to post a polished patch by the end of next week.
Blocks: 876480
Blocks: 1093989
(Assignee)

Comment 5

2 years ago
I'm going to take this over and try to fix it up along with bug 891247
Assignee: nobody → michael
(Assignee)

Comment 6

2 years ago
Created attachment 8652868 [details] [diff] [review]
Part 1: Implement DataTransferItem and DataTransferItemList

This is a reworking of the patch to implement DataTransferItem and DataTransferItemList. I haven't written tests for the new functionality yet, but this patch passes try (when applied on top of bug 891247): https://treeherder.mozilla.org/#/jobs?repo=try&revision=f483a03290e7

The interactions between the new DataTransfer.items property and the Moz* APIs exposed on DataTransfer are kept as close as possible to the interactions of the original APIs. The items list will only display and permit you to interact with the items stored in index 0, or files stored in any index. Files which are added are added to a new index. 

Open Questions / incomplete:

* Modifying the files attached to the DataTransfer currently incorrectly doesn't update the .files property of the DataTransfer. These two lists should be kept in sync.

* I don't really like the mechanism for handling images on the clipboard right now. I feel like we should hold File objects directly in the data store rather than converting the objects to files when needed. This shouldn't be too hard to do (I would think), so I'll probably try it either here or in a follow-up patch.

(I'm sure that there are other things which I can't think about right now)
Attachment #8513216 - Attachment is obsolete: true
(Assignee)

Comment 7

2 years ago
Created attachment 8654291 [details] [diff] [review]
Implement DataTransferItem and DataTransferItemList

This patch implements DataTransferItem and DataTransferItemList in our DataTransfer model. It keeps the (as far as I am aware) full semantics of our Moz* APIs on DataTransfer functional, and adds the ability to use the items property.

There are a bunch of ugly hacks in this patch which exist in order to make our Moz* API work on the same backing data as the .items API, as the spec assumes an incompatible store implementation to our Moz* API's datastore.

This is a mega-patch. It also fixes bug 891247. I could split that part of the bug out, if I am asked to, and move it over to that bug. It'll just be slightly annoying to do, so I haven't bothered.

I haven't had a chance to push it to try recently, and try is closed right now, so there are probably bugs which that will detect which I haven't found yet.
Attachment #8652868 - Attachment is obsolete: true
Attachment #8654291 - Flags: review?(amarchesini)
Keywords: dev-doc-needed
Comment on attachment 8654291 [details] [diff] [review]
Implement DataTransferItem and DataTransferItemList

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

looks good but there are a few small details here and there. Can I see it again with these comments applied?
And sorry for the delay!

::: dom/base/nsCopySupport.cpp
@@ +683,5 @@
>    bool doDefault = true;
>    nsRefPtr<DataTransfer> clipboardData;
>    if (chromeShell || Preferences::GetBool("dom.event.clipboardevents.enabled", true)) {
>      clipboardData =
> +		new DataTransfer(doc->GetScopeObject(), aType, aType == NS_PASTE, 

extra space.

@@ +684,5 @@
>    nsRefPtr<DataTransfer> clipboardData;
>    if (chromeShell || Preferences::GetBool("dom.event.clipboardevents.enabled", true)) {
>      clipboardData =
> +		new DataTransfer(doc->GetScopeObject(), aType, aType == NS_PASTE, 
> +				aClipboardType);

better indentation

::: dom/events/DataTransfer.cpp
@@ +30,5 @@
>  #include "mozilla/dom/Element.h"
>  #include "mozilla/dom/FileList.h"
>  #include "mozilla/dom/BindingUtils.h"
>  #include "mozilla/dom/OSFileSystem.h"
> +#include "mozilla/dom/DataTransferItemList.h"

usually we try an alphabetic ordering

@@ +132,5 @@
>  {
>    MOZ_ASSERT(mParent);
> +
> +  // We clone the items array after everything else, so that it has a valid mParent value
> +  mItems = aItems->Clone(this);

MOZ_ASSERT(aItems) before using it.

@@ +281,5 @@
>  DataTransfer::GetFiles(nsIDOMFileList** aFileList)
>  {
>    ErrorResult rv;
> +  nsRefPtr<FileList> files = GetFiles(rv);
> +  files.forget(aFileList);

NS_WARN_IF(NS_FAILED(rv));

@@ +293,5 @@
> +
> +  const nsTArray<nsRefPtr<DataTransferItem> >* items = mItems->MozItemsAt(0);
> +  if (items) {
> +    for (uint32_t i = 0; i < items->Length(); i++) {
> +      if (DataTransferItem* item = items->ElementAt(i)) {

can it actually be null? What about:

DataTransferItem* item = items->ElementAt(i);
MOZ_ASSERT(item);

...

@@ +296,5 @@
> +    for (uint32_t i = 0; i < items->Length(); i++) {
> +      if (DataTransferItem* item = items->ElementAt(i)) {
> +        nsAutoString type;
> +        item->GetType(type);
> +        types->Add(type);

This can fail.

@@ +300,5 @@
> +        types->Add(type);
> +
> +        // If we have any files, we need to also add the "Files" type!
> +        if (item->Kind() == DataTransferItem::KIND_FILE) {
> +          types->Add(NS_LITERAL_STRING("Files"));

This can fail.

@@ +501,2 @@
>      // note that you can retrieve the types regardless of their principal
> +    const nsTArray<nsRefPtr<DataTransferItem> >& items = *mItems->MozItemsAt(aIndex);

no space between '>' and '>'

@@ +503,5 @@
> +
> +    for (uint32_t i = 0; i < items.Length(); i++) {
> +      nsAutoString type;
> +      items[i]->GetType(type);
> +      types->Add(type);

this can fail.

@@ +567,5 @@
> +  if (item->Principal() && principal && !principal->Subsumes(item->Principal())) {
> +    return NS_ERROR_DOM_SECURITY_ERR;
> +  }
> +
> +  nsCOMPtr<nsIVariant> data = item->Data();

MOZ_ASSERT(data);

@@ +787,5 @@
>      return nullptr;
>    }
>  
>    nsRefPtr<Promise> p = Promise::Create(global, aRv);
>    if (aRv.Failed()) {

NS_WARN_IF

@@ +792,5 @@
>      return nullptr;
>    }
>  
> +  nsRefPtr<FileList> files = mItems->Files();
> +  if (!files) {

NS_WARN_IF

@@ +798,5 @@
>    }
>  
>    Sequence<OwningFileOrDirectory> filesAndDirsSeq;
>  
> +  if (!filesAndDirsSeq.SetLength(files->Length(), mozilla::fallible_t())) {

NS_WARN_IF

@@ +920,4 @@
>      return nullptr;
>    }
>  
> +  const nsTArray<nsRefPtr<DataTransferItem> >& item = *mItems->MozItemsAt(aIndex);

no extra space between >>

@@ +949,5 @@
>  
>      // the underlying drag code uses text/unicode, so use that instead of text/plain
>      const char* format;
> +    nsAutoString type;
> +    formatitem->GetType(type);

can this fail?

@@ +1049,5 @@
>  void
>  DataTransfer::ClearAll()
>  {
> +  ErrorResult rv;
> +  mItems->Clear(rv);

NS_WARN_IF(rv.Failed());

@@ +1161,5 @@
>    ssm->GetSystemPrincipal(getter_AddRefs(sysPrincipal));
>  
>    // there isn't a way to get a list of the formats that might be available on
>    // all platforms, so just check for the types that can actually be imported
> +  const char* formats[] = { kFileMime, kHTMLMime, kURLMime, kURLDataMime,

can we avoid to duplicate this formats?

::: dom/events/DataTransfer.h
@@ +36,5 @@
>  class Element;
>  class FileList;
>  template<typename T> class Optional;
>  
> +class DataTransferItem;

alphabetic order. Move these 2 lines before line 35.

@@ +42,2 @@
>  
>  #define NS_DATATRANSFER_IID \

Change this UUID.

@@ +139,5 @@
>  
>    already_AddRefed<Promise> GetFilesAndDirectories(ErrorResult& aRv);
>  
>    void AddElement(Element& aElement, mozilla::ErrorResult& aRv);
> +  uint32_t MozItemCount();

uint32_t MozItemCount() const;

@@ +171,5 @@
>    }
>  
> +  DataTransferItemList* Items() const { return mItems; }
> +
> +  bool IsReadOnly() { return mReadOnly; }

const

@@ +176,3 @@
>    void SetReadOnly() { mReadOnly = true; }
>  
> +  int32_t ClipboardType() { return mClipboardType; }

const

@@ +176,4 @@
>    void SetReadOnly() { mReadOnly = true; }
>  
> +  int32_t ClipboardType() { return mClipboardType; }
> +  uint32_t EventType() { return mEventType; }

const

::: dom/events/DataTransferItem.cpp
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/EventForwards.h"
> +#include "mozilla/ContentEvents.h"

alphabetic order for all these headers.

@@ +10,5 @@
> +#include "nsISupportsPrimitives.h"
> +#include "nsNetUtil.h"
> +#include "nsQueryObject.h"
> +
> +#include "DataTransferItem.h"

move these 2 on top.

@@ +16,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(DataTransferItem, mData, mPrincipal, mParent)

80chars.

@@ +26,5 @@
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END
> +
> +JSObject*
> +DataTransferItem::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)

80chars here and in the rest of the file.

@@ +33,5 @@
> +}
> +
> +already_AddRefed<DataTransferItem>
> +DataTransferItem::Clone(DataTransferItemList* aParent)
> +{

MOZ_ASSERT(aParent) ?

@@ +48,5 @@
> +}
> +
> +/* static */ already_AddRefed<File>
> +DataTransferItem::FileFromISupports(nsISupports* aSupports)
> +{

MOZ_ASSERT(aSupports);

@@ +71,5 @@
> +DataTransferItem::SetData(nsIVariant* aData)
> +{
> +  if (!aData) {
> +    // We are holding a temporary null which will later be filled.
> +    // These are provided by the system, and have guaranteed properties about their

80chars here and in the rest of the file.

@@ +77,5 @@
> +    MOZ_ASSERT(!mType.IsEmpty());
> +    if (mType == NS_LITERAL_STRING(kFileMime) ||
> +        mType == NS_LITERAL_STRING(kPNGImageMime) ||
> +        mType == NS_LITERAL_STRING(kJPEGImageMime) ||
> +        mType == NS_LITERAL_STRING(kGIFImageMime)) {

we should avoid this list because it's hard to keep in sync with the rest of the code.
Can you have a static array, or something?

@@ +91,5 @@
> +  mKind = KIND_OTHER;
> +
> +  nsCOMPtr<nsISupports> supports;
> +  nsresult rv = aData->GetAsISupports(getter_AddRefs(supports));
> +  nsRefPtr<File> file = FileFromISupports(supports);

I don't like these 2 operations, what about:

if (NS_SUCCEEDED(rv)) {
  nsRefPtr<File> file = FileFromISupports(supports):
  if (file) {
    mKind = KIND_FILE;
  }
}

if (mKind == KIND_OTHER) {
  ...
}

@@ +98,5 @@
> +    mKind = KIND_FILE;
> +  } else {
> +    nsAutoString string;
> +    rv = aData->GetAsAString(string);
> +    if (NS_SUCCEEDED(rv)) {

you should print the error here, right?

@@ +123,5 @@
> +  }
> +
> +  nsCOMPtr<nsITransferable> trans =
> +    do_CreateInstance("@mozilla.org/widget/transferable;1");
> +  if (!trans) {

NS_WARN_IF ?

@@ +152,5 @@
> +
> +  uint32_t length = 0;
> +  nsCOMPtr<nsISupports> data;
> +  trans->GetTransferData(format, getter_AddRefs(data), &length);
> +  if (!data) {

NS_WARN_IF

@@ +163,5 @@
> +    // whatever type happens to actually be stored into a dom::File.
> +
> +    nsRefPtr<File> file = FileFromISupports(data);
> +    if (file) {
> +      /* do nothing */

I don't know if I like this empty if.
What about:

if (!file) {
  if (nsCOMPtr<...

@@ +184,5 @@
> +    data = do_QueryObject(file);
> +  }
> +
> +  nsCOMPtr<nsIWritableVariant> variant = do_CreateInstance(NS_VARIANT_CONTRACTID);
> +  if (!variant) {

NS_WARN_IF

@@ +210,5 @@
> +    return nullptr;
> +  }
> +
> +  nsIVariant* data = Data();
> +  if (!data) {

NS_WARN_IF

@@ +252,5 @@
> +  } else if (mType.EqualsASCII(kPNGImageMime)) {
> +    key = "GenericImageNamePNG";
> +  } else if (mType.EqualsASCII(kFileMime)) {
> +    key = "GenericFileName";
> +  } else {

same issue here with the list of mime types.

@@ +296,5 @@
> +  nsString stringData;
> +  Data()->GetAsAString(stringData);
> +
> +  // Dispatch the callback to the main thread
> +  class Runnable : public nsRunnable

final

@@ +304,5 @@
> +             const nsAString& aStringData)
> +      : mCallback(aCallback), mStringData(aStringData)
> +    {}
> +
> +    NS_IMETHOD Run() override {

{ in a new line

@@ +306,5 @@
> +    {}
> +
> +    NS_IMETHOD Run() override {
> +      ErrorResult rv;
> +      mCallback->Call(mStringData, rv);

NS_WARN_IF(rv.Failed())

@@ +315,5 @@
> +    nsString mStringData;
> +  };
> +
> +  nsRefPtr<Runnable> runnable = new Runnable(aCallback, stringData);
> +  NS_DispatchToMainThread(runnable);

This can fail.

::: dom/events/DataTransferItem.h
@@ +6,5 @@
> +#ifndef mozilla_dom_DataTransferItem_h
> +#define mozilla_dom_DataTransferItem_h
> +
> +#include "mozilla/dom/DOMString.h"
> +#include "mozilla/dom/DataTransfer.h"

alphabetic order.

@@ +57,5 @@
> +    }
> +  }
> +  eKind Kind() const { return mKind; }
> +
> +  void GetType(nsAString& aType) const {

{ in a new line.

@@ +68,5 @@
> +  already_AddRefed<File> GetAsFile(ErrorResult& aRv);
> +
> +  DataTransferItemList* GetParentObject() const { return mParent; }
> +
> +  nsIPrincipal* Principal() const {

{ in a new line.

@@ +71,5 @@
> +
> +  nsIPrincipal* Principal() const {
> +    return mPrincipal;
> +  }
> +  void SetPrincipal(nsIPrincipal* aPrincipal) {

{ in a new line.

@@ +74,5 @@
> +  }
> +  void SetPrincipal(nsIPrincipal* aPrincipal) {
> +    mPrincipal = aPrincipal;
> +  }
> +  nsIVariant* Data() {

{ in a new line.

@@ +81,5 @@
> +    }
> +    return mData;
> +  }
> +  void SetData(nsIVariant* aData);
> +  uint32_t Index() const {

{ in a new line.

::: dom/events/DataTransferItemList.cpp
@@ +9,5 @@
> +#include "nsIScriptObjectPrincipal.h"
> +#include "nsIClipboard.h"
> +#include "nsISupportsPrimitives.h"
> +#include "nsQueryObject.h"
> +#include "nsVariant.h"

alphabetic order.

@@ +111,5 @@
> +  if (mIsCrossDomainSubFrameDrop) {
> +    principal = nsContentUtils::SubjectPrincipal();
> +  }
> +
> +  if (item->Principal() && principal && !principal->Subsumes(item->Principal())) {

80chars here and in the rest of the file.

@@ +202,5 @@
> +  // want to update an existing entry if it is already present, as per the spec.
> +  nsRefPtr<DataTransferItem> item;
> +  aRv = SetDataWithPrincipal(format, data, 0,
> +                             nsContentUtils::SubjectPrincipal(),
> +                             true, getter_AddRefs(item));

if (NS_WARN_IF(aRv.Failed())) {
  return nullptr;
}

@@ +232,5 @@
> +  uint32_t index = mIndexedItems.Length();
> +  aRv = SetDataWithPrincipal(type, data, index,
> +                             nsContentUtils::SubjectPrincipal(),
> +                             true, getter_AddRefs(item));
> +  ENSURE_SUCCESS(aRv, nullptr);

ditto

@@ +271,5 @@
> +      uint32_t index = items.Length() - 1;
> +      MOZ_ASSERT(index == count - i - 1);
> +
> +      ClearDataHelper(items[index], -1, index, aRv);
> +      ENSURE_SUCCESS_VOID(aRv);

ditto.

::: dom/events/DataTransferItemList.h
@@ +7,5 @@
> +#define mozilla_dom_DataTransferItemList_h
> +
> +#include "mozilla/dom/FileList.h"
> +#include "mozilla/dom/DataTransfer.h"
> +#include "mozilla/dom/DataTransferItem.h"

alphabetic order

@@ +35,5 @@
> +  already_AddRefed<DataTransferItemList> Clone(DataTransfer* aParent);
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
> +
> +  uint32_t Length() {

{ in a new line.
Attachment #8654291 - Flags: review?(amarchesini) → review-
(Assignee)

Comment 9

2 years ago
Created attachment 8660883 [details] [diff] [review]
V2 Interdiff

Here's a diff between the updated version of the patch, and the one you previously reviewed.
I'll upload the actual updated patch in a second.
(Assignee)

Comment 10

2 years ago
Created attachment 8660884 [details] [diff] [review]
Implement DataTransferItem and DataTransferItemList

Updated - for changes see V2 Interdiff.
Attachment #8654291 - Attachment is obsolete: true
Attachment #8660884 - Flags: review?(amarchesini)
Comment on attachment 8660884 [details] [diff] [review]
Implement DataTransferItem and DataTransferItemList

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

look good. Can you submit this patch again with this comment fixed?
Mainly the comments are about the error handling.

::: dom/base/nsCopySupport.cpp
@@ +683,5 @@
>    bool doDefault = true;
>    nsRefPtr<DataTransfer> clipboardData;
>    if (chromeShell || Preferences::GetBool("dom.event.clipboardevents.enabled", true)) {
>      clipboardData =
> +      new DataTransfer(doc->GetScopeObject(), aType, aType == NS_PASTE,

is it 80chars?

::: dom/events/DataTransfer.cpp
@@ +132,5 @@
>  {
>    MOZ_ASSERT(mParent);
> +  MOZ_ASSERT(aItems);
> +
> +  // We clone the items array after everything else, so that it has a valid mParent value

80chars.

@@ +283,5 @@
>  {
>    ErrorResult rv;
> +  nsRefPtr<FileList> files = GetFiles(rv);
> +  files.forget(aFileList);
> +  NS_WARN_IF(rv.Failed());

ErrorResult rv;

nsRefPtr<FileList> files = GetFiles(rv);
if (NS_WARN_IF(rv.Failed())) {
  return rv.StealNSResult();
}

files.forget(aFileList);
return NS_OK;

@@ +296,5 @@
> +  const nsTArray<nsRefPtr<DataTransferItem>>* items = mItems->MozItemsAt(0);
> +  if (items) {
> +    for (uint32_t i = 0; i < items->Length(); i++) {
> +      // XXX ElementAt can fail because of principal access problems - maybe
> +      // should check that here?

what do you mean?

@@ +306,5 @@
> +      NS_WARN_IF(!types->Add(type));
> +
> +      // If we have any files, we need to also add the "Files" type!
> +      if (item->Kind() == DataTransferItem::KIND_FILE) {
> +        NS_WARN_IF(!types->Add(NS_LITERAL_STRING("Files")));

What about if Types() throws an exception in case?
If it does, you will have a ErrorResult obj and you can do:

if (NS_WARN_IF(!types->Add(NS_LITERAL_STRING("Files"))) {
  aRv.Throw(NS_ERROR_FAILURE);
  return;
}

same for the previous Add().

@@ +508,5 @@
> +
> +    for (uint32_t i = 0; i < items.Length(); i++) {
> +      nsAutoString type;
> +      items[i]->GetType(type);
> +      NS_WARN_IF(!types->Add(type));

if (NS_WARN_IF(!types->Add(type))) {
  aRv.Throw(NS_ERROR_FAILURE);
  return nullptr;
}

@@ +575,5 @@
> +
> +  nsCOMPtr<nsIVariant> data = item->Data();
> +  MOZ_ASSERT(data);
> +  nsCOMPtr<nsISupports> isupportsData;
> +  data->GetAsISupports(getter_AddRefs(isupportsData));

nsresult rv = data->GetAsISupports(...);
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +1135,4 @@
>    uint32_t count;
>    dragSession->GetNumDropItems(&count);
>    for (uint32_t c = 0; c < count; c++) {
> +    for (uint32_t f = 0; f < ArrayLength(kFormats); f++) {

In any other for loop, you do ++f instead f++. Doesn't really matter, but maybe you want to change this one.

::: dom/events/DataTransferItem.cpp
@@ +13,5 @@
> +#include "nsISupportsPrimitives.h"
> +#include "nsNetUtil.h"
> +#include "nsQueryObject.h"
> +
> +namespace {

struct FileMimeNameData;
{
  const char* mMimeName;
  const char* mFileName;
};

FileMimeNameData kFileMimeNameMap[] = {
  { kFileMime, "GenericFileName" },
  { ... }
};

@@ +20,5 @@
> +  kJPEGImageMime, "GenericImageNameJPEG",
> +  kGIFImageMime,  "GenericImageNameGIF",
> +  kPNGImageMime,  "GenericImageNamePNG",
> +};
> +}

} // anonymous namespace

@@ +90,5 @@
> +    // their kind based on their type.
> +    MOZ_ASSERT(!mType.IsEmpty());
> +
> +    mKind = KIND_STRING;
> +    for (uint32_t i = 0; i + 1 < ArrayLength(kFileMimeNameMap); i += 2) {

if you use a data struct it makes this cleaner: ++i

@@ +161,5 @@
> +    if (!clipboard || mParent->ClipboardType() < 0) {
> +      return;
> +    }
> +
> +    clipboard->GetData(trans, mParent->ClipboardType());

can this fail?

@@ +168,5 @@
> +    if (!dragSession) {
> +      return;
> +    }
> +
> +    dragSession->GetData(trans, mIndex);

can this fail?

@@ +173,5 @@
> +  }
> +
> +  uint32_t length = 0;
> +  nsCOMPtr<nsISupports> data;
> +  trans->GetTransferData(format, getter_AddRefs(data), &length);

can this fail?

@@ +191,5 @@
> +      } else if (nsCOMPtr<nsIInputStream> stream = do_QueryInterface(data)) {
> +        // This consumes the stream object
> +        ErrorResult rv;
> +        file = CreateFileFromInputStream(GetParentObject(), stream, rv);
> +        if (NS_WARN_IF(rv.Failed())) {

maybe you want to return this error code instead and propagate the error properly.

@@ +232,5 @@
> +  if (mKind != KIND_FILE) {
> +    return nullptr;
> +  }
> +
> +  nsIVariant* data = Data();

if this gets aRv, you can use it in FillInExternalData.

::: dom/events/DataTransferItemList.cpp
@@ +95,5 @@
> +DataTransferItem*
> +DataTransferItemList::IndexedGetter(uint32_t aIndex, bool& aFound, ErrorResult& aRv)
> +{
> +  if (aIndex >= mItems.Length()) {
> +    aFound = false;

I guess you should return an error in this case. Don't you?
This is not what you do in Remove().

@@ +127,5 @@
> +  nsCOMPtr<EventTarget> pt = do_QueryInterface(data);
> +  if (pt) {
> +    nsresult rv = NS_OK;
> +    nsIScriptContext* c = pt->GetContextForEventHandlers(&rv);
> +    if (NS_FAILED(rv) || !c) {

NS_WARN_IF

@@ +133,5 @@
> +      return nullptr;
> +    }
> +
> +    nsIGlobalObject* go = c->GetGlobalObject();
> +    if (!go) {

NS_WARN_IF

@@ +145,5 @@
> +    nsIPrincipal* dataPrincipal = sp->GetPrincipal();
> +    if (!principal) {
> +      principal = nsContentUtils::SubjectPrincipal();
> +    }
> +    if (!dataPrincipal || !principal->Equals(dataPrincipal)) {

NS_WARN_IF

@@ +191,5 @@
> +                          const nsAString& aType,
> +                          ErrorResult& aRv)
> +{
> +  if (IsReadOnly()) {
> +    return nullptr;

ditto.

@@ +216,5 @@
> +
> +DataTransferItem*
> +DataTransferItemList::Add(File& aData, ErrorResult& aRv)
> +{
> +  if (IsReadOnly()) {

ditto.

@@ +225,5 @@
> +  nsCOMPtr<nsIWritableVariant> data = new nsVariant();
> +  data->SetAsISupports(supports);
> +
> +  nsAutoString type;
> +  aData.GetType(type);

can fail?

@@ +261,5 @@
> +                                        uint32_t aIndex,
> +                                        ErrorResult& aRv)
> +{
> +  if (IsReadOnly() ||
> +      aIndex >= mIndexedItems.Length()) {

ditto.

@@ +301,5 @@
> +
> +DataTransferItem*
> +DataTransferItemList::MozItemByTypeAt(const nsAString& aType, uint32_t aIndex)
> +{
> +  if (aIndex >= mIndexedItems.Length()) {

ditto

@@ +309,5 @@
> +  uint32_t count = mIndexedItems[aIndex].Length();
> +  for (uint32_t i = 0; i < count; i++) {
> +    nsRefPtr<DataTransferItem> item = mIndexedItems[aIndex][i];
> +    nsString type;
> +    item->GetType(type);

can fail?

@@ +335,5 @@
> +      item = items[i];
> +      nsString type;
> +      item->GetType(type);
> +      if (type.Equals(aType)) {
> +        if (aInsertOnly) {

NS_WARN_IF ?

@@ +342,5 @@
> +
> +        // don't allow replacing data that has a stronger principal
> +        bool subsumes;
> +        if (item->Principal() && aPrincipal &&
> +            (NS_FAILED(aPrincipal->Subsumes(item->Principal(), &subsumes))

NS_WARN_IF ?

@@ +481,5 @@
> +                                      uint32_t aMozOffsetHint,
> +                                      ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(aItem);
> +  if (IsReadOnly()) {

ditto.

::: dom/events/DataTransferItemList.h
@@ +84,5 @@
> +  DataTransferItem* AppendNewItem(uint32_t aIndex, const nsAString& aType,
> +                                  nsIVariant* aData, nsIPrincipal* aPrincipal);
> +  void RegenerateFiles();
> +
> +  ~DataTransferItemList() {}

empty line after this.

@@ +90,5 @@
> +  bool mIsCrossDomainSubFrameDrop;
> +  bool mIsExternal;
> +  nsRefPtr<FileList> mFiles;
> +  nsTArray<nsRefPtr<DataTransferItem> > mItems;
> +  nsTArray<nsTArray<nsRefPtr<DataTransferItem> > > mIndexedItems;

no extra spaces between >
Attachment #8660884 - Flags: review?(amarchesini) → review-
(Assignee)

Comment 12

2 years ago
Created attachment 8664270 [details] [diff] [review]
V3 Interdiff

New interdiff - patch to follow
Attachment #8660883 - Attachment is obsolete: true
(Assignee)

Comment 13

2 years ago
Created attachment 8664272 [details] [diff] [review]
Implement DataTransferItem and DataTransferItemList

(In reply to Andrea Marchesini (:baku) from comment #11)
> Comment on attachment 8660884 [details] [diff] [review]
> Implement DataTransferItem and DataTransferItemList
> 
> Review of attachment 8660884 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> look good. Can you submit this patch again with this comment fixed?
> Mainly the comments are about the error handling.
> 
> ::: dom/base/nsCopySupport.cpp
> @@ +683,5 @@
> >    bool doDefault = true;
> >    nsRefPtr<DataTransfer> clipboardData;
> >    if (chromeShell || Preferences::GetBool("dom.event.clipboardevents.enabled", true)) {
> >      clipboardData =
> > +      new DataTransfer(doc->GetScopeObject(), aType, aType == NS_PASTE,
> 
> is it 80chars?

The Preferences::GetBool line isn't, but the new DataTransfer line is. Should I modify
the Preferences::GetBool line?

> @@ +296,5 @@
> > +  const nsTArray<nsRefPtr<DataTransferItem>>* items = mItems->MozItemsAt(0);
> > +  if (items) {
> > +    for (uint32_t i = 0; i < items->Length(); i++) {
> > +      // XXX ElementAt can fail because of principal access problems - maybe
> > +      // should check that here?
> 
> what do you mean?

I messed up (got this ElementAt confused with
DataTransferItemList::IndexedGetter) :S - removed the comment

> @@ +306,5 @@
> > +      NS_WARN_IF(!types->Add(type));
> > +
> > +      // If we have any files, we need to also add the "Files" type!
> > +      if (item->Kind() == DataTransferItem::KIND_FILE) {
> > +        NS_WARN_IF(!types->Add(NS_LITERAL_STRING("Files")));
> 
> What about if Types() throws an exception in case?
> If it does, you will have a ErrorResult obj and you can do:
> 
> if (NS_WARN_IF(!types->Add(NS_LITERAL_STRING("Files"))) {
>   aRv.Throw(NS_ERROR_FAILURE);
>   return;
> }
> 
> same for the previous Add().
> 
> @@ +508,5 @@
> > +
> > +    for (uint32_t i = 0; i < items.Length(); i++) {
> > +      nsAutoString type;
> > +      items[i]->GetType(type);
> > +      NS_WARN_IF(!types->Add(type));
> 
> if (NS_WARN_IF(!types->Add(type))) {
>   aRv.Throw(NS_ERROR_FAILURE);
>   return nullptr;
> }

I added the exception, I figure it can't hurt - that being said, I'm pretty sure that the false value here is in case FileList fails to allocate space in the array, which should only happen with OOM. In addition, I'm pretty sure that FileList actually uses an infallible allocator (nsTArray), so this will never happen anyways :S. It's probably a good idea to be ready for a potential future where that changes though!

> @@ +1135,4 @@
> >    uint32_t count;
> >    dragSession->GetNumDropItems(&count);
> >    for (uint32_t c = 0; c < count; c++) {
> > +    for (uint32_t f = 0; f < ArrayLength(kFormats); f++) {
> 
> In any other for loop, you do ++f instead f++. Doesn't really matter, but
> maybe you want to change this one.

Consistency is always good :)

> ::: dom/events/DataTransferItem.cpp
> @@ +13,5 @@
> > +#include "nsISupportsPrimitives.h"
> > +#include "nsNetUtil.h"
> > +#include "nsQueryObject.h"
> > +
> > +namespace {
> 
> struct FileMimeNameData;
> {
>   const char* mMimeName;
>   const char* mFileName;
> };
> 
> FileMimeNameData kFileMimeNameMap[] = {
>   { kFileMime, "GenericFileName" },
>   { ... }
> };
> 
> @@ +20,5 @@
> > +  kJPEGImageMime, "GenericImageNameJPEG",
> > +  kGIFImageMime,  "GenericImageNameGIF",
> > +  kPNGImageMime,  "GenericImageNamePNG",
> > +};
> > +}
> 
> } // anonymous namespace

Much nicer, thanks :)

> @@ +161,5 @@
> > +    if (!clipboard || mParent->ClipboardType() < 0) {
> > +      return;
> > +    }
> > +
> > +    clipboard->GetData(trans, mParent->ClipboardType());
> 
> can this fail?
> 
> @@ +168,5 @@
> > +    if (!dragSession) {
> > +      return;
> > +    }
> > +
> > +    dragSession->GetData(trans, mIndex);
> 
> can this fail?
> 
> @@ +173,5 @@
> > +  }
> > +
> > +  uint32_t length = 0;
> > +  nsCOMPtr<nsISupports> data;
> > +  trans->GetTransferData(format, getter_AddRefs(data), &length);
> 
> can this fail?
> 
> @@ +191,5 @@
> > +      } else if (nsCOMPtr<nsIInputStream> stream = do_QueryInterface(data)) {
> > +        // This consumes the stream object
> > +        ErrorResult rv;
> > +        file = CreateFileFromInputStream(GetParentObject(), stream, rv);
> > +        if (NS_WARN_IF(rv.Failed())) {
> 
> maybe you want to return this error code instead and propagate the error
> properly.

Added some checks - not propagating failures because I feel like this lazy
loading failing shouldn't create exceptions at the arbitrary function which
caused the lazy loading. (The FillInExternalData function is lazy loading of
resources from the OS when they are needed, which I'm not sure I feel
comfortable making visible to user scripts in the case of failure.

Let me know if I should change that though!

> @@ +232,5 @@
> > +  if (mKind != KIND_FILE) {
> > +    return nullptr;
> > +  }
> > +
> > +  nsIVariant* data = Data();
> 
> if this gets aRv, you can use it in FillInExternalData.

Again, with the above statement, I feel a bit weird making getting Data() throw
an error if we mess up getting data from the OS. It would mean that the first
call had a chance to throw, but not subsequent ones, and it feels like it might
expose too much of our internals to scripts, but I'm willing to make the change
if you want.

> ::: dom/events/DataTransferItemList.cpp
> @@ +95,5 @@
> > +DataTransferItem*
> > +DataTransferItemList::IndexedGetter(uint32_t aIndex, bool& aFound, ErrorResult& aRv)
> > +{
> > +  if (aIndex >= mItems.Length()) {
> > +    aFound = false;
> 
> I guess you should return an error in this case. Don't you?
> This is not what you do in Remove().

IndexedGetter is the [] getter, and I'm pretty sure that by spec I don't throw in that case.
This is consistent with chrome and how other fake array objects work in javascript.

> @@ +225,5 @@
> > +  nsCOMPtr<nsIWritableVariant> data = new nsVariant();
> > +  data->SetAsISupports(supports);
> > +
> > +  nsAutoString type;
> > +  aData.GetType(type);
> 
> can fail?

Nope, File::GetType is infallible :)

> @@ +309,5 @@
> > +  uint32_t count = mIndexedItems[aIndex].Length();
> > +  for (uint32_t i = 0; i < count; i++) {
> > +    nsRefPtr<DataTransferItem> item = mIndexedItems[aIndex][i];
> > +    nsString type;
> > +    item->GetType(type);
> 
> can fail?

Also infallible :)
Attachment #8660884 - Attachment is obsolete: true
Attachment #8664272 - Flags: review?(amarchesini)
Comment on attachment 8664272 [details] [diff] [review]
Implement DataTransferItem and DataTransferItemList

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

lgtm!

::: dom/events/DataTransferItemList.h
@@ +36,5 @@
> +
> +  virtual JSObject* WrapObject(JSContext* aCx,
> +                               JS::Handle<JSObject*> aGivenProto) override;
> +
> +  uint32_t Length()

uint32_t Length() const

@@ +48,5 @@
> +
> +  void Remove(uint32_t aIndex, ErrorResult& aRv);
> +
> +  DataTransferItem* IndexedGetter(uint32_t aIndex, bool& aFound,
> +                                  ErrorResult& aRv);

can it be const?

@@ +72,5 @@
> +  void MozRemoveByTypeAt(const nsAString& aType, uint32_t aIndex,
> +                         ErrorResult& aRv);
> +  DataTransferItem* MozItemByTypeAt(const nsAString& aType, uint32_t aIndex);
> +  const nsTArray<nsRefPtr<DataTransferItem> >* MozItemsAt(uint32_t aIndex);
> +  uint32_t MozItemCount();

const
Attachment #8664272 - Flags: review?(amarchesini) → review+
(Assignee)

Updated

2 years ago
Duplicate of this bug: 891247
(Assignee)

Comment 16

2 years ago
Created attachment 8664332 [details] [diff] [review]
Implement DataTransferItem and DataTransferItemList

Had to rebase - waiting on try to merge: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b361f308cbf1
Attachment #8664270 - Attachment is obsolete: true
Attachment #8664272 - Attachment is obsolete: true
(Assignee)

Comment 17

2 years ago
There's a memory leak in the impl which is caused by nsVariant not being cycle collected. I'm blocking on that.
Depends on: 931283
(Assignee)

Comment 18

2 years ago
Created attachment 8668478 [details] [diff] [review]
Part 2: Fix assorted test failures caused by part 1

This patch is still failing tests after these changes because nsVariant doesn't participate in cycle collection,
and thus there is a memory leak. I'm currently depending on a bug which hopefully will fix that, but otherwise I
can probably do some extra work to avoid creating cycles through nsVariants.
Attachment #8668478 - Flags: review?(amarchesini)
Depends on: 1210591
No longer depends on: 931283
Comment on attachment 8668478 [details] [diff] [review]
Part 2: Fix assorted test failures caused by part 1

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

::: dom/events/DataTransfer.cpp
@@ +1082,5 @@
>  void
>  DataTransfer::ClearAll()
>  {
> +  mItems = new DataTransferItemList(this, mIsExternal,
> +                                    mIsCrossDomainSubFrameDrop);

tell me more about this. Why Clear() is not enough?
Attachment #8668478 - Flags: review?(amarchesini)
(Assignee)

Comment 20

2 years ago
(In reply to Andrea Marchesini (:baku) from comment #19)
> Comment on attachment 8668478 [details] [diff] [review]
> Part 2: Fix assorted test failures caused by part 1
> 
> Review of attachment 8668478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/events/DataTransfer.cpp
> @@ +1082,5 @@
> >  void
> >  DataTransfer::ClearAll()
> >  {
> > +  mItems = new DataTransferItemList(this, mIsExternal,
> > +                                    mIsCrossDomainSubFrameDrop);
> 
> tell me more about this. Why Clear() is not enough?

Clear() checks if the type is read-only, and doesn't clear if the type is read only. ClearAll is used internally after clipboard events are fired to ensure that people can't access & modify the clipboard after the event handler has been fired, and the DataTransfer is read-only at that point in time. The old semantics always cleared (as ClearAll was internal-only), but the new ones were using an exposed API.

This was the simplest way I could think of to avoid this problem.
Comment on attachment 8668478 [details] [diff] [review]
Part 2: Fix assorted test failures caused by part 1

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

The reason why I don't like this patch is because it breaks this code:

var items = something.items;
something.clear();
items == something.items // should be true.

Do you agree?
Attachment #8668478 - Flags: review-
(Assignee)

Comment 22

2 years ago
Created attachment 8671348 [details] [diff] [review]
Part 2: Fix assorted test failures caused by part 1

(In reply to Andrea Marchesini (:baku) from comment #21)
> Comment on attachment 8668478 [details] [diff] [review]
> Part 2: Fix assorted test failures caused by part 1
> 
> Review of attachment 8668478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The reason why I don't like this patch is because it breaks this code:
> 
> var items = something.items;
> something.clear();
> items == something.items // should be true.
> 
> Do you agree?

I agree that the patch was bad, I've attached a new one which shouldn't have the issue.

That being said, ClearAll isn't exposed to JS (it's only for internal use - as it's intended to ignore read-only status, and be used for clearing out a DataTransfer after its event is over). JS can only see clearData (and mozClearData) which use a different code path.
Attachment #8668478 - Attachment is obsolete: true
Attachment #8671348 - Flags: review?(amarchesini)
Blocks: 938991

Updated

2 years ago
No longer blocks: 938991
Comment on attachment 8671348 [details] [diff] [review]
Part 2: Fix assorted test failures caused by part 1

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

sorry for the delay.

::: dom/events/DataTransfer.cpp
@@ +600,5 @@
> +  if (NS_SUCCEEDED(rv) && isupportsData) {
> +    // Make sure the code that is calling us is same-origin with the data.
> +    nsCOMPtr<EventTarget> pt = do_QueryInterface(isupportsData);
> +    if (pt) {
> +      nsresult rv = NS_OK;

rv already exists.
Attachment #8671348 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 24

2 years ago
There have been a lot of changes to this code since last time I updated it (like the nsRefPtr->RefPtr and bugs like bug 1217614), so it's going to take a while until I land this - ni?-ing myself to make sure that it gets fixed up.
(Assignee)

Updated

2 years ago
Flags: needinfo?(michael)
(Assignee)

Comment 25

2 years ago
try for rebased patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ceb1b8acf223
Flags: needinfo?(michael)
Duplicate of this bug: 1225453
Summary: [DnD] dataTransfer.items undefined in Firefox → [DnD] dataTransfer.items undefined in Firefox (implement DataTransferItem and DataTransferItemList)
(Assignee)

Updated

2 years ago
Depends on: 1231480
Blocks: 1200697
:mystor, can I steal this bug?
Flags: needinfo?(michael)
The reason is that I need to have this implemented for the Directory Upload API.
(Assignee)

Comment 29

a year ago
(In reply to Andrea Marchesini (:baku) from comment #27)
> :mystor, can I steal this bug?

As I said on IRC, yes, go ahead.
Flags: needinfo?(michael)
(Assignee)

Comment 30

a year ago
Created attachment 8748038 [details] [diff] [review]
Implement DataTransferItem and DataTransferItemList

I'm so looking forward to finally landing this and no longer dealing with how much it's bitrotted.
Attachment #8748038 - Flags: review?(amarchesini)
(Assignee)

Updated

a year ago
Attachment #8664332 - Attachment is obsolete: true
Attachment #8671348 - Attachment is obsolete: true
(Assignee)

Comment 31

a year ago
Created attachment 8748135 [details] [diff] [review]
Implement DataTransferItem and DataTransferItemList

I noticed after running the try push that somehow the:

  [Window interface: operation showModalDialog(DOMString,any)]
    disabled:
      if e10s: https://bugzilla.mozilla.org/show_bug.cgi?id=981796

line had been deleted. I've restored it and everything should be working now.
Attachment #8748135 - Flags: review?(amarchesini)
(Assignee)

Updated

a year ago
Attachment #8748038 - Attachment is obsolete: true
Attachment #8748038 - Flags: review?(amarchesini)
Comment on attachment 8748135 [details] [diff] [review]
Implement DataTransferItem and DataTransferItemList

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

First a quick thing: I was already reviewing your patch :) Next time, submit a interdiff or ping me before canceling a review.

1. Potentially, DnD can contain a lot of data. We should use FallibleArray instead of nsTArray
2. nsVariantCC ?
3. I want to understand more about this index 0 issue.

r- just because I want to have more info.

::: dom/events/DataTransfer.cpp
@@ +95,5 @@
>    , mClipboardType(aClipboardType)
>    , mDragImageX(0)
>    , mDragImageY(0)
>  {
> +  mItems = new DataTransferItemList(this, aIsExternal, false);

add /* aIsCrossDomainSubFrameDrop */ just next to 'false'.

@@ +146,5 @@
> +  MOZ_ASSERT(aItems);
> +
> +  // We clone the items array after everything else, so that it has a valid
> +  // mParent value
> +  mItems = aItems->Clone(this);

pass aParent instead of this. and maybe, this can be a nsIGlobalObject.

@@ +293,5 @@
>  }
>  
>  NS_IMETHODIMP
>  DataTransfer::GetFiles(nsIDOMFileList** aFileList)
>  {

if (NS_WARN_IF(!aFileList)) {
  return NS_ERROR_FAILURE;
}

@@ +310,5 @@
>  {
>    RefPtr<DOMStringList> types = new DOMStringList();
> +
> +  const nsTArray<RefPtr<DataTransferItem>>* items = mItems->MozItemsAt(0);
> +  if (items) {

if (!item || item->IsEmpty()) {
  return types.forget();
}

@@ +325,3 @@
>  
> +      // If we have any files, we need to also add the "Files" type!
> +      if (item->Kind() == DataTransferItem::KIND_FILE) {

I don't think this block should be into the for-loop. Do something similar to what the previous code did.

@@ +337,5 @@
>  }
>  
>  NS_IMETHODIMP
>  DataTransfer::GetTypes(nsISupports** aTypes)
>  {

if (NS_WARN_IF(!aTypes)) {
  return NS_ERROR_FAILURE;
}

@@ +1282,5 @@
> +// all platforms, so just check for the types that can actually be imported
> +// XXXndeakin there are some other formats but those are platform specific.
> +const char* kFormats[] = { kFileMime, kHTMLMime, kURLMime, kURLDataMime,
> +                           kUnicodeMime, kPNGImageMime, kJPEGImageMime,
> +                           kGIFImageMime };

Add new formats in a separate patch.

::: dom/events/DataTransferItem.cpp
@@ +67,5 @@
> +  return it.forget();
> +}
> +
> +/* static */ already_AddRefed<File>
> +DataTransferItem::FileFromISupports(nsISupports* aSupports)

move this to a anonymous namespace and make it a static function.

@@ +109,5 @@
> +    mData = nullptr;
> +    return;
> +  }
> +
> +  mKind = KIND_OTHER;

mData = aData;

@@ +116,5 @@
> +  nsresult rv = aData->GetAsISupports(getter_AddRefs(supports));
> +  if (NS_SUCCEEDED(rv) && supports) {
> +    RefPtr<File> file = FileFromISupports(supports);
> +    if (file) {
> +      mKind = KIND_FILE;

return;

@@ +120,5 @@
> +      mKind = KIND_FILE;
> +    }
> +  }
> +
> +  if (mKind == KIND_OTHER) {

remove this check.

@@ +133,5 @@
> +      mKind = KIND_STRING;
> +    }
> +  }
> +
> +  mData = aData;

remove this.

@@ +220,5 @@
> +    data = do_QueryObject(file);
> +  }
> +
> +  nsCOMPtr<nsIWritableVariant> variant =
> +    do_CreateInstance(NS_VARIANT_CONTRACTID);

You are not using a nsVariantCC, why?

@@ +254,5 @@
> +  if (mKind != KIND_FILE) {
> +    return nullptr;
> +  }
> +
> +  nsIVariant* data = Data();

This is not supposed to fail, right?

@@ +332,5 @@
> +  if (!aCallback || mKind != KIND_STRING) {
> +    return;
> +  }
> +
> +  nsIVariant* data = Data();

failing?

@@ +337,5 @@
> +  if (NS_WARN_IF(!data)) {
> +    return;
> +  }
> +
> +  nsString stringData;

nsAutoString

::: dom/events/DataTransferItem.h
@@ +33,5 @@
> +    KIND_STRING,
> +    KIND_OTHER,
> +  };
> +
> +  explicit DataTransferItem(DataTransferItemList* aParent, const nsAString& aType)

no explicit for multiple params CTOR

@@ +35,5 @@
> +  };
> +
> +  explicit DataTransferItem(DataTransferItemList* aParent, const nsAString& aType)
> +    : mIndex(0), mKind(KIND_OTHER), mType(aType), mData(nullptr),
> +    mPrincipal(nullptr), mParent(aParent)

remove mPrincipal(nullptr) and mData(nullptr)

@@ +38,5 @@
> +    : mIndex(0), mKind(KIND_OTHER), mType(aType), mData(nullptr),
> +    mPrincipal(nullptr), mParent(aParent)
> +  {}
> +
> +  already_AddRefed<DataTransferItem> Clone(DataTransferItemList* aParent);

) const; ?

@@ +64,5 @@
> +  {
> +    aType = mType;
> +  }
> +
> +  void SetKind(eKind aKind) { mKind = aKind; }

Be consistent:

void SetKind(eKind aKind)
{
  mKind = aKind;
}

same for the other methods.

@@ +65,5 @@
> +    aType = mType;
> +  }
> +
> +  void SetKind(eKind aKind) { mKind = aKind; }
> +  void SetType(const nsAString& aType);

move it close to GetType

::: dom/events/DataTransferItemList.h
@@ +31,5 @@
> +    // An index 0 should always be present
> +    mIndexedItems.SetLength(1);
> +  }
> +
> +  already_AddRefed<DataTransferItemList> Clone(DataTransfer* aParent);

const?

@@ +53,5 @@
> +
> +  void Clear(ErrorResult& aRv);
> +
> +  /* Internal Only */
> +  DataTransfer* GetParentObject() const { return mParent; }

indentation consistency.

@@ +56,5 @@
> +  /* Internal Only */
> +  DataTransfer* GetParentObject() const { return mParent; }
> +
> +  // Accessors for data from ParentObject
> +  bool IsReadOnly();

const

@@ +57,5 @@
> +  DataTransfer* GetParentObject() const { return mParent; }
> +
> +  // Accessors for data from ParentObject
> +  bool IsReadOnly();
> +  int32_t ClipboardType();

const

@@ +58,5 @@
> +
> +  // Accessors for data from ParentObject
> +  bool IsReadOnly();
> +  int32_t ClipboardType();
> +  EventMessage GetEventMessage();

const ?

@@ +98,5 @@
> +  nsTArray<RefPtr<DataTransferItem> > mItems;
> +  nsTArray<nsTArray<RefPtr<DataTransferItem>>> mIndexedItems;
> +};
> +
> +

extra line
Attachment #8748135 - Flags: review?(amarchesini) → review-
Blocks: 1150519
(Assignee)

Comment 33

a year ago
Created attachment 8749185 [details] [diff] [review]
Part 1: Implement DataTransferItem and DataTransferItemList
Attachment #8749185 - Flags: review?(amarchesini)
(Assignee)

Updated

a year ago
Attachment #8748135 - Attachment is obsolete: true
(Assignee)

Comment 34

a year ago
Created attachment 8749186 [details] [diff] [review]
Part 2: Add support for images to DataTransfer
Attachment #8749186 - Flags: review?(amarchesini)
(Assignee)

Comment 35

a year ago
Created attachment 8749187 [details]
overall_interdiff

This is the interdiff between the final state (after both patches were applied). Hopefully it's useful so you don't have to read the entire patch again :S.
Comment on attachment 8749185 [details] [diff] [review]
Part 1: Implement DataTransferItem and DataTransferItemList

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

::: dom/events/DataTransfer.cpp
@@ +332,5 @@
>      }
>    }
>  
> +  // If we have any files, we need to also add the "Files" type!
> +  if (NS_WARN_IF(!types->Add(NS_LITERAL_STRING("Files")))) {

well... you should use addFile here, right?

if (addFile && NS_WARN_IF(!...

::: dom/events/DataTransferItemList.cpp
@@ +331,5 @@
> +                                           nsIVariant* aData,
> +                                           uint32_t aIndex,
> +                                           nsIPrincipal* aPrincipal,
> +                                           bool aInsertOnly,
> +                                           DataTransferItem** aItem)

What about if this is:

already_AddRefed<DataTransferItem>
DataTransferItemList::SetDataWithPrincipal(const nsAString& aType,
                                           nsIVariant* aData,
                                           uint32_t aIndex,
                                           nsIPrincipal* aPrincipal,
                                           bool aInsertOnly,
                                           ErrorResult& aRv)

@@ +335,5 @@
> +                                           DataTransferItem** aItem)
> +{
> +  RefPtr<DataTransferItem> item;
> +  if (aIndex < mIndexedItems.Length()) {
> +    nsTArray<RefPtr<DataTransferItem> >& items = mIndexedItems[aIndex];

remove space between > >

@@ +392,5 @@
> +    aIndex = mIndexedItems.Length();
> +  }
> +
> +  // Add the new item
> +  item = AppendNewItem(aIndex, aType, aData, aPrincipal);

just use the type here.

::: dom/events/DataTransferItemList.h
@@ +26,5 @@
> +                       bool aIsCrossDomainSubFrameDrop)
> +    : mParent(aParent)
> +    , mIsCrossDomainSubFrameDrop(aIsCrossDomainSubFrameDrop)
> +    , mIsExternal(aIsExternal)
> +  {

MOZ_ASSERT(aParent);

@@ +52,5 @@
> +                                  ErrorResult& aRv) const;
> +
> +  void Clear(ErrorResult& aRv);
> +
> +  /* Internal Only */

actually is not. Why this comment?

@@ +97,5 @@
> +  RefPtr<DataTransfer> mParent;
> +  bool mIsCrossDomainSubFrameDrop;
> +  bool mIsExternal;
> +  RefPtr<FileList> mFiles;
> +  nsTArray<RefPtr<DataTransferItem> > mItems;

remove space between > >

@@ +98,5 @@
> +  bool mIsCrossDomainSubFrameDrop;
> +  bool mIsExternal;
> +  RefPtr<FileList> mFiles;
> +  nsTArray<RefPtr<DataTransferItem> > mItems;
> +  nsTArray<nsTArray<RefPtr<DataTransferItem>>> mIndexedItems;

This array of array is quite confusing. Let's try to make it simpler. Here an idea:

2 lists:

nsTArray<RefPtr<DataTransferItem>> mStringItems;
nsTArray<RefPtr<DataTransferItem>> mFileItems;

mItems at this point will be an array of raw pointers. Or viceversa, mStringItems and mFileItems can be raw.

Instead having:
  nsresult SetDataWithPrincipal(const nsAString& aType, nsIVariant* aData,
                                uint32_t aIndex, nsIPrincipal* aPrincipal,
                                bool aInsertOnly = false, DataTransferItem** aItem = nullptr);

we have:


  nsresult SetDataWithPrincipal(const nsAString& aType, nsIVariant* aData,
                                ItemType aType, nsIPrincipal* aPrincipal,
                                bool aInsertOnly = false, DataTransferItem** aItem = nullptr);

Where ItemType is { eFile, eString }.

In SetDataWithPrincipal we can assert that nsIVariant is actually containing File when we have eType, and String when we have eString.
In #ifdef DEBUG #endif.
Attachment #8749185 - Flags: review?(amarchesini)
(Assignee)

Comment 37

a year ago
(In reply to Andrea Marchesini (:baku) from comment #36)
> Comment on attachment 8749185 [details] [diff] [review]
> @@ +98,5 @@
> > +  bool mIsCrossDomainSubFrameDrop;
> > +  bool mIsExternal;
> > +  RefPtr<FileList> mFiles;
> > +  nsTArray<RefPtr<DataTransferItem> > mItems;
> > +  nsTArray<nsTArray<RefPtr<DataTransferItem>>> mIndexedItems;
> 
> This array of array is quite confusing. Let's try to make it simpler. Here
> an idea:
> 
> 2 lists:
> 
> nsTArray<RefPtr<DataTransferItem>> mStringItems;
> nsTArray<RefPtr<DataTransferItem>> mFileItems;
> 
> mItems at this point will be an array of raw pointers. Or viceversa,
> mStringItems and mFileItems can be raw.
> 
> Instead having:
>   nsresult SetDataWithPrincipal(const nsAString& aType, nsIVariant* aData,
>                                 uint32_t aIndex, nsIPrincipal* aPrincipal,
>                                 bool aInsertOnly = false, DataTransferItem**
> aItem = nullptr);
> 
> we have:
> 
> 
>   nsresult SetDataWithPrincipal(const nsAString& aType, nsIVariant* aData,
>                                 ItemType aType, nsIPrincipal* aPrincipal,
>                                 bool aInsertOnly = false, DataTransferItem**
> aItem = nullptr);
> 
> Where ItemType is { eFile, eString }.
> 
> In SetDataWithPrincipal we can assert that nsIVariant is actually containing
> File when we have eType, and String when we have eString.
> In #ifdef DEBUG #endif.

I wish that I could dump mIndexedItems. Unfortunately, mIndexedItems is required to support the legacy moz* APIs exposed on DataTransfer. the moz* APIs exposed the 2d array data structure. Right now mItems and mIndexedItems are kept in sync by the methods. Once the moz* APIs are dropped, we can throw out mIndexedItems, but until then I need to keep track of indexes and other gross stuff like that. Your model with FileItems and StringItems unfortunately doesn't work for this :(

Once we drop the moz* APIs, however, we can drop mIndexedItems, and simplify everything down to just use mItems.
(Assignee)

Comment 38

a year ago
Created attachment 8749371 [details] [diff] [review]
Part 1: Implement DataTransferItem and DataTransferItemList

This implements the suggested changes except for the one about changing the internal representation (which wasn't changed for the reasons described in the above comment).
Attachment #8749371 - Flags: review?(amarchesini)
(Assignee)

Updated

a year ago
Attachment #8749185 - Attachment is obsolete: true

Comment 39

a year ago
@Michael, thanks addressing this bug. Do you have any sense when it will make it into the product?
Attachment #8749186 - Flags: review?(amarchesini) → review+
Comment on attachment 8749371 [details] [diff] [review]
Part 1: Implement DataTransferItem and DataTransferItemList

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

Sorry for the delay.

::: dom/events/DataTransfer.cpp
@@ +136,5 @@
>    , mIsExternal(aIsExternal)
>    , mUserCancelled(aUserCancelled)
>    , mIsCrossDomainSubFrameDrop(aIsCrossDomainSubFrameDrop)
>    , mClipboardType(aClipboardType)
> +  , mItems(nullptr)

remove this.

@@ +293,5 @@
>  }
>  
>  NS_IMETHODIMP
>  DataTransfer::GetFiles(nsIDOMFileList** aFileList)
>  {

if (!aFileList) {
  return NS_ERRROR_FAILURE;
}

::: dom/events/DataTransferItem.cpp
@@ +24,5 @@
> +  const char* mFileName;
> +};
> +
> +FileMimeNameData kFileMimeNameMap[] = {
> +  { kFileMime,      "GenericFileName" },

why so many spaces between , and " ?

@@ +259,5 @@
> +    return nullptr;
> +  }
> +
> +  RefPtr<File> file = FileFromISupports(supports);
> +  MOZ_ASSERT(file, "Files should be stored with type dom::File!");

You should remove this MOZ_ASSERT or the comment above.

::: dom/events/DataTransferItemList.cpp
@@ +41,5 @@
> +
> +already_AddRefed<DataTransferItemList>
> +DataTransferItemList::Clone(DataTransfer* aParent) const
> +{
> +  RefPtr<DataTransferItemList> it =

"it"?

@@ +168,5 @@
> +{
> +  uint32_t length = mIndexedItems.Length();
> +  // XXX: Compat hack - Index 0 always exists due to changes in internals, but
> +  // if it is empty, scripts using the moz* APIs should see it as not existing.
> +  if (length == 1 && mIndexedItems[0].Length() == 0) {

IsEmpty()

::: dom/events/DataTransferItemList.h
@@ +27,5 @@
> +    : mParent(aParent)
> +    , mIsCrossDomainSubFrameDrop(aIsCrossDomainSubFrameDrop)
> +    , mIsExternal(aIsExternal)
> +  {
> +    // An index 0 should always be present

why? write here why or write where a good description is located.

::: dom/webidl/DataTransferItem.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * The origin of this IDL file is:
> + * http://www.whatwg.org/specs/web-apps/current-work/#the-datatransferitem-interface

can you use the multipage URL? here and in all the other webidl files you touched.

::: image/imgTools.cpp
@@ +126,5 @@
>    nsAutoCString encoderCID(
>      NS_LITERAL_CSTRING("@mozilla.org/image/encoder;2?type=") + aMimeType);
>  
>    nsCOMPtr<imgIEncoder> encoder = do_CreateInstance(encoderCID.get());
> +  if (NS_WARN_IF(!encoder)) {

completely unrelated changes. Remove it.

::: widget/gonk/nsClipboard.cpp
@@ +275,5 @@
> +        nsresult rv = imgTool->EncodeImage(imageContainer,
> +                                           flavorStr,
> +                                           EmptyString(),
> +                                           getter_AddRefs(byteStream));
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

Why so positive here? I prefer a if (NS_WARN_IF(...))) {\ncontinue }
Attachment #8749371 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 41

a year ago
Created attachment 8758433 [details] [diff] [review]
Part 3: Either expose files or strings generated by system drag or clipboard events to content, not both

Changes to patch to accomodate changes to DataTransfer during the last month
Attachment #8758433 - Flags: review?(amarchesini)
Comment on attachment 8758433 [details] [diff] [review]
Part 3: Either expose files or strings generated by system drag or clipboard events to content, not both

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

::: dom/events/DataTransfer.cpp
@@ +1255,5 @@
>    GetRealFormat(aFormat, format);
>  
>    ErrorResult rv;
>    RefPtr<DataTransferItem> item =
> +    mItems->SetDataWithPrincipal(format, aData, aIndex, aPrincipal, false, false, rv);

write some comments about these 'false' values.

@@ +1304,3 @@
>    if (strcmp(aFormat, kUnicodeMime) == 0) {
> +    item = mItems->SetDataWithPrincipal(NS_LITERAL_STRING("text/plain"), nullptr,
> +                                        aIndex, aPrincipal, false, aHidden, rv);

if (NS_WARN_IF(rv.Failed())) {
  return rv.StealNSResult();
}

return NS_OK;

@@ +1309,5 @@
>  
>    if (strcmp(aFormat, kURLDataMime) == 0) {
> +    item = mItems->SetDataWithPrincipal(NS_LITERAL_STRING("text/uri-list"), nullptr,
> +                                        aIndex, aPrincipal, false, aHidden, rv);
> +    return rv.StealNSResult();

yeah. I prefer to see a warning here.

@@ +1316,5 @@
> +  nsAutoString format;
> +  GetRealFormat(NS_ConvertUTF8toUTF16(aFormat), format);
> +  item = mItems->SetDataWithPrincipal(format, nullptr, aIndex,
> +                                      aPrincipal, false, aHidden, rv);
> +  return rv.StealNSResult();

ditto.

@@ +1374,5 @@
>        dragSession->IsDataFlavorSupported(kFormats[f], &supported);
>        // if the format is supported, add an item to the array with null as
>        // the data. When retrieved, GetRealData will read the data.
>        if (supported) {
> +        CacheExternalData(kFormats[f], c, sysPrincipal, /* hidden = */ f != 0 && hasFileData);

/* hidden = */ f && hasFileData

@@ +1403,5 @@
>  
> +  // Check if the clipboard has any files
> +  bool hasFileData = false;
> +  const char *fileMime = kFileMime;
> +  clipboard->HasDataMatchingFlavors(&fileMime, 1, mClipboardType, &hasFileData);

It's exactly the same thing, but can you write it as:

const char* fileMime[] = { kFileMime };
clipboard->HasDataMatchingFlavors(fileMime, 1, ... ) ?

::: dom/events/DataTransferItem.h
@@ +106,5 @@
>      mIndex = aIndex;
>    }
>    void FillInExternalData();
>  
> +  bool ChromeOnly()

bool ChromeOnly() const

::: dom/events/DataTransferItemList.cpp
@@ +211,5 @@
>    // want to update an existing entry if it is already present, as per the spec.
>    RefPtr<DataTransferItem> item =
>      SetDataWithPrincipal(format, data, 0,
>                           nsContentUtils::SubjectPrincipal(),
> +                         true, false, aRv);

comment about this 'false'.

@@ +242,5 @@
>    uint32_t index = mIndexedItems.Length();
>    RefPtr<DataTransferItem> item =
>      SetDataWithPrincipal(type, data, index,
>                           nsContentUtils::SubjectPrincipal(),
> +                         true, false, aRv);

comment here too.
Attachment #8758433 - Flags: review?(amarchesini) → review+

Comment 43

a year ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c836acf3197
Part 1: Implement DataTransferItem and DataTransferItemList, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3615a839821
Part 2: Add support for images to DataTransfer, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/18b3c95f1a38
Part 3: Either expose files or strings generated by system drag or clipboard events to content, not both, r=baku
https://hg.mozilla.org/mozilla-central/rev/5c836acf3197
https://hg.mozilla.org/mozilla-central/rev/e3615a839821
https://hg.mozilla.org/mozilla-central/rev/18b3c95f1a38
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You left in a printf in DataTransferItem::FileFromISupports.
Flags: needinfo?(michael)
Blocks: 1278939
Blocks: 1278946
Depends on: 1279000
backed out from m-c and retrigged nightlys too
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 47

a year ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/6b9638327b6c
Backed out changeset 18b3c95f1a38 
https://hg.mozilla.org/mozilla-central/rev/f3ff62941927
Backed out changeset e3615a839821 
https://hg.mozilla.org/mozilla-central/rev/f8e3b81a79f4
Backed out changeset 5c836acf3197 on developer request by baku

Comment 48

a year ago
Hi, a Thunderbird developer here.

This breaks dragging an image from a file to a <contenteditable>, for example at http://www-archive.mozilla.org/editor/midasdemo/ or any Thunderbird compose window.

In fact, I just saw the debug "Creating a File from a nsIFile!" when it didn't work. It would be good if you could test this before landing the patch again.

I've tried with a Thunderbird Daily of 2016-06-10 (after the backout) and my image drag works again.

Why was this backed out?
Flags: needinfo?(amarchesini)
(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #48)
> Hi, a Thunderbird developer here.
> 
> This breaks dragging an image from a file to a <contenteditable>, for
> example at http://www-archive.mozilla.org/editor/midasdemo/ or any
> Thunderbird compose window.
> 
This was covered by bug 1278939.
Flags: needinfo?(amarchesini)

Comment 50

a year ago
Thanks, but that doesn't answer the question why this was backed out. Too many regressions?
Yes, including a crash regression.
Duplicate of this bug: 1166892
Any news here?

Comment 54

a year ago
Apparently bug 1278939 is ready for review which is blocking this bug here.
(Assignee)

Comment 55

a year ago
This patch is currently blocked on a patch in bug 1278939, which should make it possible to land this patch again.
Flags: needinfo?(michael)
(Assignee)

Comment 56

a year ago
Created attachment 8766010 [details] [diff] [review]
Part 1: Implement DataTransferItem and DataTransferItemList

Rebase
(Assignee)

Updated

a year ago
Attachment #8749371 - Attachment is obsolete: true
(Assignee)

Comment 57

a year ago
Created attachment 8766011 [details] [diff] [review]
Part 2: Add support for images to DataTransfer
(Assignee)

Updated

a year ago
Attachment #8749186 - Attachment is obsolete: true
(Assignee)

Comment 58

a year ago
Created attachment 8766012 [details] [diff] [review]
Part 3: Either expose files or strings generated by system drag or clipboard events to content, not both
(Assignee)

Updated

a year ago
Attachment #8758433 - Attachment is obsolete: true

Comment 59

a year ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/170fc45fb692
Part 1: Implement DataTransferItem and DataTransferItemList, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e50fc285df9
Part 2: Add support for images to DataTransfer, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/c207d5b6b8c4
Part 3: Either expose files or strings generated by system drag or clipboard events to content, not both, r=baku
https://hg.mozilla.org/mozilla-central/rev/170fc45fb692
https://hg.mozilla.org/mozilla-central/rev/3e50fc285df9
https://hg.mozilla.org/mozilla-central/rev/c207d5b6b8c4
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
Duplicate of this bug: 891247

Updated

a year ago
Depends on: 1287242

Updated

11 months ago
Duplicate of this bug: 1261586
Docs updated:

https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer

https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItem
https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItem/kind
https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItem/type
https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItem/getAsFile
https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItem/getAsString

https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItemList
https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItemList/length
https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItemList/add
https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItemList/remove
https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItemList/clear
https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItemList/DataTransferItem

And updated the Firefox 50 for developers page.

Filed bug 1302287 about issues with the quality and non-working state of many or all of the samples in this documentation.
Keywords: dev-doc-needed → dev-doc-complete

Updated

10 months ago
Depends on: 1305163

Updated

10 months ago
Depends on: 1308007

Updated

9 months ago
Blocks: 1312029

Updated

8 months ago
Blocks: 1317322

Comment 64

8 months ago
Chromium is trying to match that "DataTransferItemList#add() file argument should not be nullable". Could you please keep us informed with any reports of problems due to the update in DataTransfer.items.

Thanks
That's happening in https://codereview.chromium.org/2508773005/ but commenting here would suffice. Or email us. Thanks!

Updated

7 months ago
Depends on: 1322127
Depends on: 1336990

Updated

5 months ago
Depends on: 1342057
You need to log in before you can comment on or make changes to this bug.