412768 - Make use of calUtils.jsm' |cal.getIOService()|

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Description

[Daniel Boelzle [:dbo]]

There are quite a lot of places that could easily use getIOService():
<http://mxr.mozilla.org/mozilla1.8/search?string=network%2Fio-service&find=calendar&findi=&filter=&tree=mozilla1.8>

Comment 1

[Daniel Boelzle [:dbo]]

mxr: <http://mxr.mozilla.org/comm-central/search?string=io-service&find=calendar/>

Comment 2

[Edmund Wong (:ewong)]

Attachment: Feedback patch on direction request.

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 464326 [details] [diff] [review]
Feedback patch on direction request.

Yep, right direction. If there are places where the variable ioService is only
used once, go ahead and directly call i.e

cal.getIOService().newChannelFromURI(icsURL)

Comment 4

[Edmund Wong (:ewong)]

Attachment: Changed code to use cal.getIOService() instead of the full Components.Classes["@mozilla.org/network/io-service;1"]

This patch has all but the following entry done.  

/calendar/base/src/calItemModule.js (View Hg log or Hg annotations)
    * line 184 -- let ioService = Components.classes["@mozilla.org/network/io-service;1"]

/calendar/base/src/calItemModule.js (as of this writing) has only 106 lines.

Comment 5

[Edmund Wong (:ewong)]

(In reply to comment #4)
> Created attachment 464356 [details] [diff] [review]
> Changed code to use cal.getIOService() instead of the full
> Components.Classes["@mozilla.org/network/io-service;1"]
> 
> This patch has all but the following entry done.  
> 
> /calendar/base/src/calItemModule.js (View Hg log or Hg annotations)
>     * line 184 -- let ioService =
> Components.classes["@mozilla.org/network/io-service;1"]
> 
> /calendar/base/src/calItemModule.js (as of this writing) has only 106 lines.

Ignore this.  It seems as if mxr is now returning the right list.  This
entry is gone.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 464356 [details] [diff] [review]
Changed code to use cal.getIOService() instead of the full Components.Classes["@mozilla.org/network/io-service;1"]

>diff --git a/calendar/base/content/dialogs/calendar-event-dialog-attendees.xml b/calendar/base/content/dialogs/calendar-event-dialog-attendees.xml
>--- a/calendar/base/content/dialogs/calendar-event-dialog-attendees.xml
>+++ b/calendar/base/content/dialogs/calendar-event-dialog-attendees.xml
> 
>-        this.mIOService = Components.classes["@mozilla.org/network/io-service;1"]
>-                                    .getService(Components.interfaces.nsIIOService);
>+        this.mIOService = cal.getIOService();
>         this.mIsOffline = this.mIOService.offline;
>       ]]></constructor>

this.mIOService is only used once, at construction. It is used to set mIsOffline, which is also used only once. Please go ahead and remove both mIOService and mIsOffline, and then use cal.getIOService().offline instead of mIsOffline.




> 
>     /**
>      * Cached getter for the IO Service
>      * XXX replace with cal.getIOService()
>      */
>     get ioService() {
>         if (!this.mIoService) {
>-            this.mIoService = Components.classes["@mozilla.org/network/io-service;1"]
>-                              .getService(Components.interfaces.nsIIOService);
>+            this.mIoService = cal.getIOService();
>         }
>         return this.mIoService;
>     },
Same here. Its only used once, it would be nice to get rid of this getter and use cal.getIOService() directly.


>         // Convert the file url into a nsILocalFile
>         if (aFileURL) {
>-            var ios = Components.classes["@mozilla.org/network/io-service;1"]
>-                                .getService(Components.interfaces.nsIIOService);
>+            var ios = cal.getIOService();
>             var fph = ios.getProtocolHandler("file")
>                          .QueryInterface(Components.interfaces.nsIFileProtocolHandler);
>             return fph.getFileFromURLSpec(aFileURL);

ios is only used once in this function, please change to:

let fph = cal.getIOService().getProtocolHandler("file")
                            .QueryInterface(...);

Same goes for some other usages in this and some other files. Sorry if I was unclear about this, I meant to say that if its only used once per function, then replace the variable with cal.getIOService() directly.

Other files affected:
>diff --git a/calendar/providers/ics/calICSCalendar.js b/calendar/providers/ics/calICSCalendar.js
>diff --git a/calendar/resources/content/publish.js b/calendar/resources/content/publish.js
>diff --git a/calendar/test/unit/head_consts.js b/calendar/test/unit/head_consts.js



>       urlToOpen = regionBundle.GetStringFromName(aResourceName);
>       
>-    var uri = Components.classes["@mozilla.org/network/io-service;1"]
>-              .getService(Components.interfaces.nsIIOService)
>-              .newURI(urlToOpen, null, null);
>-
>     var protocolSvc = Components.classes["@mozilla.org/uriloader/external-protocol-service;1"]
>                       .getService(Components.interfaces.nsIExternalProtocolService);
>-    protocolSvc.loadUrl(uri);
>+    protocolSvc.loadUrl(cal.getIOService().newURI(urlToOpen, null, null));

Yeah, this is more like it, great!


>diff --git a/calendar/base/src/calUtils.js b/calendar/base/src/calUtils.js
>--- a/calendar/base/src/calUtils.js
>+++ b/calendar/base/src/calUtils.js
>@@ -31,16 +31,17 @@
>  * under the terms of either the GPL or the LGPL, and not to allow others to
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
>+Components.utils.import("resource://calendar/modules/calUtils.jsm");

Be careful in this file. calUtils.jsm loads calUtils.js into its namespace, all function in calUtils.js are then available via cal.functionName().
We shouldn't be loading calUtils.jsm in calUtils.js, since calUtils.jsm loads calUtils.js :-)


> 
> /* Shortcut to the IO service */
> function getIOService() {
>-    return Components.classes["@mozilla.org/network/io-service;1"]
>-                     .getService(Components.interfaces.nsIIOService2);
>+    return cal.getIOService();
> }

This is exactly the function that provides cal.getIOService()

>  */
> function makeURL(aUriString) {
>-    var ioSvc = Components.classes["@mozilla.org/network/io-service;1"].
>-                getService(Components.interfaces.nsIIOService);
>-    return ioSvc.newURI(aUriString, null, null);
>+    return cal.getIOService().newURI(aUriString, null, null);
> }

Since we are in calUtils.js, I'd suggest just calling

>+    return getIOService().newURI(aUriString, null, null);

for now, until we migrate everything to calUtils.jsm. Same goes for the remaining locations in this file.


>diff --git a/calendar/timezones/update.js b/calendar/timezones/update.js
This file is called directly from xpcshell only. We can't load files from the resource://calendar/ uris since the location is often not correctly registered for the shell. I'd suggest to leave this file as it is.

r- to get a new patch with the nits fixed. Thanks for taking care!

Comment 7

[Edmund Wong (:ewong)]

Attachment: Changed code to use cal.getIOService() instead of the full Components.Classes["@mozilla.org/network/io-service;1"] (v2)

Fixed nits as per comment 6.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 466537 [details] [diff] [review]
Changed code to use cal.getIOService() instead of the full Components.Classes["@mozilla.org/network/io-service;1"]   (v2)

>@@ -206,11 +206,7 @@ var gDataMigrator = {
>      * XXX replace with cal.getIOService()
>      */
>     get ioService() {
>-        if (!this.mIoService) {
>-            this.mIoService = Components.classes["@mozilla.org/network/io-service;1"]
>-                              .getService(Components.interfaces.nsIIOService);
>-        }
>-        return this.mIoService;
>+        return cal.getIOService();
>     },
This is only used once, replace gDataMigrator.ioService.whatever with cal.getIOService().whatever.

> /* Shortcut to the IO service */
> function getIOService() {
>-    return Components.classes["@mozilla.org/network/io-service;1"]
>-                     .getService(Components.interfaces.nsIIOService2);
>+    var ioSvc = Components.classes["@mozilla.org/network/io-service;1"].
>+                getService(Components.interfaces.nsIIOService);
>+    return ioSvc.newURI(aUriString, null, null);
> }
Now the IO Service returns a new uri. I'm sure this is just a copy/paste error :) This function needs to stay unchanged.

> 
>-        protoSvc.loadUrl(ioService.newURI(uriString, null, null));
>+        protoSvc.loadUrl(getIOService().newURI(uriString, null, null));
Go ahead and use makeURL while we are here: protoSvc.loadUrl(makeURL(uriString));



The rest looks fine. I've corrected these minor things locally while testing the patch, so I'm going to check it in with the nits fixed.
r=philipp Thanks for the patch!

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/b22f57815531>
-> FIXED

Comment 10

[Martin Schröder [:mschroeder]]

Comment on attachment 466537 [details] [diff] [review]
Changed code to use cal.getIOService() instead of the full Components.Classes["@mozilla.org/network/io-service;1"]   (v2)

>diff --git a/calendar/test/unit/head_consts.js b/calendar/test/unit/head_consts.js
>--- a/calendar/test/unit/head_consts.js
>+++ b/calendar/test/unit/head_consts.js
>@@ -37,18 +37,16 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
>+Components.utils.import("resource://calendar/modules/calUtils.jsm");
> 
> const Cc = Components.classes;
> const Ci = Components.interfaces;
> 
>-let protHandler = Components.classes["@mozilla.org/network/io-service;1"]
>-                            .getService(Components.interfaces.nsIIOService2)
>-                            .getProtocolHandler("resource")
>-                            .QueryInterface(Components.interfaces.nsIResProtocolHandler);
>+let protHandler = cal.getIOService()
>+                     .getProtocolHandler("resource")
>+                     .QueryInterface(Components.interfaces.nsIResProtocolHandler);
> protHandler.setSubstitution("calendar", protHandler.getSubstitution("gre"));
> 
>-Components.utils.import("resource://calendar/modules/calUtils.jsm");
>-

This change is wrong afaik! We should not import the jsm before setting our resource protocol handler.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Martin is right, this change needs to be reverted. Thanks for catching this!

> (10:09:34 AM) Callek|SkipForQueue: [07:07:39] ewong: no; jsm import _after_ 
> or before that code block won't change the way the jsm is evaluated
> (10:09:34 AM) Callek|SkipForQueue: [07:07:57] actually it *could* if it is 
> the _FIRST_ import of that jsm
Yes, this *is* the first import of that jsm, its unit testing code.

> (10:09:34 AM) ewong: [07:09:13] Callek|SkipForQueue, if I import it after the 
> code block,  then I can't use the cal.getIOService(), right?
> (10:09:34 AM) Callek|SkipForQueue: [07:09:45] ewong: correct, but you could 
> possibly instead import Services.jsm above that code block and use Services 
Since its possible that the next release will come from 1.9.2 I'd rather not use Services.jsm too much yet. Lets stay with the long version for now.

> (10:09:34 AM) Callek|SkipForQueue: [07:12:53] ewong: from a skim of calUtils 
> I don't see how doing that protocol handler before || after the import should 
> matter actually
If its called afterwards, you'll likely get an error that file cannot be found. Back in 1.9.2, chrome registrations of resources were sometimes made after xpcom registration, which causes problems. I think this is fixed on trunk though. Lets keep the registration the way we have it for now though.

ewong, could you prepare a small patch to revert this change?

Comment 12

[Edmund Wong (:ewong)]

Attachment: Revert part of the initial patch that dealt with head_consts.js

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 468135 [details] [diff] [review]
Revert part of the initial patch that dealt with head_consts.js

r=philipp

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/6ca853c662fe>
-> FIXED

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/3f466ce7fe37> and <http://hg.mozilla.org/releases/comm-1.9.2/rev/b055842903c0>

a=philipp