68187 - nsIWebBrowser::AddBrowserListener() needs to take an nsIWeakReference

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P1)

Description

[Judson Valeski]

right now we have the impl requirement that the listener impl nsIWeakReference. 
this should be reflected by replacing nsISupports w/ nsIWeakReference in the 
method signature.

Comment 1

[Judson Valeski]

Attachment: diffs to webbrowser impl and callers.

Comment 2

[Scott Collins]

This all looks good, except that you should prefer direct-initialization over
copy-initialization, that is, prefer constructions

  nsCOMPtr<nsIWeakReference> listener(do_QueryInterface(...));
  // ...
  nsCOMPtr<nsIWeakReference> thisListener(do_QueryInterface(listener));

over the form with '='.  After you fix that, sr=scc.

Comment 3

[Judson Valeski]

Attachment: updated diffs to reflect NS_GetWeakReference usage.

Comment 4

[Judson Valeski]

Attachment: patch to viewer so it uses the new api.

Comment 5

[Judson Valeski]

I've updated the patches as the direct QI to a nsIWeakReference was failing. I 
needed to be using NS_GetWeakReference (which I now am). I've also added a patch 
to viewer so it can use the new mechanism as well.

Comment 6

[Scott Collins]

More evidence, I guess, that I really need to provide a |do_GetWeakReference|.
This still isn't right.  To get a weak reference you need to say

  nsCOMPtr<nsIWeakReference> weakling( dont_AddRef(NS_GetWeakReference(...)) );

Comment 7

[Judson Valeski]

Attachment: updated patch to wrap w/ dont_AddRef().

Comment 8

[Judson Valeski]

Attachment: updated patch to viewer to use dont_addref

Comment 9

[Scott Collins]

Now I buy it.  One of us should file a bug, though, that |do_GetWeakReference|
is wanted.  sr=scc

Comment 10

[Conrad Carlen (not reading bugmail)]

Attachment: updated patch to CBrowserShell.cpp

Comment 11

[Conrad Carlen (not reading bugmail)]

The patch to CBrowserShell.cpp in patch id=25206 didn't quite compile - now it does.

Comment 12

[Judson Valeski]

thanks for the mac check conrad. these changes (including conrad's mods) are in.

Comment 13

[Michael Dunn]

Correction: Changing QA contact for the Embed API bugs to David Epstein.

Comment 14

[David Epstein]

in nsIWebBrowser.idl, we see that the listener accepts a weak reference:
void addWebBrowserListener(in nsIWeakReference aListener, in nsIIDRef aIID);