Closed Bug 954192 Opened 10 years ago Closed 10 years ago

Clean up jsProtoHelper

Categories

(Chat Core :: General, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

Details

Attachments

(2 files, 3 obsolete files)

*** Original post on bio 758 at 2011-04-19 13:24:00 UTC ***

This was discussed on IRC so I figured I'd file a bug about it before we forget.

jsProtoHelper includes a variety of functions that really have nothing to do with JS Protocols and could be safely removed and then included from a different module so they can be used in other places.

The following currently exported symbols could probably be moved:
setTimeout
clearTimeout
nsSimpleEnumerator
EmptyEnumerator
ClassInfo
doXHRequest

I think we (jokingly?) said we could move them to an XPCOMUtils.jsm (which perhaps could include the toolkit XPCOMUtils.jsm and reexport, not sure if that's worth it though) and moved into instantbird/modules/, so we can use it in standard Instantbird code (not that we actually can't right now...but it'd be kind of weird to still have it under the purplexpcom stuff).

doXHRequest probably doesn't fit into that sort of thing, but is a good 70 lines or so and could probably be moved into a separate file.
*** Original post on bio 758 at 2011-04-21 14:51:53 UTC ***

(In reply to comment #0)

> I think we (jokingly?) said we could move them to an XPCOMUtils.jsm (which
> perhaps could include the toolkit XPCOMUtils.jsm and reexport, not sure if
> that's worth it though)

That would be great. The reason the XPCOMUtils.jsm name was a joke is that it's not possible because both the toolkit modules and the application modules end up in the same folder inside the omni.jar file when not building above xulrunner (only linux distribution builds of Instantbird are built above xulrunner).

> and moved into instantbird/modules/, so we can use it
> in standard Instantbird code (not that we actually can't right now...but it'd
> be kind of weird to still have it under the purplexpcom stuff).

purple/ was supposed to be the reusable part and instantbird/ the Instantbird-specific part of our code. More on this topic in bug 954193 (bio 759).
 
> doXHRequest probably doesn't fit into that sort of thing, but is a good 70
> lines or so and could probably be moved into a separate file.

Agreed.
Attached patch v1.0 (obsolete) — Splinter Review
*** Original post on bio 758 as attmnt 596 at 2011-04-22 00:14:00 UTC ***

Here's an initial cut, I haven't tested it since my build machine isn't working right now, but I think I caught all the instances. The one thing I wasn't sure about was the use of setTimeouts, do those depend on the one from jsProtoHelper or not?
*** Original post on bio 758 at 2011-04-22 00:14:03 UTC ***

Also...left out the reason I'm attaching this without trying it: I won't be online with my own computer for the next five days or so. :)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
*** Original post on bio 758 at 2011-04-22 09:02:15 UTC ***

Comment on attachment 8352338 [details] [diff] [review] (bio-attmnt 596)
v1.0

Drive by comments:

>diff --git a/instantbird/modules/Makefile.in b/instantbird/modules/Makefile.in
>--- a/instantbird/modules/Makefile.in
>+++ b/instantbird/modules/Makefile.in
>@@ -44,6 +44,7 @@
> EXTRA_JS_MODULES = \
> 	ibCore.jsm \
> 	ibNotifications.jsm \
>+	ibXPCOMUtils.jsm \


Nothing in purple/ should depend on things in instantbird/ as purple/ is supposed to be reusable.

>diff --git a/instantbird/modules/ibXPCOMUtils.jsm b/instantbird/modules/ibXPCOMUtils.jsm

>+Cu.import("resource:///modules/imServices.jsm");
>+Cu.import("resource:///modules/jsProtoHelper.jsm");
>+Cu.import("resource:///modules/http.jsm");

Is any of these used here?

>diff --git a/purple/purplexpcom/src/facebookOverrideProtocol.js b/purple/purplexpcom/src/facebookOverrideProtocol.js

> UsernameSplit.prototype = {
>-  QueryInterface: XPCOMUtils.generateQI([Ci.purpleIUsernameSplit,
>-                                         Ci.nsIClassInfo]),
>-  getInterfaces: function(countRef) {
>-    var interfaces = [Ci.nsIClassInfo, Ci.nsISupports, Ci.purpleIUsernameSplit];
>-    countRef.value = interfaces.length;
>-    return interfaces;
>-  },
>-  getHelperForLanguage: function(language) null,
>-  contractID: null,
>-  classDescription: "Username Split object",
>-  classID: null,
>-  implementationLanguage: Ci.nsIProgrammingLanguage.JAVASCRIPT,
>-  flags: Ci.nsIClassInfo.DOM_OBJECT,
>+  __proto__: ClassInfo("purpleIUsernameSplit", "username split object"),

:-)

>diff --git a/purple/purplexpcom/src/http.jsm b/purple/purplexpcom/src/http.jsm

>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource:///modules/imServices.jsm");

What are these used for?
I suspect LOG is undefined, by the way.

>diff --git a/purple/purplexpcom/src/imConversations.js b/purple/purplexpcom/src/imConversations.js
>--- a/purple/purplexpcom/src/imConversations.js
>+++ b/purple/purplexpcom/src/imConversations.js
>@@ -39,6 +39,7 @@
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Cu.import("resource:///modules/imServices.jsm");
> Cu.import("resource:///modules/jsProtoHelper.jsm");
>+Cu.import("resource:///modules/ibXPCOMUtils.jsm");

Remove jsProtoHelper where it has no reason to be included, otherwise the split is worthless :).

>diff --git a/purple/purplexpcom/src/jsProtoHelper.jsm b/purple/purplexpcom/src/jsProtoHelper.jsm

>+const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> 
>-Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>-Components.utils.import("resource:///modules/imServices.jsm");
>-
>-const Cc = Components.classes;
>-const Ci = Components.interfaces;
>-const Cr = Components.results;

Cr is actually used.

>@@ -239,9 +142,9 @@
>   },
> 
>   addBuddy: function(aTag, aName) {
>-    Components.classes["@instantbird.org/purple/contacts-service;1"]
>-              .getService(Ci.imIContactsService)
>-              .accountBuddyAdded(new AccountBuddy(this, null, aTag, aName));
>+    Cc["@instantbird.org/purple/contacts-service;1"]
>+      .getService(Ci.imIContactsService)
>+      .accountBuddyAdded(new AccountBuddy(this, null, aTag, aName));

Services.contacts here...

>@@ -318,9 +221,9 @@
>   set tag(aNewTag) {
>     let oldTag = this._tag;
>     this._tag = aNewTag;
>-    Components.classes["@instantbird.org/purple/contacts-service;1"]
>-              .getService(Ci.imIContactsService)
>-              .accountBuddyMoved(this, oldTag, aNewTag);
>+    Cc["@instantbird.org/purple/contacts-service;1"]
>+      .getService(Ci.imIContactsService)
>+      .accountBuddyMoved(this, oldTag, aNewTag);

here...

>@@ -340,9 +243,9 @@
>   },
> 
>   remove: function() {
>-    Components.classes["@instantbird.org/purple/contacts-service;1"]
>-              .getService(Ci.imIContactsService)
>-              .accountBuddyRemoved(this);
>+    Cc["@instantbird.org/purple/contacts-service;1"]
>+      .getService(Ci.imIContactsService)
>+      .accountBuddyRemoved(this);

and here! :-)
*** Original post on bio 758 at 2011-04-22 09:04:03 UTC ***

(In reply to comment #2)

> The one thing I wasn't sure
> about was the use of setTimeouts, do those depend on the one from jsProtoHelper
> or not?

setTimeout is part of the "window" global objects. It's not defined by default in XPCOM components and JS modules.
*** Original post on bio 758 at 2011-04-26 16:56:22 UTC ***

(In reply to comment #4)
> (From update of attachment 8352338 [details] [diff] [review] (bio-attmnt 596) [details])
> Drive by comments:
> >diff --git a/instantbird/modules/Makefile.in b/instantbird/modules/Makefile.in
> >--- a/instantbird/modules/Makefile.in
> >+++ b/instantbird/modules/Makefile.in
> >@@ -44,6 +44,7 @@
> > EXTRA_JS_MODULES = \
> > 	ibCore.jsm \
> > 	ibNotifications.jsm \
> >+	ibXPCOMUtils.jsm \
> Nothing in purple/ should depend on things in instantbird/ as purple/ is
> supposed to be reusable.
Can these really be split then? jsProtoHelper heavily uses ClassInfo, nsSimpleEnumerator and EmptyEnumerator, or are you suggesting we plut it under purple/ somewhere and argue about it in bug 954193 (bio 759)? ;)

> >diff --git a/instantbird/modules/ibXPCOMUtils.jsm b/instantbird/modules/ibXPCOMUtils.jsm
> >+Cu.import("resource:///modules/imServices.jsm");
> >+Cu.import("resource:///modules/jsProtoHelper.jsm");
> >+Cu.import("resource:///modules/http.jsm");
> Is any of these used here?
Copy & paste error, thanks.

> >diff --git a/purple/purplexpcom/src/http.jsm b/purple/purplexpcom/src/http.jsm
> >+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> >+Cu.import("resource:///modules/imServices.jsm");
> What are these used for?
> I suspect LOG is undefined, by the way.
Yes, it is. Thanks.

> >diff --git a/purple/purplexpcom/src/imConversations.js b/purple/purplexpcom/src/imConversations.js
> >--- a/purple/purplexpcom/src/imConversations.js
> >+++ b/purple/purplexpcom/src/imConversations.js
> >@@ -39,6 +39,7 @@
> > Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > Cu.import("resource:///modules/imServices.jsm");
> > Cu.import("resource:///modules/jsProtoHelper.jsm");
> >+Cu.import("resource:///modules/ibXPCOMUtils.jsm");
> Remove jsProtoHelper where it has no reason to be included, otherwise the split
> is worthless :).
Unfortunately I think there's going to be some overlap where both are used. jsProtoHelper (at least) seems to never really be used without needing nsSimpleEnumerator or EmptyEnumerator.

> >diff --git a/purple/purplexpcom/src/jsProtoHelper.jsm b/purple/purplexpcom/src/jsProtoHelper.jsm
> >+const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> > 
> >-Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> >-Components.utils.import("resource:///modules/imServices.jsm");
> >-
> >-const Cc = Components.classes;
> >-const Ci = Components.interfaces;
> >-const Cr = Components.results;
> Cr is actually used.
Will fix that. :)

I'll look over this again and see what needs updating and possibly comment out some of jsProtoHelper (although that might end up being a follow up bug).
*** Original post on bio 758 at 2011-04-26 17:01:53 UTC ***

(In reply to comment #6)
> are you suggesting we put it under
> purple/ somewhere and argue about it in bug 954193 (bio 759)? ;)
Yes.

> > >diff --git a/purple/purplexpcom/src/imConversations.js b/purple/purplexpcom/src/imConversations.js

> > > Cu.import("resource:///modules/jsProtoHelper.jsm");
> > >+Cu.import("resource:///modules/ibXPCOMUtils.jsm");
> > Remove jsProtoHelper where it has no reason to be included, otherwise the split
> > is worthless :).
> Unfortunately I think there's going to be some overlap where both are used.
> jsProtoHelper (at least) seems to never really be used without needing
> nsSimpleEnumerator or EmptyEnumerator.

Sure. Most protocol plugins will need to import both. But non-protocol code won't need to import jsProtoHelper (with the message theme preview in the preference window being an exception).
Attached patch v1.5 (obsolete) — Splinter Review
*** Original post on bio 758 as attmnt 601 at 2011-04-27 00:26:00 UTC ***

This fixes the comments. Two other files could benefit from ibXPCOMUtils.jsm -- /instantbird/content/debug/fake/fake.js and /instantbird/components/logger.js; I didn't use them since I'm not sure of the purple/ instantbird/ relationship here.
Comment on attachment 8352338 [details] [diff] [review]
v1.0

*** Original change on bio 758 attmnt 596 at 2011-04-27 00:26:22 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352338 - Attachment is obsolete: true
Attached patch v2.0 (unbitrotted and updated!) (obsolete) — Splinter Review
*** Original post on bio 758 as attmnt 747 at 2011-07-16 00:53:00 UTC ***

This might break a few extensions (LJ Talk, JS-IRC of mine, I know...also XMPP-JS of varuna's).

I ran this for a bit and it seemed OK.
Attachment #8352489 - Flags: review?(florian)
Comment on attachment 8352344 [details] [diff] [review]
v1.5

*** Original change on bio 758 attmnt 601 at 2011-07-16 00:53:24 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352344 - Attachment is obsolete: true
Comment on attachment 8352489 [details] [diff] [review]
v2.0 (unbitrotted and updated!)

*** Original change on bio 758 attmnt 747 at 2011-07-16 01:43:17 UTC ***

>diff --git a/instantbird/components/logger.js b/instantbird/components/logger.js

>+Components.utils.import("resource:///modules/ibXPCOMUtils.jsm");

The ib prefix are for files that are specific to the Instantbird UI. No file in purple/ should use files prefixed with ib. I think you wanted to call this file imXPCOMUtils.jsm.


imServices.jsm re-exports Services from the Mozilla Services.jsm, should imXPCOMUtils.jsm re-export XPCOMUtils?



> function Logger() { }
> Logger.prototype = {
>   _enumerateLogs: function logger__enumerateLogs(aAccount, aNormalizedName) {
>     let file = getLogFolderForAccount(aAccount);
>     file.append(aNormalizedName);
>     if (!file.exists())
>       return EmptyEnumerator;
> 
>-    return new LogEnumerator([file.directoryEntries]);
>+    return new nsSimpleEnumerator([file.directoryEntries]);

LogEnumerator is not the same thing as nsSimpleEnumerator.



>diff --git a/instantbird/modules/ibCore.jsm b/instantbird/modules/ibCore.jsm
>--- a/instantbird/modules/ibCore.jsm
>+++ b/instantbird/modules/ibCore.jsm

Are the changes to this file related in any way with cleaning up jsProtoHelper?
I'm not super happy with them. If you really want to make them, then there are a few places where saving a few characters should reduce the number of lines.

>     var um =
>-      Components.classes["@mozilla.org/updates/update-manager;1"]
>-                .getService(Components.interfaces.nsIUpdateManager);
>+      Cc["@mozilla.org/updates/update-manager;1"]
>+        .getService(Ci.nsIUpdateManager);
>     var prompter =
>-      Components.classes["@mozilla.org/updates/update-prompt;1"]
>-                .createInstance(Components.interfaces.nsIUpdatePrompt);
>+      Cc["@mozilla.org/updates/update-prompt;1"]
>+        .createInstance(Ci.nsIUpdatePrompt);

Here for example.



>diff --git a/purple/purplexpcom/src/jsProtoHelper.jsm b/purple/purplexpcom/src/jsProtoHelper.jsm

>   getTooltipInfo: function() {
>-    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
>+    throw Cr.NS_ERROR_NOT_IMPLEMENTED;
>   },
>   createConversation: function() {
>-    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
>+    throw Cr.NS_ERROR_NOT_IMPLEMENTED;
>   }

You can save space by writing |function() { throw Cr.NS_ERROR_NOT_IMPLEMENTED; } on a single line.

I wonder if we should change getTooltipInfo here to return an EmptyEnumerator instead (I'm a bit tired of the errors in my terminal when hovering buddies of the JSTest protocol plugin ;)).

Other changes looks pretty good (or I'm too tired to notice anything in them at almost 4am ;)).
Attachment #8352489 - Flags: review?(florian) → review-
Attached patch v2.5Splinter Review
*** Original post on bio 758 as attmnt 749 at 2011-07-16 02:00:00 UTC ***

(In reply to comment #10)
> (From update of attachment 8352489 [details] [diff] [review] (bio-attmnt 747) [details])
> >diff --git a/instantbird/components/logger.js b/instantbird/components/logger.js
> 
> >+Components.utils.import("resource:///modules/ibXPCOMUtils.jsm");
> 
> The ib prefix are for files that are specific to the Instantbird UI. No file in
> purple/ should use files prefixed with ib. I think you wanted to call this file
> imXPCOMUtils.jsm.
Good call!

> imServices.jsm re-exports Services from the Mozilla Services.jsm, should
> imXPCOMUtils.jsm re-export XPCOMUtils?
I also added that and removed XPCOMUtils import from the affected files.

> LogEnumerator is not the same thing as nsSimpleEnumerator.
My mistake, I reverted those changes

> >diff --git a/instantbird/modules/ibCore.jsm b/instantbird/modules/ibCore.jsm
> >--- a/instantbird/modules/ibCore.jsm
> >+++ b/instantbird/modules/ibCore.jsm
> 
> Are the changes to this file related in any way with cleaning up jsProtoHelper?
I reverted these changes as well.

> >diff --git a/purple/purplexpcom/src/jsProtoHelper.jsm b/purple/purplexpcom/src/jsProtoHelper.jsm
> 
> >   getTooltipInfo: function() {
> >-    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> >+    throw Cr.NS_ERROR_NOT_IMPLEMENTED;
> >   },
> >   createConversation: function() {
> >-    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> >+    throw Cr.NS_ERROR_NOT_IMPLEMENTED;
> >   }
> 
> You can save space by writing |function() { throw Cr.NS_ERROR_NOT_IMPLEMENTED;
> } on a single line.
> 
> I wonder if we should change getTooltipInfo here to return an EmptyEnumerator
> instead (I'm a bit tired of the errors in my terminal when hovering buddies of
> the JSTest protocol plugin ;)).
getTooltipInfo now returns an EmptyEnumerator, I moved the other one onto single lines.
Attachment #8352491 - Flags: review?(florian)
Comment on attachment 8352489 [details] [diff] [review]
v2.0 (unbitrotted and updated!)

*** Original change on bio 758 attmnt 747 at 2011-07-16 02:00:37 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352489 - Attachment is obsolete: true
Comment on attachment 8352491 [details] [diff] [review]
v2.5

*** Original change on bio 758 attmnt 749 at 2011-07-20 23:17:34 UTC ***

Looks good except you didn't actually revert your changes in ibCore.jsm and forgot to change instantbird/content/debug/fake/fake.js and purple/purplexpcom/src/jsTestProtocol.js.
Attachment #8352491 - Flags: review?(florian) → review+
*** Original post on bio 758 at 2011-07-20 23:21:23 UTC ***

Fixed: https://hg.instantbird.org/instantbird/rev/2825c6d440c4 Thanks!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.1
*** Original post on bio 758 as attmnt 754 at 2011-07-22 23:32:00 UTC ***

This ironically broke my other patch that got checked in at the same time (bug 954311 (bio 878)).  imConversations instantiates a new Message object which actually is in jsProtoHelper, so it needs both jsProtoHelper as well as imXPCOMUtils.
Attachment #8352496 - Flags: review?(florian)
*** Original post on bio 758 at 2011-07-22 23:45:48 UTC ***

Reopening for the follow up.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8352496 [details] [diff] [review]
Follow up to fix systemMessage

*** Original change on bio 758 attmnt 754 at 2011-08-04 21:07:12 UTC ***

I dislike it but I've nothing better to propose here :(.
Attachment #8352496 - Flags: review?(florian) → review+
*** Original post on bio 758 at 2011-08-04 21:10:26 UTC ***

https://hg.instantbird.org/instantbird/rev/500ecc0aa248
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.