Closed Bug 1167229 Opened 5 years ago Closed 5 years ago

Make nsBrowserGlue instantiations use nsISupports where nsIBrowserGlue is not needed

Categories

(Firefox :: General, defect)

defect
Not set
Points:
1

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: mak, Assigned: ma.steiman, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 1 obsolete file)

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.
Flags: qe-verify-
Flags: firefox-backlog+
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!!
Assignee: nobody → ma.steiman
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77)
(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!
Flags: needinfo?(mak77)
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).
Flags: needinfo?(mak77)
Marco, thank you for following up I appreciate the help. I've attached my proposed patch.
Flags: needinfo?(mak77)
Attachment #8615328 - Flags: review?(gavin.sharp)
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)
Flags: needinfo?(mak77)
Attachment #8615328 - Flags: review?(gavin.sharp) → review?(mak77)
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.
Attachment #8615328 - Flags: review?(mak77) → review-
An update of the patch with the mentioned changes.
Attachment #8615328 - Attachment is obsolete: true
Attachment #8615414 - Flags: review?(mak77)
Comment on attachment 8615414 [details] [diff] [review]
nsBrowserGlue instantiations 2

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

Thanks, it looks good
Attachment #8615414 - Flags: review?(mak77) → review+
(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]
https://hg.mozilla.org/mozilla-central/rev/d538a04610e2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
(In reply to neil@parkwaycc.co.uk 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.
You need to log in before you can comment on or make changes to this bug.