(In reply to comment #54)
> Comment on attachment 534375 [details] [review]
> Patch 2
> 
> Review of attachment 534375 [details] [review]:
> -----------------------------------------------------------------
> 
> 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.

I don't know about this, wouldn't it be faster to simply use the existing list rather than allocate a new one? Also, I don't see what's confusing about the name especially since it's a private method.

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

I know. I couldn't figure it out either and couldn't get rid of it.

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

Bigger drop area = better usability :)

> @@ +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;
> }

Will do.

> And BTW, why do you need to prevent default on dragover?

Because that's what other drop targets seem to do (like the browser), and apparently (looking at synthesizeDrop() in the test harness) if a dragover event is not consumed, that is interpreted as having nothing that takes a drop at that point.

> @@ +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?

I couldn't think of a good reason to allow it, and thought for security reasons it's best to not let websites find out what you drag in there immediately upon drop.

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

OK.

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

Wouldn't that be taken care of already? If we get an empty file list, we set the file upload control blank, and I think that's a good behaviour.

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

OK.

I'd like some more opinion of what I discussed here, otherwise I'll fix what I can after everyone else's reviews.