Judson Valeski Reporter
	Description • 24 years ago

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.

	Judson Valeski Reporter
	Comment 1 • 24 years ago

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?

	Darin Fisher
	Comment 2 • 24 years ago

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?

	Dan M
	Comment 3 • 24 years ago

  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?

	Brian Ryner (not reading)
	Comment 4 • 24 years ago

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.

	Darin Fisher
	Comment 5 • 24 years ago

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?

	Dan M
	Comment 6 • 24 years ago

  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.

	Darin Fisher
	Comment 7 • 24 years ago

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.

	Dan M
	Comment 8 • 24 years ago

(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.

	Michael Dunn
	Comment 9 • 24 years ago

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

	Judson Valeski Reporter
	Comment 10 • 24 years ago

nominating 0.9.1

	Mike Pinkerton (not reading bugmail)
	Comment 11 • 23 years ago

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

	Darin Fisher
	Comment 12 • 23 years ago

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.

	rpotts (gone)
	Comment 13 • 23 years ago

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

	Brian Ryner (not reading)
	Comment 14 • 23 years ago

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

	Jaime Rodriguez, Jr.
	Comment 15 • 23 years ago

Marking as nsbranch- based on bryner comments.

	selmer (gone)
	Comment 16 • 23 years ago

really marking minus now :-)

	Brian Ryner (not reading)
	Comment 17 • 23 years ago

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

	Darin Fisher
	Comment 18 • 23 years ago

taking

	Darin Fisher
	Comment 19 • 23 years ago

-> 0.9.9

	Darin Fisher
	Comment 20 • 23 years ago

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

	Vinay Badami Assignee
	Comment 21 • 23 years ago

assign it to me.

	Darin Fisher
	Comment 22 • 23 years ago

-> badami

	Darin Fisher
	Comment 24 • 23 years ago

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)

	Darin Fisher
	Comment 26 • 23 years ago

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

sr=darin

	Bradley Baetz (:bbaetz)
	Comment 27 • 23 years ago

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.

	Bradley Baetz (:bbaetz)
	Comment 28 • 23 years ago

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.

	Vinay Badami Assignee
	Comment 29 • 23 years ago

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

	David Epstein
	Comment 30 • 23 years ago

verified against Mozilla 0.9.8 Gecko 20020214.