72111 - fall back on nsILoadGroup when nsIChannel has no nsIPrompt

Status: VERIFIED FIXED

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

Description

[Judson Valeski]

Per the api review meeting today we're going to continue to have the nsIPrompt
accessible via the notification callbacks hanging of of nsIChannel, but we also
need it to be an attribute off of nsILoadGroup.

Comment 1

[Judson Valeski]

nsILoadGroup is about to freeze. I don't have this in our notes for api review.
why do we need nsIPrompt hanging off the load group? the callbacks aren't enough?

Comment 2

[Darin Fisher]

there are no notificationCallbacks on nsILoadGroup... given a nsILoadGroup, one
would have to get the defaultLoadRequest, QI it to a nsIChannel, then get the
notificationCallbacks from the nsIChannel, and then call GetInterface on the
notificationCallbacks to get a nsIPrompt.

obviously, this would fail if the defaultLoadRequest did not implement
nsIChannel.  so, is this good enough?  or, do people need to be able to 
associate notificationCallbacks more directly with a loadGroup?

Comment 3

[Dan M]

  Darin has added notificationCallbacks to nsILoadGroup as part of bug 74221. 
This is the right thing to add, rather than a naked nsIPrompt. So I believe the 
API part of this bug has been satisfied, and the work remaining is no longer 
absolutely required for 0.9. It'll probably get moved off to 0.9.1 as the date 
approaches. I'm changing the summary to reflect what I believe is the actual work 
remaining to be done.
  The problem is that sometimes channels are initialized without notification 
callbacks, or with callbacks that don't include an nsIPrompt. (That's a problem 
if the channel consumer needs to pose an nsIPrompt.) Remaining to be done is 
initialising loadgroups with a docshell notification callback, and teaching 
channels to return a new aggregate nsIInterfaceRequestor that internally will try 
first the channel's, and then the loadgroup's, when asked for an interface.
  Dissent?

Comment 4

[Brian Ryner (not reading)]

Dan, I can take a look at this for 0.9... I've dealt with
notificationCallbacks/nsIPrompt issues before.  Unless you've got some headway
on this, kick it over to me.

Comment 5

[Darin Fisher]

danm: i don't think we need to make nsIChannel::GetNotificationCallbacks do
any aggregation.  my suggestion was simply that a channel implementation could
ask its loadgroup's notificationCallbacks for a particular interface if its own
doesn't provide an implementation.  wouldn't this be sufficient?

Comment 6

[Dan M]

  Darin: I don't think so, because of the way it's used. The channel doesn't know 
that an nsIPrompt is what's wanted from it. It just gets a request for its 
callbacks (an nsIInterfaceRequestor). The requestor then calls GetInterface on 
the callbacks, after it's already been returned from the channel. It's only at 
that point that we know which interface was wanted, so it's at that point that we 
have to put the fallback code.

Comment 7

[Darin Fisher]

danm: i'm worried not about channel consumers getting at the callbacks, but
rather about the channel impl itself getting at the callbacks.  consumers
should view the notificationCallbacks attribute as a "simple" attribute.  i.e.,
what they set is what they get.

Comment 8

[Dan M]

(Darin's and my disagreement is still being worked on.)
Moving to 0.9.1 and cutting the link to embedding/API tracking bugs 65233 and 
70229 because the API part of this problem has been fixed as part of bug 74221. 
What remains is purely implementation. (Besides, most of the visual effect of 
this fix, once completed, has already been hacked around ^H^H^H fixed in bug 
44809.)

reassigning to bryner because he still wants it.

Comment 9

[Michael Dunn]

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

Comment 10

[Judson Valeski]

nominating 0.9.1

Comment 11

[Mike Pinkerton (not reading bugmail)]

what's the status on this? is this still an issue?

Comment 12

[Darin Fisher]

this is still an issue.  nsILoadGroup has a notificationCallbacks attribute,
but it is not currently implemented and the channel's do not currently use it.
but, i think rpotts may be working on a patch... though i'm not really sure.

Comment 13

[rpotts (gone)]

Yes...  I'm working on a patch which will cause each channel (without a 
notification callbacks) to 'inherit' the notification callbacks of the load 
group when it added...

-- rick

Comment 14

[Brian Ryner (not reading)]

not critical for emojo or embedding (to my knowledge).

Comment 15

[Jaime Rodriguez, Jr.]

Marking as nsbranch- based on bryner comments.

Comment 16

[selmer (gone)]

really marking minus now :-)

Comment 17

[Brian Ryner (not reading)]

rpotts or darin, do you want to take this bug?  I've been out of the necko loop
for awhile.

Comment 18

[Darin Fisher]

taking

Comment 19

[Darin Fisher]

-> 0.9.9

Comment 20

[Darin Fisher]

vinay: you recently fixed this bug for the http channel... you interested in
doing the same for the other channels?

Comment 21

[Vinay Badami]

assign it to me.

Comment 22

[Darin Fisher]

-> badami

Comment 23

[Vinay Badami]

Attachment: for FTP and gopher channels

Comment 24

[Darin Fisher]

Comment on attachment 66281 [details] [diff] [review]
for FTP and gopher channels

>Index: gopher/src/nsGopherChannel.cpp

>+            nsCOMPtr<nsIPrompt> prompt(mPrompter);
>+            if (!prompt) {

i think you could do away with the |prompt| variable and just assign the
loadgroup's
nsIPrompt impl into mPrompter.



>Index: ftp/src/nsFtpConnectionThread.cpp
> 
>+
>             rv = mAuthPrompter->PromptUsernameAndPassword(nsnull,

intentional?



>@@ -1809,6 +1810,16 @@

>+                mPrompter = do_GetInterface(cbs, &rv);
>+        }

no need to capture |rv| if it's not going to be used.

otherwise, sr=darin (please submit a cleaned up patch)

Comment 25

[Vinay Badami]

Attachment: incorporating darins comments

Comment 26

[Darin Fisher]

Comment on attachment 66428 [details] [diff] [review]
incorporating darins comments

sr=darin

Comment 27

[Bradley Baetz (:bbaetz)]

Comment on attachment 66428 [details] [diff] [review]
incorporating darins comments

As darin mentioned, you don't need to capture &rv in the do_GetInterface for
gopher.

r=bbaetz with that fixed.

Comment 28

[Bradley Baetz (:bbaetz)]

Comment on attachment 66428 [details] [diff] [review]
incorporating darins comments

As darin mentioned, you don't need to capture &rv in the do_GetInterface for
gopher.

r=bbaetz with that fixed.

Comment 29

[Vinay Badami]

Fixed with checkins
Checking in ftp/src/nsFtpConnectionThread.cpp;
/cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp,v  <-- 
nsFtpConnectionThread.cpp
new revision: 1.221; previous revision: 1.220
done
Checking in gopher/src/nsGopherChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/gopher/src/nsGopherChannel.cpp,v  <-- 
nsGopherChannel.cpp
new revision: 1.19; previous revision: 1.18
done

Comment 30

[David Epstein]

verified against Mozilla 0.9.8 Gecko 20020214.