Closed Bug 384139 Opened 12 years ago Closed 12 years ago

move openURL() to a reusable place in toolkit

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: kairo, Assigned: kairo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

The openURL() function used by EM to open "get new themes" etc. should probably move to a globally accessible place in toolkit so that it can be reused in other places where we need to open a URL but don't know if we're a browser ourself.
We have such cases in all sorts of shared code in toolkit and also shared mailnews, for example.

Having once function to use at several places eases life for all those consumers.
I suggest globalOverlay.js since extensions.xul already imports this script.
qv Bug 349309 Comment 9 :
------------------------------------------------------------
Steffen Wilberg   2007-05-30 08:31:22 PDT

That's a lot of generic-looking code, yet it's in extensions.js.
It would be nice to have that in a generic location, so that it can be used by
other toolkit components as well (bug 345001, bug 263433, bug 262808)?
Also see visitLink() in globalOverlay.js, which is for the about dialog:
http://mxr.mozilla.org/seamonkey/source/toolkit/content/globalOverlay.js#150
------------------------------------------------------------
Right visitLink() would certainly benefit from my patch to openUrl()

Kairo: I think this bug should have been filed under Toolkit not Core.
> I think this bug should have been filed under Toolkit not Core.
Core -> Toolkit
Component: General → XUL Widgets
Product: Core → Toolkit
QA Contact: general → xul.widgets
From a look today, I think openURL() should probably go into contentAreaUtils.js - we could do a patch here that moves it from extensions.js here and makes EM use that version, other consumers of similar functions (like openUILink() in Firefox' utilityOverlay.js) can be ported over to this function later.
Uh, I wouldn't have believed this to be so easy... Still need to test other apps, but from what I have seen so far, this actually works fine!
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #275817 - Flags: review?(benjamin)
(In reply to comment #5)
> Uh, I wouldn't have believed this to be so easy...
That's because it isn't.

+  var ios = Components.classes["@mozilla.org/network/io-service;1"]
+                            .getService(nsIIOService);
--> Components.interfaces.nsIIOService

+    var loadgroup = Components.classes["@mozilla.org/network/load-group;1"]
+                              .createInstance(nsILoadGroup);
--> Components.interfaces.nsILoadGroup

+    var appstartup = Components.classes["@mozilla.org/toolkit/app-startup;1"]
+                               .getService(nsIAppStartup);
--> Components.interfaces.nsIAppStartup

+        if (iid.equals(nsILoadGroup))
--> Components.interfaces.nsILoadGroup

You can't rely on these constants being available from extensions.js
Right, the shorthands aren't defined everywhere. nsILoadGroup would have been defined locally, but it looks more readable to me to remove that as well when we can not use the other shorthands.
Attachment #275817 - Attachment is obsolete: true
Attachment #275962 - Flags: review?(benjamin)
Attachment #275817 - Flags: review?(benjamin)
Attachment #275962 - Flags: review?(benjamin) → review+
Checked in. I'll leave it to others to add further consumers as mentioned in comment #2 ;-)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Duplicate of this bug: 345001
Blocks: 263433
Blocks: 262808
You need to log in before you can comment on or make changes to this bug.