774125 - Switch to new drag and drop api for home button

Status: RESOLVED FIXED

(SeaMonkey :: Location Bar, defect)

Description

[Ian Neal]

Attachment: Switch home button

This patch expands on the helper introduced in bug 773983 and changes the home button to use the new API. I may have been over generous with the number of aEvent.stopPropagation(); entries.

Comment 1

[[e-mail]]

Comment on attachment 642437 [details] [diff] [review]
Switch home button

Which other consumer(s) are you expecting to use this?

>+  canDropLink: function (aEvent)
>+  {
>+    return Services.droppedLinkHandler.canDropLink(aEvent, true);
>+  },
Hardly seems worth it.

>+  drop: function (aEvent, aName, aDisallowInherit)
>+  {
>+    return Services.droppedLinkHandler
>+                   .dropLink(aEvent, aName, aDisallowInherit);
>   }
Ditto.

>+  dragOver: function (aEvent, aStatusString)
>+  {
>+    if (this.canDropLink(aEvent))
>+      aEvent.preventDefault();
>+
>+    if (aStatusString)
>+    {
>+      var statusTextFld = document.getElementById("statusbar-display");
>+      statusTextFld.label = gNavigatorBundle.getString(aStatusString);
>+    }
>+  },
...
>+  onDragExit: function (aEvent)
>+  {
>+    aEvent.stopPropagation();
>+    document.getElementById("statusbar-display").label = "";
>+  }
This separates the status updating, which makes the code confusing.

Comment 2

[Ian Neal]

Attachment: Less complex version

Changes since last version:
* Unneeded helpers removed.
* Remaining helper renamed.

Comment 3

[[e-mail]]

Comment on attachment 642588 [details] [diff] [review]
Less complex version

>+function BeginDragOver(aEvent)
You don't begin a drag over another element.
Call it DragLinkOver perhaps?

>+  if (Services.droppedLinkHandler.canDropLink(aEvent, true))
Nice idea.

>+    var homepage = nsPreferences.getLocalizedUnicharPref("browser.startup.homepage", "about:blank");
I knew there was something I forgot to mention.
We should stop using nsPreferences and use one of our other APIs.

>-      if (aEvent.target == aDragSession.dataTransfer.mozSourceNode)
>-        {
>-          aDragSession.dragAction = nsIDragService.DRAGDROP_ACTION_NONE;
>-          return;
>-        }
Unfortunately we lost this so the patch lets you drop the home button on itself :-(

Comment 4

[Ian Neal]

Attachment: With less nsPreferences

Changes since last version:
* Remove the two uses of nsPreferences.
* Stop Home button from dropping on itself.

Comment 5

[Philip Chee]

> -    nsPreferences.setUnicharPref("browser.startup.homepage", aURL);
> +    Services.prefs.setCharPref("browser.startup.homepage", aURL);
Urgh, I think you want GetLocalizedStringPref() here.

Comment 6

[Ian Neal]

(In reply to Philip Chee from comment #5)
> > -    nsPreferences.setUnicharPref("browser.startup.homepage", aURL);
> > +    Services.prefs.setCharPref("browser.startup.homepage", aURL);
> Urgh, I think you want GetLocalizedStringPref() here.

Eh? We need to set a pref not get one!

Comment 7

[[e-mail]]

Comment on attachment 642995 [details] [diff] [review]
With less nsPreferences

>-      var homepage = nsPreferences.getLocalizedUnicharPref("browser.startup.homepage", "about:blank");
[Note to self: is default value reasonable?]

>+      // XXX find a readable title string for homepage,
>+      // perhaps do a history lookup.
[Sadly we probably can't do that any more because history is async]

>+    Services.prefs.setCharPref("browser.startup.homepage", aURL);
[Note to self: try a unicode URL]

Comment 8

[[e-mail]]

(In reply to comment #7)
> >-      var homepage = nsPreferences.getLocalizedUnicharPref("browser.startup.homepage", "about:blank");
> [Note to self: is default value reasonable?]
Yes it is :-)

> >+    Services.prefs.setCharPref("browser.startup.homepage", aURL);
> [Note to self: try a unicode URL]
Oops, this doesn't quite work. If you have a the URL http://☃.net/ then dragging the link will work because the link target is actually http://xn--n3h.net/ and therefore ASCII. But unfortunately dragging the text ☃.net on its own used to work with the old code, but doesn't with your patch.

Comment 9

[Ian Neal]

Attachment: With SetStringPref [Checked in: Comment 10]

Changes since last version:
* Use SetStringPref to set a complex value.

Comment 10

[Ian Neal]

Comment on attachment 647364 [details] [diff] [review]
With SetStringPref [Checked in: Comment 10]

http://hg.mozilla.org/comm-central/rev/ca8eda5e95fd