Closed Bug 789781 Opened 8 years ago Closed 8 years ago

Switch to new drag and drop api in tabbrowser

Categories

(SeaMonkey :: Tabbed Browser, defect)

defect
Not set

Tracking

(seamonkey2.15 fixed)

RESOLVED FIXED
seamonkey2.15
Tracking Status
seamonkey2.15 --- fixed

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Make the switch (obsolete) — Splinter Review
We're still using the old drag and drop API in tabbrowser, so this patch fixes that
Attachment #659564 - Flags: review?(neil)
Comment on attachment 659564 [details] [diff] [review]
Make the switch

When I tried this out it didn't want to show me the indicator for dropping a link between two tabs. (I didn't actually complete the drag to see whether it would create a new tab or not.)

>-  <script type="application/javascript" src="chrome://global/content/nsDragAndDrop.js"/>
Woohoo! [Is there a bug on removing nsDragAndDrop.js?]

>+              dt.mozSetDataAt("application/x-moz-node", target, 0);
Not sure why Firefox's tabbrowser does this, navigatorDD.js for example uses .mozSourceNode instead.

>+              if (URI) {
Why this change?

>+                dt.mozSetDataAt("text/uri-list", spec + "\n" + title, 0);
IIRC URI lists don't include titles.

>+              if (newIndexBetween == tabIndex ||
>+                  newIndexBetween == tabIndex + 1) {
[Does this not fit on one line?]

>+                url = Services.droppedLinkHandler.dropLink(aEvent, {}, true);
[tabbrowser.xml doesn't normally use Services]

>+              else if (draggedTab.sourceDocument &&
>+                       draggedTab.sourceDocument.defaultView.top == content) {
Did you mean aEvent.dataTransfer.mozSourceDocument?
Attachment #659564 - Flags: review?(neil) → review-
Assignee: nobody → iann_bugzilla
Status: NEW → ASSIGNED
Attached patch Revised switch (obsolete) — Splinter Review
Fixed review comments where possible.
I had spotted the missing drop indicator when I first wrote the patch and had worked out why but forgot about it when submitting. Anyway should be fixed now.

As for removal, bug 719188 mentions flagging nsDragAndDrop.js and nsTransferable.js as deprecated but no confirmation of if and when.
Attachment #659564 - Attachment is obsolete: true
Attachment #659603 - Flags: review?(neil)
Comment on attachment 659603 [details] [diff] [review]
Revised switch

Not tested yet.

>+              dt.mozSetDataAt("application/x-moz-node", target, 0);
[???]

>+            var dt = aEvent.dataTransfer;
>+            var draggedTab = dt.mozSourceNode;
Nit: move these to where you use them.

>+            var within = draggedTab &&
>+                         draggedTab.parentNode == this.tabContainer;
...
>+            var within = draggedTab &&
>+                         draggedTab.parentNode == this.tabContainer;
Oops.
Fixed duplication and missing removal.
var dt lines moved closer to use.
Attachment #659603 - Attachment is obsolete: true
Attachment #659603 - Flags: review?(neil)
Attachment #660006 - Flags: review?(neil)
Attachment #660006 - Flags: review?(neil) → review+
Comment on attachment 660006 [details] [diff] [review]
Switch with less within [Checked in: Comment 5]

http://hg.mozilla.org/comm-central/rev/ab06a84a717a
Attachment #660006 - Attachment description: Switch with less within → Switch with less within [Checked in: Comment 5]
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Flags: in-qa-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.15
You need to log in before you can comment on or make changes to this bug.