1167229 - Make nsBrowserGlue instantiations use nsISupports where nsIBrowserGlue is not needed

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Marco Bonardo [:mak]]

We have various callpoints where we instantiate nsBrowserGlue and QI it to nsIBrowserGlue, but effectively we don't need any of the methods from nsIBrowserGlue.

out of this list

http://mxr.mozilla.org/mozilla-central/search?string=nsIBrowserGlue

only browser-sets.inc and preferences/ need the nsIBrowserGlue interface, any other callpoint could either use getService(Ci.nsISupports) or getService(Ci.nsIObserver), depending on the case. Those callpoints should be updated to not rely on nsIBrowserGlue.

Comment 1

[Tyler Steiman]

Hey Marco, so essentially the invocations of nsIBrowserGlue just need to be removed according to the list above? Just want to make sure I'm understanding clearly. Thanks!!

Comment 2

[Marco Bonardo [:mak]]

not removed, basically when you getService or queryInterface you must provide an interface, in many of the above invocations we provide nsIBrowserGlue, but then we don't use any method from nsIBrowserGlue... those should be changed to nsISupports or nsIObserver depending on the purpose of the QI or getService.
Should be a trivial search and replace.

Comment 3

[Tyler Steiman]

(In reply to Marco Bonardo [::mak] from comment #2)
> not removed, basically when you getService or queryInterface you must
> provide an interface, in many of the above invocations we provide
> nsIBrowserGlue, but then we don't use any method from nsIBrowserGlue...
> those should be changed to nsISupports or nsIObserver depending on the
> purpose of the QI or getService.
> Should be a trivial search and replace.

I apologize in advance if this is a dumb question, but looking at the code how will I know if nsISupports should be used or nsIObserver? Most of the instances are :

  // Initialize nsBrowserGlue before Places.
  Cc["@mozilla.org/browser/browserglue;1"].getService(Ci.nsIBrowserGlue);

I'm still new to the code so I don't know how to identify which. If you could give me a little information on how these would be identified I would appreciate it!

Comment 4

[Marco Bonardo [:mak]]

if you look at http://mxr.mozilla.org/mozilla-central/source/browser/components/nsIBrowserGlue.idl you see the only method is provides is sanitize().

so if the instance is using .sanitize() it must be an nsIBrowserGlue.

if the instance is doing .observe(...) then it should be an nsIObserver (since that's the interface implementing observer)

otherwise an nsISupports will do (that means we are only instantiating the component, we don't need any of its methods).

Comment 5

[Tyler Steiman]

Attachment: Make nsBrowserGlue instantiations use nsISupports and nsIObserver where nsIBrowserGlue is not needed.

Marco, thank you for following up I appreciate the help. I've attached my proposed patch.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8615328 [details] [diff] [review]
Make nsBrowserGlue instantiations use nsISupports and nsIObserver where nsIBrowserGlue is not needed.

it is ok to set the review request on me (generally the mentor is also the reviewer)

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8615328 [details] [diff] [review]
Make nsBrowserGlue instantiations use nsISupports and nsIObserver where nsIBrowserGlue is not needed.

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

There is still something not completely correct


Also please add . r=mak at the end of the commit message

::: browser/components/nsBrowserGlue.js
@@ +2271,5 @@
>    classID:          Components.ID("{eab9012e-5f74-4cbc-b2b5-a590235513cc}"),
>  
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
>                                           Ci.nsISupportsWeakReference,
> +                                         Ci.nsISupports]),

This should not be modified, QueryInterface it the xpcom method that says which interfaces this component is implementing, it is still implementing the nsIBrowserGlue interface for now (at least until we remove sanitize() in bug 1167237, then we could also remove this)

::: browser/components/places/tests/unit/test_421483.js
@@ +12,1 @@
>                  QueryInterface(Ci.nsIObserver);

Since you are calling getService(Ci.nsIObserver) what you get is already an nsIObserver, so you can remove the further call to QueryInterface.

::: browser/components/test/browser_bug538331.js
@@ +114,1 @@
>                      QueryInterface(Ci.nsIObserver);

here you can directly getService(Ci.nsIObserver) and avoid the QI call.

Comment 8

[Tyler Steiman]

Attachment: nsBrowserGlue instantiations 2

An update of the patch with the mentioned changes.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8615414 [details] [diff] [review]
nsBrowserGlue instantiations 2

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

Thanks, it looks good

Comment 10

[Marco Bonardo [:mak]]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=774c61534645

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d538a04610e2

Comment 12

[[e-mail]]

(In reply to Marco Bonardo from comment #2)
> those should be changed to nsISupports or nsIObserver depending on the
> purpose of the QI or getService.
[getService() defaults to nsISupports, you only need to specify nsIObserver]

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d538a04610e2

Comment 14

[Marco Bonardo [:mak]]

(In reply to [e-mail] from comment #12)
> [getService() defaults to nsISupports, you only need to specify nsIObserver]

thanks for the pointer, it doesn't hurt to specify it, but I will keep that in mind for future.