Last Comment Bug 673080 - Dropping a link on Firefox from an external program has no effect.
: Dropping a link on Firefox from an external program has no effect.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: All Other
: -- normal with 1 vote (vote)
: mozilla9
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on:
Blocks: 357601 475045 673157
  Show dependency treegraph
 
Reported: 2011-07-21 04:37 PDT by ebrahim
Modified: 2011-09-02 08:50 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Drag and Drop from Gtalk to Firefox (147.40 KB, image/png)
2011-07-21 04:37 PDT, ebrahim
no flags Details
Drag and Drop to search (22.54 KB, image/png)
2011-07-21 10:06 PDT, ebrahim
no flags Details
Fix for dragging link only elements onto any Win32 Widgets (8.17 KB, patch)
2011-07-25 08:01 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Fix for dragging link only elements onto any Win32 Widgets + review feedback (8.57 KB, patch)
2011-07-25 10:35 PDT, Brian R. Bondy [:bbondy]
neil: review+
Details | Diff | Splinter Review
Fix for dragging link only elements onto any Win32 Widgets + review feedback (8.58 KB, patch)
2011-07-26 21:20 PDT, Brian R. Bondy [:bbondy]
neil: review+
Details | Diff | Splinter Review
Fix for dragging link only elements onto any Win32 Widgets + review feedback (8.56 KB, patch)
2011-08-08 06:18 PDT, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Splinter Review

Description ebrahim 2011-07-21 04:37:08 PDT
Created attachment 547367 [details]
Drag and Drop from Gtalk to Firefox

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:8.0a1) Gecko/20110720 Firefox/8.0a1
Build ID: 20110720030844

Steps to reproduce:

I wanted to drag a link from Gtalk or Yahoo Messenger and drop it on Firefox.


Actual results:

Firefox can not accept this drop link and nothing happens.


Expected results:

Accepting drag and drop links from external application. This is supported on Chrome and IE.
Comment 1 Brian R. Bondy [:bbondy] 2011-07-21 09:13:23 PDT
Confirmed, with IE you can drop the link anywhere.
With Chrome you can drop the link on the address bar or bookmarks bar.

I like the feature of being able to place a link directly in the bookmarks bar or loading it by dragging it into the URL bar so I think I will go that route.
Comment 2 ebrahim 2011-07-21 09:49:33 PDT
Oh, Also drag text (from an external application or a webpage inside firefox) and drop to search can be cool. Thank you
Comment 3 Brian R. Bondy [:bbondy] 2011-07-21 09:59:06 PDT
The drag text you can already do.
Comment 4 ebrahim 2011-07-21 10:06:23 PDT
Created attachment 547428 [details]
Drag and Drop to search

Oh, Yes, But I mean to tab bar for searching, Thanks again
Comment 5 Brian R. Bondy [:bbondy] 2011-07-21 10:17:10 PDT
Please file another bug for that one.  It currently tries to load any text as a URL even if it is not a URI.  Please CC me on it.
Comment 6 Brian R. Bondy [:bbondy] 2011-07-25 07:04:42 PDT
I moved the task for bookmarking non hyperlinked URLs via drag and drop into its own bug since it is not directly related to this task's Win32 widget code fix that is coming. It was moved here: Bug 673912 - Dragging non hyperlinked URLs on the bookmark bar should produce a bookmark
Comment 7 Brian R. Bondy [:bbondy] 2011-07-25 08:01:13 PDT
Created attachment 548185 [details] [diff] [review]
Fix for dragging link only elements onto any Win32 Widgets

The problem was that we didn't handle any item that was being dragged which didn't support "move" and didn't support "copy", but did support "link".
Comment 8 neil@parkwaycc.co.uk 2011-07-25 09:34:32 PDT
Comment on attachment 548185 [details] [diff] [review]
Fix for dragging link only elements onto any Win32 Widgets

>+  else if(!mCanMove && !mCanCopy && mCanLink) {
Nit: space between if and (

>   mCanMove = (*pdwEffect) & DROPEFFECT_MOVE;
Ooh, naughty, putting an int into a PRBool. None of our tools have spotted this because they only run on Linux and this is Windows-only code.

It occurs to me that rather than storing three separate boolean values, you could simply store the drop effect in an int member and test its bits later.

>+  if (pIDataSource) {
>+    mCanLink = (S_OK == ::OleQueryLinkFromData(pIDataSource) ? PR_TRUE : PR_FALSE);
I would only bother doing this if we don't already know we can link.
Comment 9 Brian R. Bondy [:bbondy] 2011-07-25 10:35:22 PDT
Created attachment 548214 [details] [diff] [review]
Fix for dragging link only elements onto any Win32 Widgets + review feedback

Thanks for the quick review, implemented the review comments.
Comment 10 neil@parkwaycc.co.uk 2011-07-26 14:46:22 PDT
Comment on attachment 548214 [details] [diff] [review]
Fix for dragging link only elements onto any Win32 Widgets + review feedback

>     mMovePreferred = (preferredEffect & DROPEFFECT_MOVE) != 0;
>   }
>   else
>-    mMovePreferred = mCanMove;
>+    mMovePreferred = (mEffect & DROPEFFECT_MOVE);
Needs to be != 0 as above, since mMovePreferred is a boolean (unless you want to turn that into a WORD too...) r=me with that fixed.
Comment 11 Brian R. Bondy [:bbondy] 2011-07-26 21:20:22 PDT
Created attachment 548685 [details] [diff] [review]
Fix for dragging link only elements onto any Win32 Widgets + review feedback

Oops fixed, I'm used to bool/implicit conversions.
Comment 12 neil@parkwaycc.co.uk 2011-08-04 02:56:17 PDT
(In reply to comment #11)
> I'm used to bool/implicit conversions.
I don't think we go in for implicit bool conversions anyway; they generate compiler warnings in at least one of the compilers we use.
Comment 13 Marco Bonardo [::mak] 2011-08-04 03:01:22 PDT
I suggest to keep using checkin-needed keyword to ask for checkin, a lot of developers are not yet tracking checkin? flag.
Comment 14 Dão Gottwald [:dao] 2011-08-05 13:06:31 PDT
I may be wrong, but it seems to me that the commit message's grammar is completely broken...
Comment 15 Brian R. Bondy [:bbondy] 2011-08-05 13:11:01 PDT
It was the same as the subject of a bug, but I agree.
Do you need me to submit a new patch with it fixed?

How about:
Bug 673080 - Dropping a link on Firefox from an external program has no effect.
Comment 16 Brian R. Bondy [:bbondy] 2011-08-05 13:47:41 PDT
Comment on attachment 548685 [details] [diff] [review]
Fix for dragging link only elements onto any Win32 Widgets + review feedback

Going to run through try before checkin flag
Comment 17 ebrahim 2011-08-05 17:21:27 PDT
Sorry :)
Comment 18 Brian R. Bondy [:bbondy] 2011-08-08 06:13:43 PDT
ebraminio: No worries, thanks for posting!
Comment 19 Brian R. Bondy [:bbondy] 2011-08-08 06:18:29 PDT
Created attachment 551441 [details] [diff] [review]
Fix for dragging link only elements onto any Win32 Widgets + review feedback

Updated commit message
Comment 20 Brian R. Bondy [:bbondy] 2011-08-31 13:37:53 PDT
This was pushed to try, results in progress:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=cde44aaad760
Comment 21 Brian R. Bondy [:bbondy] 2011-09-01 07:21:02 PDT
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/89c47cdfad0a
Comment 22 Ed Morley [:emorley] 2011-09-01 13:53:34 PDT
http://hg.mozilla.org/mozilla-central/rev/89c47cdfad0a

Note You need to log in before you can comment on or make changes to this bug.