451401 - NewAPI: Add access to underlying XPCOM objects

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, enhancement)

Description

[Pelle Johnsen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: 

It should be possible to get access to the underlying XPCOM objects for gaining using the functionality not yet provided by the new API. A couple of options for this has been suggested:

 (1) return a nsIWebBrowser* but this is often not the desired interface
 (2) return a nsISupports* and then use QueryInterface/GetInterface
 (3) return a nsIInterfaceRequestor*
 (4) have a GetInterface(const IID&, void**) directly on MozView, but this doesn't play too well with nsCOMPtr

So far (3) looks most promising and allows client code like this:

#include "nsIInterfaceRequestor.h"
#include "nsIInterfaceRequestorUtils.h"
#include "nsIDocShell.h"

...

nsCOMPtr<nsIDocShell> docshell =
  do_GetInterface(pMozView->GetInterfaceRequestor());


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Pelle Johnsen]

Attachment: Patch for option (3) to add access to XPCOM

Added preliminary patch for option (3)

Comment 2

[Pelle Johnsen]

Comment on attachment 334696 [details] [diff] [review]
Patch for option (3) to add access to XPCOM

r? dcamp, mfinkle

Comment 3

[Boris Zbarsky [:bzbarsky]]

Shouldn't there be some API documentation saying the caller needs to release this?  Or should we just be using already_AddRefed?

Comment 4

[Pelle Johnsen]

Hmmm, it would be nicest if the embedder didn't have to explicitly release it. But it should at least be very clear that releasing is required. Maybe it is better to have it like this: nsresult GetInterfaceRequestor(nsIInterfaceRequestor** aInterfaceRequestor) and then have embedders use getter_AddRefs

As an alternative MozView could hold the raw pointer, and then we just document that it is only valid as long as the MozView instance lives.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> But it should at least be very clear that releasing is required.

Right.  So is there a reason we're not using already_AddRefed?  Embedders can use nsCOMPtr, right?

Comment 6

[Pelle Johnsen]

Yes and they should if they want to use this :) The thing I don't like about already_AddRefed is that I believe it will need to #include <nsCOMPtr.h> even for embedders who are only using the new api and don't need access to xpcom.

My hope was that users of the new api would only have to #include the new stuff and thus didn't have to setup the include paths needed for xpcom.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah.  Then yeah, out param is probably better, with comments about why it's that way.

Comment 8

[Benjamin Smedberg]

Technically, you can forward-declare

template<class T>
class already_AddRefed;

Not sure if you actually want to, of course.

Comment 9

[Pelle Johnsen]

Attachment: Changed to use out param

Created new patch using out param

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 336030 [details] [diff] [review]
Changed to use out param

Yeah, we'll need something like this for a while.

Comment 11

[Pelle Johnsen]

Pushed to http://hg.mozilla.org/incubator/embedding