Couldn't you just create a nsCOMArray<nsIDOMFile> from the nsIDOMFileList and call SetFiles() with the array in parameter? That would prevent creating another method with a name that I personally find confusing.
Here, you should do:
if (!dragEvent || !IsValidDropData(dragEvent)) {
return NS_OK;
}
It will save some indentation.
@@ +540,5 @@
> + aEvent->GetType(eventType);
> + if (eventType.EqualsLiteral("dragover") &&
> + IsValidDropData(dragEvent)) {
> + // Prevent default if we can accept this drag data
> + aEvent->PreventDefault();
And here, that could be:
if (eventType.EqualsLiteral("dragover")) {
aEvent->PreventDefault();
return NS_OK;
}
And BTW, why do you need to prevent default on dragover?
@@ +544,5 @@
> + aEvent->PreventDefault();
> + } else if (eventType.EqualsLiteral("drop")) {
> + // We probably shouldn't let any drop data propagate from a file
> + // upload control
> + aEvent->StopPropagation();
You don't need this method to be private it should actually be a class method (ie. static method). And you should return a |bool| instead of a |PRBool|.
Comment on attachment 534375
Patch 2
Review of attachment 534375: ------- ------- ------- ------- ------- ------- ------- ------- --
-------
f=mounir with the nits fixed.
Moving the review of the dragover/drop events handling to Neil.
::: content/ html/content/ src/nsHTMLInput Element. cpp AppendObject( file); aSetValueChange d);
@@ +1390,5 @@
> + mFiles.
> + }
> + }
> +
> + AfterSetFiles(
Couldn't you just create a nsCOMArray< nsIDOMFile> from the nsIDOMFileList and call SetFiles() with the array in parameter? That would prevent creating another method with a name that I personally find confusing.
::: content/ html/content/ src/nsHTMLInput Element. h NSIDOMHTMLELEME NT_NOFOCUSCLICK (nsGenericHTMLF ormElement: :)
@@ +148,5 @@
> +
> + // Forward nsIDOMHTMLElement
> + NS_FORWARD_
> + NS_IMETHOD Focus();
> + NS_IMETHOD Click();
This chunk doesn't seem to change something.
::: layout/ forms/nsFileCon trolFrame. cpp STATE(dragTarge t); >AddEventListen er(NS_LITERAL_ STRING( "drop") , >AddEventListen er(NS_LITERAL_ STRING( "dragover" ),
@@ +274,5 @@
> + NS_ENSURE_
> + dragTarget-
> + mMouseListener, PR_FALSE);
> + dragTarget-
> + mMouseListener, PR_FALSE);
IMO, it's weird to be able to drag-n-drop a file on the "Browse" button but I see that's what Safari and Chrome do.
@@ +534,5 @@ nsIDOMDragEvent > dragEvent = do_QueryInterfa ce(aEvent) ;
> + return NS_OK;
> + }
> +
> + nsCOMPtr<
> + if (dragEvent) {
Here, you should do: a(dragEvent) ) {
if (!dragEvent || !IsValidDropDat
return NS_OK;
}
It will save some indentation.
@@ +540,5 @@ >GetType( eventType) ; EqualsLiteral( "dragover" ) && (dragEvent) ) { >PreventDefault ();
> + aEvent-
> + if (eventType.
> + IsValidDropData
> + // Prevent default if we can accept this drag data
> + aEvent-
And here, that could be: EqualsLiteral( "dragover" )) { >PreventDefault ();
if (eventType.
aEvent-
return NS_OK;
}
And BTW, why do you need to prevent default on dragover?
@@ +544,5 @@ >PreventDefault (); EqualsLiteral( "drop") ) { >StopPropagatio n();
> + aEvent-
> + } else if (eventType.
> + // We probably shouldn't let any drop data propagate from a file
> + // upload control
> + aEvent-
Why?
@@ +551,5 @@ >PreventDefault (); >GetContent( );
> + aEvent-
> +
> + nsIContent* content = mFrame-
> + if (!content)
> + return NS_ERROR_FAILURE;
You can probably assert here instead of returning NS_ERROR_FAILURE.
@@ +555,5 @@ ent::FromConten t(content) ;
> + return NS_ERROR_FAILURE;
> +
> + nsHTMLInputElement* inputElement = nsHTMLInputElem
> + if (!inputElement)
> + return NS_ERROR_FAILURE;
For sure, you should assert here.
@@ +584,5 @@ Contains( NS_LITERAL_ STRING( "Files" ), &typeSupported);
> +
> + // We only support dropping files onto a file upload control
> + PRBool typeSupported;
> + types->
> + return typeSupported;
You can also get the file list and check if it's empty.
::: layout/ forms/nsFileCon trolFrame. h Frame.h" tener.h"
@@ +40,5 @@
>
> #include "nsBlockFrame.h"
> #include "nsIFormControl
> #include "nsIDOMMouseLis
> +#include "nsIDOMDragEvent.h"
Do the include in the cpp file and do a forward declaration in the header instead.
@@ +168,5 @@ ener: public MouseListener { ener(nsFileCont rolFrame* aFrame) : MouseListener( aFrame) {}; nsIDOMEvent* aMouseEvent);
>
> class BrowseMouseList
> public:
> BrowseMouseList
> + NS_IMETHOD MouseClick(
You broke the indentation here.
@@ +171,5 @@ ener(nsFileCont rolFrame* aFrame) : MouseListener( aFrame) {}; nsIDOMEvent* aMouseEvent); nsIDOMEvent* aEvent); (nsIDOMDragEven t* aEvent);
> BrowseMouseList
> + NS_IMETHOD MouseClick(
> + NS_IMETHOD HandleEvent(
> + private:
> + PRBool IsValidDropData
You don't need this method to be private it should actually be a class method (ie. static method). And you should return a |bool| instead of a |PRBool|.