Closed Bug 1238537 Opened 4 years ago Closed 4 years ago

make nsBrowserContentHandler.js use Services.jsm

Categories

(Firefox :: General, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: florian, Assigned: malayaleecoder, Mentored)

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 3 obsolete files)

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.
I would like to take this as my first bug. Can you please guide me through?
(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?
(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!
(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.
(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!
Attached patch Bug1238537_v1.diff (obsolete) — Splinter Review
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 :)
Assignee: nobody → malayaleecoder
Status: NEW → ASSIGNED
Flags: needinfo?(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.
Attachment #8718720 - Flags: feedback+
Flags: needinfo?(florian)
Attached patch Bug1238537_v2.diff (obsolete) — Splinter Review
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.
Attachment #8718720 - Attachment is obsolete: true
Flags: needinfo?(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).
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?
Flags: needinfo?(florian)
Attached patch Bug1238537_v3.diff (obsolete) — Splinter Review
Florian, I have made the above said changes in this attachment. Please check and tell me if any corrections are needed.

Thank You.
Attachment #8718906 - Attachment is obsolete: true
Flags: needinfo?(florian)
Attachment #8719126 - Flags: review?(florian)
(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 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.
Attachment #8719126 - Flags: review?(florian) → feedback+
Done. Please have a look :-)
Attachment #8719126 - Attachment is obsolete: true
Comment on attachment 8719547 [details] [diff] [review]
Bug1238537_v4.diff

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

Looks good to me, thanks!
Attachment #8719547 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/61db19d669b711a03ea2f9ac0c0967b78f5e708e
Bug 1238537 - make nsBrowserContentHandler.js use Services.jsm, r=florian.
https://hg.mozilla.org/mozilla-central/rev/61db19d669b7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Thank You Florian for guiding me through in fixing this bug!
Flags: needinfo?(florian)
You need to log in before you can comment on or make changes to this bug.