251775 - [FIX]Remove direct drag-drop knowledge from nsFrame::HandlePress

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 240903 comment 23.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153466 [details] [diff] [review]
Proposed patch 

biesi, jst, what do you think?

Comment 3

[Boris Zbarsky [:bzbarsky]]

That patch fixes bug 244859 too.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 153466 [details] [diff] [review]
Proposed patch 

thanks!

Index: content/base/public/nsContentUtils.h

+   * @return the nsIImage corresponding to the first frame of the image
+   */
+  static nsresult GetImageFromContent(nsIImageLoadingContent* aContent,
+				       nsIImage** aImage);

+   * @return whether it's draggable
+   */
+  static PRBool ContentIsDraggable(nsIContent* aContent) {

This is a bit inconsistent. the @return refers to two different things here...
why not use something like:
@param [out]
in the first case?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> This is a bit inconsistent. the @return refers to two different things here...

That seems to be the style of the file, basically...

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 153466 [details] [diff] [review]
Proposed patch 

content/base/src/nsContentUtils.cpp
+  return CallGetInterface(ir.get(), aImage);

is the .get() needed?

+      nsCOMPtr<nsIURI> absURI;

what's this COMptr for?

content/base/src/nsContentAreaDragDrop.cpp

+  NS_PRECONDITION(outParent, "Null out param?");

I kinda find these useless, but ok.

 void
 nsTransferableFactory::FindParentLinkNode(nsIDOMNode* inNode, 
					   nsIDOMNode** outParent)

maybe this should return an already_AddRefed, instead of using an out param

Comment 7

[Boris Zbarsky [:bzbarsky]]

> is the .get() needed?

Yes, if you want it to compile.

> +      nsCOMPtr<nsIURI> absURI;
> what's this COMptr for?

Oops.  That's from an earlier patch iteration.  Fixed.

> maybe this should return an already_AddRefed, instead of using an out param

Hmm.. yeah, I could do that.  There're just a _lot_ of callsites...

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 153466 [details] [diff] [review]
Proposed patch 

- In content/base/public/nsContentUtils.h:

+   * Method to get an nsIImage from an image loading content
+   *
+   * @param aContent The image loading content.  Must not be null.
+   * @return the nsIImage corresponding to the first frame of the image
+   */
+  static nsresult GetImageFromContent(nsIImageLoadingContent* aContent,
+				       nsIImage** aImage);

Seems like this could return already_AddRefed().

sr=jst with that and the other comments addressed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch with comments addressed

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed.