yugang Reporter
	Description • 11 years ago

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

	Loic
	Comment 1 • 11 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

	The 8472
	Comment 2 • 10 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.

	[ex-Mozilla] Agi Sferro | :agi
	Comment 3 • 10 years ago

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.

	[ex-Mozilla] Agi Sferro | :agi
	Comment 4 • 10 years ago

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.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 5 • 9 years ago

I'm going to take this over and try to fix it up along with bug 891247

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 6 • 9 years ago

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)

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 7 • 9 years ago

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.

	Andrea Marchesini [:baku]
	Comment 8 • 9 years ago

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.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 9 • 9 years ago

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.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 10 • 9 years ago

Updated - for changes see V2 Interdiff.

	Andrea Marchesini [:baku]
	Comment 11 • 9 years ago

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 >

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 12 • 9 years ago

New interdiff - patch to follow

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 13 • 9 years ago

(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 :)

	Andrea Marchesini [:baku]
	Comment 14 • 9 years ago

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

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 16 • 9 years ago

Had to rebase - waiting on try to merge: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b361f308cbf1

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 17 • 9 years ago

There's a memory leak in the impl which is caused by nsVariant not being cycle collected. I'm blocking on that.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 18 • 9 years ago

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.

	Andrea Marchesini [:baku]
	Comment 19 • 9 years ago

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?

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 20 • 9 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.

	Andrea Marchesini [:baku]
	Comment 21 • 9 years ago

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?

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 22 • 9 years ago

(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.

	Andrea Marchesini [:baku]
	Comment 23 • 9 years ago

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.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 24 • 9 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.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 25 • 9 years ago

try for rebased patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ceb1b8acf223

	Andrea Marchesini [:baku]
	Comment 27 • 8 years ago

:mystor, can I steal this bug?

	Andrea Marchesini [:baku]
	Comment 28 • 8 years ago

The reason is that I need to have this implemented for the Directory Upload API.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 29 • 8 years ago

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

As I said on IRC, yes, go ahead.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 30 • 8 years ago

I'm so looking forward to finally landing this and no longer dealing with how much it's bitrotted.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 31 • 8 years ago

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.

	Andrea Marchesini [:baku]
	Comment 32 • 8 years ago

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

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 35 • 8 years ago

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.

	Andrea Marchesini [:baku]
	Comment 36 • 8 years ago

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.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 37 • 8 years 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.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 38 • 8 years ago

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).

	mmanela
	Comment 39 • 8 years ago

@Michael, thanks addressing this bug. Do you have any sense when it will make it into the product?

	Andrea Marchesini [:baku]
	Comment 40 • 8 years ago

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 }

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 41 • 8 years ago

Changes to patch to accomodate changes to DataTransfer during the last month

	Andrea Marchesini [:baku]
	Comment 42 • 8 years ago

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.

	Pulsebot
	Comment 43 • 8 years 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

	Carsten Book [:Tomcat]
	Comment 44 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5c836acf3197
https://hg.mozilla.org/mozilla-central/rev/e3615a839821
https://hg.mozilla.org/mozilla-central/rev/18b3c95f1a38

	Neil Deakin
	Comment 45 • 8 years ago

You left in a printf in DataTransferItem::FileFromISupports.

	Carsten Book [:Tomcat]
	Comment 46 • 8 years ago

backed out from m-c and retrigged nightlys too

	Pulsebot
	Comment 47 • 8 years 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

	Jorg K (CEST = GMT+2)
	Comment 48 • 8 years 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?

	Neil Deakin
	Comment 49 • 8 years ago

(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.

	Jorg K (CEST = GMT+2)
	Comment 50 • 8 years ago

Thanks, but that doesn't answer the question why this was backed out. Too many regressions?

	Neil Deakin
	Comment 51 • 8 years ago

Yes, including a crash regression.

	Andrea Marchesini [:baku]
	Comment 53 • 8 years ago

Any news here?

	Jorg K (CEST = GMT+2)
	Comment 54 • 8 years ago

Apparently bug 1278939 is ready for review which is blocking this bug here.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 55 • 8 years ago

This patch is currently blocked on a patch in bug 1278939, which should make it possible to land this patch again.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 56 • 8 years ago

Rebase

	Pulsebot
	Comment 59 • 8 years 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

	Carsten Book [:Tomcat]
	Comment 60 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/170fc45fb692
https://hg.mozilla.org/mozilla-central/rev/3e50fc285df9
https://hg.mozilla.org/mozilla-central/rev/c207d5b6b8c4

	Eric Shepherd [:sheppy]
	Comment 63 • 8 years ago

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.

	Luna Lu
	Comment 64 • 8 years 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

	Philip Jägenstedt [:foolip]
	Comment 65 • 8 years ago

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