957184 - Replace |.getService() .QueryInterface(iid)| by |.getService(iid)| in comm-central

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[:aceman]

It looks like there are still some occurences of this after conversion to Services.jsm

+++ This bug was initially created as a clone of Bug #451578 +++

Example:
{{
-          strBundleService =
-          Components.classes["@mozilla.org/intl/stringbundle;1"].getService(); 
-          strBundleService = 
-          strBundleService.QueryInterface(Components.interfaces.nsIStringBundleService);

+      gStrBundleService =
+        Components.classes["@mozilla.org/intl/stringbundle;1"]
+                  .getService(Components.interfaces.nsIStringBundleService);
}}

Comment 1

[Hessam Salehi]

Attachment: 957184-v1.patch

hope I didn't miss anything.

Comment 2

[:aceman]

Comment on attachment 8396998 [details] [diff] [review]
957184-v1.patch

Review of attachment 8396998 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/ui/composer/content/ComposerCommands.js
@@ +717,5 @@
>  {
>    try {
>      var mimeService = null;
> +    mimeService = Components.classes["@mozilla.org/mime;1"].
> +                  getService(Components.interfaces.nsIMIMEService);

When you split expressions like this, try to align the dots in a column
so the ".getService" should be under ".classes" .

::: suite/common/sidebar/customize-panel.js
@@ +5,5 @@
>  
>  // the rdf service
>  var RDF = '@mozilla.org/rdf/rdf-service;1'
> +RDF = Components.classes[RDF].
> +      getService(Components.interfaces.nsIRDFService);

I would write this as:
var RDF = Components.classes[@"mozilla.org/rdf/rdf-service;1"].getService(Components.interfaces.nsIRDFService);
As the '@mozilla.org/rdf/rdf-service;1' string is not used later on a constant (we overwrite it immediately), so I do not see the point of splitting it out of the expression.

But we'll ask a Seamonkey peer on what the preferred style is.

Comment 3

[:aceman]

So the alignment should look like this:
mimeService = Components.classes["@mozilla.org/mime;1"]
                        .getService(Components.interfaces.nsIMIMEService);

Comment 4

[Hessam Salehi]

(In reply to :aceman from comment #2)
> Comment on attachment 8396998 [details] [diff] [review]
> 957184-v1.patch
> 
> Review of attachment 8396998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: editor/ui/composer/content/ComposerCommands.js
> @@ +717,5 @@
> >  {
> >    try {
> >      var mimeService = null;
> > +    mimeService = Components.classes["@mozilla.org/mime;1"].
> > +                  getService(Components.interfaces.nsIMIMEService);
> 
> When you split expressions like this, try to align the dots in a column
> so the ".getService" should be under ".classes" .
> 
Fixed.
> ::: suite/common/sidebar/customize-panel.js
> @@ +5,5 @@
> >  
> >  // the rdf service
> >  var RDF = '@mozilla.org/rdf/rdf-service;1'
> > +RDF = Components.classes[RDF].
> > +      getService(Components.interfaces.nsIRDFService);
> 
> I would write this as:
> var RDF =
> Components.classes[@"mozilla.org/rdf/rdf-service;1"].getService(Components.
> interfaces.nsIRDFService);
> As the '@mozilla.org/rdf/rdf-service;1' string is not used later on a
> constant (we overwrite it immediately), so I do not see the point of
> splitting it out of the expression.
> 
> But we'll ask a Seamonkey peer on what the preferred style is.
I'm confused too why it's written that way, anyway I created a new patch.

Comment 5

[Hessam Salehi]

Attachment: 957184-v2.patch

Comment 6

[:aceman]

Comment on attachment 8397739 [details] [diff] [review]
957184-v2.patch

Review of attachment 8397739 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks great now!
So we can try to get a review. I try Ehsan for /editor, Florian for /im, mkmelin for /mail and Neil for /mailnews and /suite.

::: im/content/engineManager.js
@@ +425,5 @@
>    },
>  
>    getSourceIndexFromDrag: function () {
>      var dragService = Cc["@mozilla.org/widget/dragservice;1"].
> +                      getService(Ci.nsIDragService);

We usually put '.' on the next line but dot on line end seems to be the style in /im so you can keep this.

::: suite/common/sidebar/customize-panel.js
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // the rdf service
> +
> +var RDF = Components.classes["@mozilla.org/rdf/rdf-service;"]

Remove the empty line.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8397739 [details] [diff] [review]
957184-v2.patch

(In reply to :aceman from comment #6)

> ::: im/content/engineManager.js
> @@ +425,5 @@
> >    },
> >  
> >    getSourceIndexFromDrag: function () {
> >      var dragService = Cc["@mozilla.org/widget/dragservice;1"].
> > +                      getService(Ci.nsIDragService);
> 
> We usually put '.' on the next line but dot on line end seems to be the
> style in /im so you can keep this.

Context here: '.' is usually on the next line in im/ but this specific file was a direct copy of http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/engineManager.js and we avoid doing cosmetic changes to files we have imported from other places.

Comment 8

[[e-mail]]

Comment on attachment 8397739 [details] [diff] [review]
957184-v2.patch

(ehsan is mozilla/editor anyway)


>diff --git a/suite/common/permissions/cookieViewer.js b/suite/common/permissions/cookieViewer.js
...
>+Components.utils.import("resource://gre/modules/Services.jsm");
...
>+  cookiemanager = Services.cookies;
>+  permissionmanager = Services.perms;
>+  promptservice = Services.prompt;
(This file isn't actually currently in use, so don't bother patching it if you don't want to, but if you do want to start using Services here, you shouldn't copy Services properties into local variables, instead you should replace all uses of those variables with their Services equivalent.)

Comment 9

[:aceman]

OK, then I suggest to do just this:
-  cookiemanager = Components.classes["@mozilla.org/cookiemanager;1"].getService();
-  cookiemanager = cookiemanager.QueryInterface(Components.interfaces.nsICookieManager);
-  permissionmanager = Components.classes["@mozilla.org/permissionmanager;1"].getService();
-  permissionmanager = permissionmanager.QueryInterface(Components.interfaces.nsIPermissionManager);
-  promptservice = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService();
-  promptservice = promptservice.QueryInterface(Components.interfaces.nsIPromptService);
+  cookiemanager = Components.classes["@mozilla.org/cookiemanager;1"]
                             .getService(Components.interfaces.nsICookieManager)
+  permissionmanager = Components.classes["@mozilla.org/permissionmanager;1"]
                                 .getService(nsIPermissionManager);
+  promptservice = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
                             .getService(Components.interfaces.nsIPromptService);

and no other changes to that file. I think Seamonky didn't yet switch wholesale to Services.jsm as TB did, so we can just do what is needed for this bug.

Comment 10

[Hessam Salehi]

Attachment: 957184-v3.patch

Thank you for the review.

Comment 11

[:aceman]

Comment on attachment 8398543 [details] [diff] [review]
957184-v3.patch

Review of attachment 8398543 [details] [diff] [review]:
-----------------------------------------------------------------

So you removed touching the cookieViewer.js file, OK.

I am not a reviewer, but we just need to get review from mkmelin for the /mail file and we are ready for checkin.

But where did the /im files get lost in this last patch?

Comment 12

[Hessam Salehi]

Attachment: 957184-v4.diff

Oops. I recently started using TortoiseHg and I messed up my local repository. sorry.
added cookieviewer and /im. Hope I didn't miss anything in the new patch. :P

Comment 13

[Philip Chee]

>> But we'll ask a Seamonkey peer on what the preferred style is.
> I'm confused too why it's written that way, anyway I created a new patch.
The RDF code is ancient Netscape era code when every thing was being re-written from the ground up and RDF might still have been buggy like hades.

Thanks muchly for thinking of us.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 8398804 [details] [diff] [review]
957184-v4.diff

Review of attachment 8398804 [details] [diff] [review]:
-----------------------------------------------------------------

mail/ bits look good to me. r=mkmelin

Comment 15

[:aceman]

Comment on attachment 8398804 [details] [diff] [review]
957184-v4.diff

Review of attachment 8398804 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, it seems the missing files are put back.

Comment 16

[:aceman]

Looks like this is done, congrats.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/11cd943edd27