Last Comment Bug 619355 - Need to replace contentAreaDD.js with droppedLinkHandler
: Need to replace contentAreaDD.js with droppedLinkHandler
Status: RESOLVED FIXED
: regression
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b2
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
: 560889 (view as bug list)
Depends on: 545714 560443 580710
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-15 09:15 PST by neil@parkwaycc.co.uk
Modified: 2010-12-17 12:36 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (14.98 KB, patch)
2010-12-15 09:26 PST, neil@parkwaycc.co.uk
jag-mozilla: review+
Details | Diff | Splinter Review
Alternative patch (14.98 KB, patch)
2010-12-15 16:10 PST, neil@parkwaycc.co.uk
jag-mozilla: review+
Details | Diff | Splinter Review
Fixed patch [Checked in: See comment 10+11] (15.01 KB, patch)
2010-12-16 01:47 PST, neil@parkwaycc.co.uk
jag-mozilla: review+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2010-12-15 09:15:08 PST
Bug 545714 effectively regressed browser drag-n-drop, because it was replaced with an internal handler that doesn't handle bookmark keywords and is being removed in bug 580710 anyway. We need to add our own dropped link handler.
Comment 1 neil@parkwaycc.co.uk 2010-12-15 09:26:31 PST
Created attachment 497791 [details] [diff] [review]
Proposed patch

Status bar drag'n'drop was implemented in bug 82965 (Mozilla 0.9.2/Netscape 6.1) and promptly broken 8 months later in bug 45605 (Mozilla 0.9.9/Netscape 7.0). I doubt we'll miss it. I think the sidebar drag probably went the same way.

I don't think the other two windows ever used content area drag'n'drop.
Comment 2 jag (Peter Annema) 2010-12-15 10:02:52 PST
Comment on attachment 497791 [details] [diff] [review]
Proposed patch

r=jag
Comment 3 jag (Peter Annema) 2010-12-15 10:57:39 PST
Hrm, Ratty made me think about this a bit more.

I'm not sure I like the getter/setter on tabbrowser. It implies the ability to dynamically change the handler, and since I'd expect the handler to be the same for all <browser>s, it should update it for all. I think rather than implement functionality we don't really need, how about instead you set |handleDroppedLink| as a field on <tabbrowser> and then from its constructor and |addTab()| method set the <browser>.droppedLinkHandler from the field?

Hrm, except I guess it's gonna be tricky to set the field before the constructor runs. Ideas for a solution along the lines outlined above? Other than just skipping the field and referring directly to a function in navigator.js ;-)
Comment 4 neil@parkwaycc.co.uk 2010-12-15 16:10:35 PST
Created attachment 497944 [details] [diff] [review]
Alternative patch

This version only sets the dropped link handler on the current browser.
Comment 5 jag (Peter Annema) 2010-12-15 16:31:15 PST
Comment on attachment 497944 [details] [diff] [review]
Alternative patch

r=jag

I think I do like this version better.
Comment 6 neil@parkwaycc.co.uk 2010-12-16 01:47:31 PST
Created attachment 498074 [details] [diff] [review]
Fixed patch
[Checked in: See comment 10+11]

The previous patch failed to update the dropped link handler when closing the current tab. This is because this.mCurrentBrowser is set to null before calling updateCurrentBrowser as an optimisation. This patch fixes that bug by using this.mCurrentTab.linkedBrowser instead.
Comment 7 Philip Chee 2010-12-16 02:03:21 PST
*** Bug 560889 has been marked as a duplicate of this bug. ***
Comment 8 Jens Hatlak (:InvisibleSmiley) 2010-12-16 02:45:27 PST
(In reply to comment #1)
> Status bar drag'n'drop was implemented in bug 82965 (Mozilla 0.9.2/Netscape
> 6.1) and promptly broken 8 months later in bug 45605 (Mozilla 0.9.9/Netscape
> 7.0). I doubt we'll miss it. I think the sidebar drag probably went the same
> way.

Sorry I don't understand, what's with the sidebar? Are we losing d&d ability to or from the sidebar? Or dragging the sidebar itself? :-/
Comment 9 neil@parkwaycc.co.uk 2010-12-16 03:25:53 PST
(In reply to comment #8)
> (In reply to comment #1)
> > Status bar drag'n'drop was implemented in bug 82965 (Mozilla 0.9.2/Netscape
> > 6.1) and promptly broken 8 months later in bug 45605 (Mozilla 0.9.9/Netscape
> > 7.0). I doubt we'll miss it. I think the sidebar drag probably went the same
> > way.
> Sorry I don't understand, what's with the sidebar? Are we losing d&d ability to
> or from the sidebar? Or dragging the sidebar itself? :-/
Prior to bug 45605, nsContentAreaDD handled draggesture, dragover and dragdrop events. The main browser used all three events; the statusbar was only a drop target while the sidebar was only a drag source. After bug 45605 the draggesture and dragover code was moved to C++. This meant that the sidebar and main browser drag sources were now automatic. The dragdrop code remained in nsContentAreaDD which allowed the main browser to keep working but the statusbar no longer worked as a drop target because it no longer had any dragover processing.

Bug 545714 moved more of the dragdrop code to C++/browser.xml, bypassing nsContentAreaDD. This doesn't affect the use of the sidebar as a drag source.
Comment 10 jag (Peter Annema) 2010-12-16 03:36:37 PST
Comment on attachment 498074 [details] [diff] [review]
Fixed patch
[Checked in: See comment 10+11]

Looks good, but I'd add the comment about why you're using mCurrentTab.linkedBrowser instead of mCurrentBrowser (though the null check on the next line does kinda hint at it).
Comment 11 neil@parkwaycc.co.uk 2010-12-16 15:38:24 PST
Pushed changeset 85faf50deb16 to comm-central.

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