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/nsHTMLInputElement.cpp
@@ +1390,5 @@
> + mFiles.AppendObject(file);
> + }
> + }
> +
> + AfterSetFiles(aSetValueChanged);
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/nsHTMLInputElement.h
@@ +148,5 @@
> +
> + // Forward nsIDOMHTMLElement
> + NS_FORWARD_NSIDOMHTMLELEMENT_NOFOCUSCLICK(nsGenericHTMLFormElement::)
> + NS_IMETHOD Focus();
> + NS_IMETHOD Click();
This chunk doesn't seem to change something.
::: layout/forms/nsFileControlFrame.cpp
@@ +274,5 @@
> + NS_ENSURE_STATE(dragTarget);
> + dragTarget->AddEventListener(NS_LITERAL_STRING("drop"),
> + mMouseListener, PR_FALSE);
> + dragTarget->AddEventListener(NS_LITERAL_STRING("dragover"),
> + 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 @@
> + return NS_OK;
> + }
> +
> + nsCOMPtr<nsIDOMDragEvent> dragEvent = do_QueryInterface(aEvent);
> + if (dragEvent) {
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();
Why?
@@ +551,5 @@
> + aEvent->PreventDefault();
> +
> + nsIContent* content = mFrame->GetContent();
> + if (!content)
> + return NS_ERROR_FAILURE;
You can probably assert here instead of returning NS_ERROR_FAILURE.
@@ +555,5 @@
> + return NS_ERROR_FAILURE;
> +
> + nsHTMLInputElement* inputElement =
> nsHTMLInputElement::FromContent(content);
> + if (!inputElement)
> + return NS_ERROR_FAILURE;
For sure, you should assert here.
@@ +584,5 @@
> +
> + // We only support dropping files onto a file upload control
> + PRBool typeSupported;
> + types->Contains(NS_LITERAL_STRING("Files"), &typeSupported);
> + return typeSupported;
You can also get the file list and check if it's empty.
::: layout/forms/nsFileControlFrame.h
@@ +40,5 @@
>
> #include "nsBlockFrame.h"
> #include "nsIFormControlFrame.h"
> #include "nsIDOMMouseListener.h"
> +#include "nsIDOMDragEvent.h"
Do the include in the cpp file and do a forward declaration in the
header instead.
@@ +168,5 @@
>
> class BrowseMouseListener: public MouseListener {
> public:
> BrowseMouseListener(nsFileControlFrame* aFrame) : MouseListener(aFrame)
> {};
> + NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent);
You broke the indentation here.
@@ +171,5 @@
> BrowseMouseListener(nsFileControlFrame* aFrame) : MouseListener(aFrame)
> {};
> + NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent);
> + NS_IMETHOD HandleEvent(nsIDOMEvent* aEvent);
> + private:
> + PRBool IsValidDropData(nsIDOMDragEvent* aEvent);
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|.
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/131145
Title:
Dragging icon from Nautilus to HTML File Input box does not work
--
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
