Closed
Bug 957184
Opened 11 years ago
Closed 11 years ago
Replace |.getService() .QueryInterface(iid)| by |.getService(iid)| in comm-central
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 31.0
People
(Reporter: aceman, Assigned: hessamms)
References
(Blocks 1 open bug, )
Details
(Whiteboard: [good first bug][mentor=aceman][lang=js])
Attachments
(1 file, 3 obsolete files)
9.34 KB,
patch
|
mkmelin
:
review+
aceman
:
feedback+
|
Details | Diff | Splinter Review |
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);
}}
Assignee | ||
Comment 1•11 years ago
|
||
hope I didn't miss anything.
Attachment #8396998 -
Flags: feedback?
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.
Attachment #8396998 -
Flags: feedback? → feedback?(philip.chee)
So the alignment should look like this:
mimeService = Components.classes["@mozilla.org/mime;1"]
.getService(Components.interfaces.nsIMIMEService);
Updated•11 years ago
|
Assignee: nobody → hessamms
Assignee | ||
Comment 4•11 years ago
|
||
(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.
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #8396998 -
Attachment is obsolete: true
Attachment #8396998 -
Flags: feedback?(philip.chee)
Attachment #8397739 -
Flags: feedback?(acelists)
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.
Attachment #8397739 -
Flags: review?(neil)
Attachment #8397739 -
Flags: review?(mkmelin+mozilla)
Attachment #8397739 -
Flags: review?(florian)
Attachment #8397739 -
Flags: review?(ehsan)
Attachment #8397739 -
Flags: feedback?(acelists)
Attachment #8397739 -
Flags: feedback+
Comment 7•11 years ago
|
||
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.
Attachment #8397739 -
Flags: review?(florian) → review+
Comment 8•11 years ago
|
||
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.)
Attachment #8397739 -
Flags: review?(neil)
Attachment #8397739 -
Flags: review?(ehsan)
Attachment #8397739 -
Flags: review+
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.
Assignee | ||
Comment 10•11 years ago
|
||
Thank you for the review.
Attachment #8397739 -
Attachment is obsolete: true
Attachment #8397739 -
Flags: review?(mkmelin+mozilla)
Attachment #8398543 -
Flags: review?(acelists)
Reporter | ||
Comment 11•11 years ago
|
||
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?
Attachment #8398543 -
Flags: review?(mkmelin+mozilla)
Attachment #8398543 -
Flags: review?(acelists)
Attachment #8398543 -
Flags: feedback+
Assignee | ||
Comment 12•11 years ago
|
||
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
Attachment #8398543 -
Attachment is obsolete: true
Attachment #8398543 -
Flags: review?(mkmelin+mozilla)
Attachment #8398804 -
Flags: review?(mkmelin+mozilla)
Comment 13•11 years ago
|
||
>> 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•11 years ago
|
||
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
Attachment #8398804 -
Flags: review?(mkmelin+mozilla) → review+
Reporter | ||
Comment 15•11 years ago
|
||
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.
Attachment #8398804 -
Flags: feedback+
Reporter | ||
Comment 16•11 years ago
|
||
Looks like this is done, congrats.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Comment 17•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
You need to log in
before you can comment on or make changes to this bug.
Description
•