1238537 - make nsBrowserContentHandler.js use Services.jsm

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Florian Quèze [:florian]]

While working on bug 1237648, I noticed that nsBrowserContentHandler.js still calls getService in plenty of places where Services.* should be used instead.

The file is at http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js

For example,
242   var wwatch = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
243                          .getService(nsIWindowWatcher);
244 
245   wwatch.openWindow(null, gBrowserContentHandler.chromeURL,
should become:
  Services.ww.openWindow(...

The way to work on this is to search for 'getService' in nsBrowserContentHandler.js, and check in http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Services.jsm if the service is available on the Services object.

Comment 1

[Vishnu (:malayaleecoder)]

I would like to take this as my first bug. Can you please guide me through?

Comment 2

[Florian Quèze [:florian]]

(In reply to malayaleecoder from comment #1)
> I would like to take this as my first bug. Can you please guide me through?

Sure. Do you already have a copy of the Firefox source code, and a local build?

Comment 3

[Vishnu (:malayaleecoder)]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> (In reply to malayaleecoder from comment #1)
> > I would like to take this as my first bug. Can you please guide me through?
> 
> Sure. Do you already have a copy of the Firefox source code, and a local
> build?

No, I don't. My system is currently running OSX 10.11.1!

Comment 4

[Florian Quèze [:florian]]

(In reply to malayaleecoder from comment #3)
> (In reply to Florian Quèze [:florian] [:flo] from comment #2)
> > (In reply to malayaleecoder from comment #1)
> > > I would like to take this as my first bug. Can you please guide me through?
> > 
> > Sure. Do you already have a copy of the Firefox source code, and a local
> > build?
> 
> No, I don't. My system is currently running OSX 10.11.1!

Ok. You can find instructions about how to get the source code and how to build on this page: https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

Once you have your local build, the description of the bug here should give you indications about where to start looking inside the source code. Let me know if you need more help to get started.

Comment 5

[Vishnu (:malayaleecoder)]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> (In reply to malayaleecoder from comment #3)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #2)
> > > (In reply to malayaleecoder from comment #1)
> > > > I would like to take this as my first bug. Can you please guide me through?
> > > 
> > > Sure. Do you already have a copy of the Firefox source code, and a local
> > > build?
> > 
> > No, I don't. My system is currently running OSX 10.11.1!
> 
> Ok. You can find instructions about how to get the source code and how to
> build on this page:
> https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
> 
> Once you have your local build, the description of the bug here should give
> you indications about where to start looking inside the source code. Let me
> know if you need more help to get started.

I have got the local build and have started working on this!

Comment 6

[Vishnu (:malayaleecoder)]

Attachment: Bug1238537_v1.diff

Hello Florian,

Very sorry for the delay. I have attached an initial patch. Many getService call objects are not listed in Services.jsm. Please go through and suggest me further improvements.

Thanks :)

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8718720 [details] [diff] [review]
Bug1238537_v1.diff

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

Thanks for the patch, this is going in the right direction!

Other things that can be simplified:
321     var prefb = Components.classes["@mozilla.org/preferences-service;1"]
322                           .getService(nsIPrefBranch);
(also at line 497)

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

750           var fl = Components.classes["@mozilla.org/file/directory_service;1"]
751                              .getService(Components.interfaces.nsIProperties);

::: browser/components/nsBrowserContentHandler.js
@@ -59,5 @@
>  
>  function resolveURIInternal(aCmdLine, aArgument) {
>    var uri = aCmdLine.resolveURI(aArgument);
> -  var urifixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
> -                           .getService(nsIURIFixup);

The nsIURIFixup constant, defined a few lines above (line 40 currently) is now unused, please remove it :-).

The same comment applies for nsIWindowWatcher and nsIWindowMediator (and nsIPrefBranch after updating the patch).

@@ +62,4 @@
>  
>    if (!(uri instanceof nsIFileURL)) {
> +    return Services.uriFixup.createFixupURI(aArgument,
> +                                   Services.uriFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS);

Please preserve the alignment here: the second parameter should be aligned with the first one.

Alternatively, as "urifixup" was used 3 times in this function, you may want to prefer keeping the variable and just doing "var urifixup = Services.uriFixup;".

@@ +242,1 @@
>                      "_blank",

Please keep the parameters aligned.

Comment 8

[Vishnu (:malayaleecoder)]

Attachment: Bug1238537_v2.diff

Hello Florian,

Please check the attached patch. I have applied all that you have suggested in the previous comment. On running |mach build| I am getting a few errors, which I am unable to decipher. Can you help me out in finding what is causing this?

Thank You.

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8718906 [details] [diff] [review]
Bug1238537_v2.diff

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

::: browser/components/nsBrowserContentHandler.js
@@ +301,5 @@
>    get chromeURL() {
>      if (this.mChromeURL) {
>        return this.mChromeURL;
>      }
> +    var prefb = Services.prefs;

We don't need this prefb variable (used only once here).

Comment 10

[Florian Quèze [:florian]]

Thanks for the updated patch!

(In reply to malayaleecoder from comment #8)
> On running |mach build| I am getting a few errors,
> which I am unable to decipher. Can you help me out in finding what is
> causing this?

Could you please pastebin the error with some context?

Comment 11

[Vishnu (:malayaleecoder)]

Attachment: Bug1238537_v3.diff

Florian, I have made the above said changes in this attachment. Please check and tell me if any corrections are needed.

Thank You.

Comment 12

[Vishnu (:malayaleecoder)]

(In reply to Florian Quèze [:florian] [:flo] from comment #10)
> Thanks for the updated patch!
> 
> (In reply to malayaleecoder from comment #8)
> > On running |mach build| I am getting a few errors,
> > which I am unable to decipher. Can you help me out in finding what is
> > causing this?
> 
> Could you please pastebin the error with some context?

It's fixed right now. The problem was that I made a mistake by replacing a file by another.
Thank You for your concern.

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8719126 [details] [diff] [review]
Bug1238537_v3.diff

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

Thanks! Unfortunately, the patch no longer applies, most likely because of http://hg.mozilla.org/mozilla-central/rev/6c56f5dad3b2 Could you please update it?

::: browser/components/nsBrowserContentHandler.js
@@ +242,3 @@
>  }
>  
>  function getMostRecentWindow(aType) {

It looks like this function is never actually used, so we could just remove it. Sorry for missing this during the previous review.

Comment 14

[Vishnu (:malayaleecoder)]

Attachment: Bug1238537_v4.diff

Done. Please have a look :-)

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8719547 [details] [diff] [review]
Bug1238537_v4.diff

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

Looks good to me, thanks!

Comment 16

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/61db19d669b711a03ea2f9ac0c0967b78f5e708e
Bug 1238537 - make nsBrowserContentHandler.js use Services.jsm, r=florian.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/61db19d669b7

Comment 18

[Vishnu (:malayaleecoder)]

Thank You Florian for guiding me through in fixing this bug!