456048 - Use the new D&D API in tabbrowser

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Asaf Romano (gone)]

Attachment: patch

Use the new D&D API in tabbrowser

Next -  places; next -  extend drop target; next - actual work on detach tabs!

Comment 1

[Neil Deakin]

Comment on attachment 339468 [details] [diff] [review]
patch

-                  ondraggesture="nsDragAndDrop.startDrag(event, this.parentNode.parentNode); event.stopPropagation();"
-                  ondragover="nsDragAndDrop.dragOver(event, this.parentNode.parentNode); event.stopPropagation();"
-                  ondragdrop="nsDragAndDrop.drop(event, this.parentNode.parentNode); event.stopPropagation();"
-                  ondragexit="nsDragAndDrop.dragExit(event, this.parentNode.parentNode); event.stopPropagation();">
+                  ondragstart="this.parentNode.parentNode._onDragStart(event); event.stopPropagation();"
+                  ondragover="this.parentNode.parentNode._onDragOver(event); event.stopPropagation();"
+                  ondragdrop="this.parentNode.parentNode._onDragDrop(event); event.stopPropagation();"
+                  ondragleave="this.parentNode.parentNode._onDragLeave(event); event.stopPropagation();">

The new api uses 'ondrop', not 'ondragdrop'.

-              aXferData.data.addDataForFlavour("text/x-moz-url", URI.spec + "\n" + aEvent.target.label);
-              aXferData.data.addDataForFlavour("text/unicode", URI.spec);
-              aXferData.data.addDataForFlavour("text/html", '<a href="' + URI.spec + '">' + aEvent.target.label + '</a>');
+              dt.mozSetDataAt("text/x-moz-url", URI.spec + "\n" + aEvent.target.label, 0);
+              dt.mozSetDataAt("text/unicode", URI.spec, 0);
+              dt.mozSetDataAt("text/html", '<a href="' + URI.spec + '">' + aEvent.target.label + '</a>', 0);
             } else {
-              aXferData.data.addDataForFlavour("text/unicode", "about:blank");
+              dt.mozSetDataAt("text/unicode", "about:blank", 0);

Use text/plain instead of text/unicode. We also want to be able to support the spec type for urls: text/uri-list, but not a big deal for now.

+      <method name="_setEffectAllowedForDataTransfer">
...
-              return false;
-            return true;
+            var dt = aEvent.dataTransfer

add semicolon here in case the line gets moved.

+            // tabs are alwasy added as the first type
 
spelling: always

+            if (types[0] == "application/x-moz-node") {
+              var sourceNode = dt.mozGetDataAt("application/x-moz-node", 0);
+              if (sourceNode instanceof Ci.nsIDOMXULElement &&
+                  sourceNode.localName == "tab" &&
+                  (sourceNode.parentNode == this.mTabContainer ||
+                   (sourceNode.ownerDocument.defaultView instanceof ChromeWindow &&
+                    sourceNode.ownerDocument.documentElement.getAttribute("windowtype") == "navigator:browser"))) {

You could just call mozGetDataAt without the earlier condition, as it will return null if the data isn't present rather than even checking the type first.

Also 'sourceNode instanceof XULElement' is sufficient.

+              var url;
+              var types = dt.mozTypesAt(0);
+              for (var i=0; i < this._supportedLinkDropTypes.length; i++) {
+                if (types.contains(this._supportedLinkDropTypes[i])) {
+                  var urlData = dt.mozGetDataAt(this._supportedLinkDropTypes[i], 0);
+                  url = transferUtils.retrieveURLFromData(urlData, this._supportedLinkDropTypes[i]);
+                  break;

I'd cache this._supportedLinkDropTypes[i] in another variable, as you use it three times.

-              if (document.getBindingParent(aEvent.originalTarget).localName != "tab" || accelKeyPressed) {
+              if (document.getBindingParent(aEvent.originalTarget).localName != "tab" || dtopEffect == "copy") {

spelling here: dropEffect

Comment 2

[Asaf Romano (gone)]

Attachment: patch

Comment 3

[Asaf Romano (gone)]

Attachment: patch

Comment 4

[Asaf Romano (gone)]

Attachment: patch

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 341083 [details] [diff] [review]
patch

ok, let's get this done

Comment 6

[Jim Jeffery not reading bug-mail 1/2/11]

Did this break d&d causing crashes:
http://crash-stats.mozilla.com/report/index/987427ae-8fc2-11dd-a81c-001cc4e2bf68?p=1

Also reported in the Builds forums.  

Also seeing an Assert dragging a link to the tab-bar:
ASSERT: copy or move action without a tab
Stack Trace: 
0:_onDrop([object DragEvent])
1:ondrop([object DragEvent])

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20081001 Minefield/3.1b1pre ID:20081001053411

Vista HP SP1

Comment 7

[Jim Jeffery not reading bug-mail 1/2/11]

The Assert seems to be caused by TabMix Plus add-on - the d&d crash is still valid on a virgin profile.

http://crash-stats.mozilla.com/report/index/2a6b1bfd-8fc5-11dd-87c2-001cc4e2bf68

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #6)
Why do you add keywords and request blocking here? I suggest you file a new bug.

Comment 9

[Jim Jeffery not reading bug-mail 1/2/11]

because this bug is still open and landed yesterday ?  

If this bug was 'closed/fixed' I would have filed a new bug...Obviously its not 'fixed' - and has broken drag&drop - a bad thing since we are frozen for B1..

Comment 10

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/fa472fbd399e

Comment 11

[Tony Chung [:tchung]]

hi, can we get testcases for this patch?  Also, what can be done manually to verify it, other than simple tab browsing tests?

Comment 12

[Tony Chung [:tchung]]

one more ping in here, looking for testcases.  thanks.

Comment 13

[Asaf Romano (gone)]

You cannot test this any other way you didn't test the old tab d&d code with.