Closed Bug 774125 Opened 8 years ago Closed 7 years ago

Switch to new drag and drop api for home button

Categories

(SeaMonkey :: Location Bar, defect)

defect
Not set

Tracking

(seamonkey2.14 fixed)

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

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Switch home button (obsolete) — Splinter Review
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.
Attachment #642437 - Flags: review?(neil)
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.
Attachment #642437 - Flags: review?(neil) → review-
Blocks: 773979
No longer depends on: 773979
Attached patch Less complex version (obsolete) — Splinter Review
Changes since last version:
* Unneeded helpers removed.
* Remaining helper renamed.
Attachment #642437 - Attachment is obsolete: true
Attachment #642588 - Flags: review?(neil)
Blocks: 774362
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 :-(
Attachment #642588 - Flags: review?(neil) → review-
Attached patch With less nsPreferences (obsolete) — Splinter Review
Changes since last version:
* Remove the two uses of nsPreferences.
* Stop Home button from dropping on itself.
Attachment #642588 - Attachment is obsolete: true
Attachment #642995 - Flags: review?(neil)
> -    nsPreferences.setUnicharPref("browser.startup.homepage", aURL);
> +    Services.prefs.setCharPref("browser.startup.homepage", aURL);
Urgh, I think you want GetLocalizedStringPref() here.
(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 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]
(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.
Changes since last version:
* Use SetStringPref to set a complex value.
Attachment #642995 - Attachment is obsolete: true
Attachment #642995 - Flags: review?(neil)
Attachment #647364 - Flags: review?(neil)
Attachment #647364 - Flags: review?(neil) → review+
Comment on attachment 647364 [details] [diff] [review]
With SetStringPref [Checked in: Comment 10]

http://hg.mozilla.org/comm-central/rev/ca8eda5e95fd
Attachment #647364 - Attachment description: With SetStringPref → With SetStringPref [Checked in: Comment 10]
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Flags: in-qa-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.14
You need to log in before you can comment on or make changes to this bug.