Comment 68 for bug 131145

Revision history for this message
In , Mounir-lamouri (mounir-lamouri) wrote :

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