-Need to split out aStateFlags for onStateChange into "who" and "what" parameters. -constant names need to be Java style and need a common prefix. -Need a name attribute in nsIRequest. -Consider onStatusChange for error notification and get rid of status parameter to onStateChange -Need a way to correlate a notification with a nsIWebBrowser instance. You should be able to do a GetInterface() on the nsIWebProgress passed into the notification method to get a nsIWebBrowser. -Rick to consider adding nsIDOMWindow as a parameter to the notification methods. -Need to add a filterMask attribute that is queried on registration and is used to filter which notifications get to the listener.

I think Rick should own this one.

Rick, as we discussed before, we could move OnSecurityChange into onStateChange. This is your call if you want to do this.

per email with Jud, changing nsbeta3+ to nsbeta3- on all "embed" keyword bugs since embedding changes will not be made in the mn6 branch. If you feel this bug fix needs to go into mn6 branch, please list the reasons/user impact/risk and nominate for rtm. Thanks.

I've just checked in some changes to nsIWebProgress.idl to add an read-only attribute for the DOMWindow. This made more sense than passing the DOMWindow along as yet-another-argument to the nsIWebProgressListener notification methods... So, one down and 5 more to go :-)

I just noticed that warren had already added a 'name' attribute to nsIRequest, and it looks like all of the constant names are Java style... So that's three down :-)

Updating QA Contact

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

I'm moving this one out to 0.9.1 I believe that the only work left is splitting out the aStateFlags in nsIWebProgressListener::OnStateChange(...) into "who" and "what" parameters...

As per discussions with Rick, I think this can wait 'til 0.9.3.

Missed the 0.9.3 train.

and the 0.9.4 train.

-> 0.9.6

-> 0.9.7

This patch makes the following interface changes: 1. nsIWebProgress::AddProgressListener(...) takes and additional argument which is a 'mask' indicating the types of notifications the caller wants to receive. 2. the 3rd argument to nsIWebProgressListener::OnStatusChange(...) has been changed from a 'long' to 'unsigned long' 3. the 4th argument to nsIWebProgressListener::OnStatusChange(...) has been changed from a 'unsigned long' to 'nsresult'

Does the interface change to nsIWebProgress::AddProgressListener also apply to nsIWebBrowser::AddWebBrowserListener? The latter is often used by embedding customers to register as web progress listeners, but it's also used to register other types of listeners.

Comment on attachment 60547 [details] [diff] [review] changes to nsIWebProgress and nsIWebProgressListener - The passing of the status change notification up the parent change in nsDocLoaderImpl::FireOnStatusChange is new, yes? How scared should we be about it? - A nit: In nsDocLoaderImpl::GetListenerInfo(nsIWeakReference *aListener), use a NS_STATIC_CAST just for consistency.

Comment on attachment 60547 [details] [diff] [review] changes to nsIWebProgress and nsIWebProgressListener >Index: accessible/src/nsRootAccessible.cpp <snip> > NS_ASSERTION(mWebProgress, "Could not get nsIWebProgress for nsRootAccessible"); >- mWebProgress->AddProgressListener(this); no need for the new call here? >Index: accessible/src/base/nsRootAccessible.cpp <snip> > NS_ASSERTION(mWebProgress, "Could not get nsIWebProgress for nsRootAccessible"); >- mWebProgress->AddProgressListener(this); same here? >Index: embedding/tests/mfcembed/BrowserImplWebPrgrsLstnr.cpp <snip> >- m_pBrowserFrameGlue->UpdateStatusBarText(aMessage); >+ if(m_pBrowserFrameGlue) >+ m_pBrowserFrameGlue->UpdateStatusBarText(aMessage); indentation/tab/space problem. >Index: mailnews/compose/src/nsMsgCompose.cpp >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v >retrieving revision 1.310 >diff -u -r1.310 nsMsgCompose.cpp >--- mailnews/compose/src/nsMsgCompose.cpp 29 Nov 2001 04:56:42 -0000 1.310 >+++ mailnews/compose/src/nsMsgCompose.cpp 5 Dec 2001 19:50:07 -0000 >@@ -976,7 +976,7 @@ > } > } > >- mProgress->OnStateChange(nsnull, nsnull, nsIWebProgressListener::STATE_START, 0); >+ mProgress->OnStateChange(nsnull, nsnull, nsIWebProgressListener::STATE_START, NS_OK); NS_OK == '0' in this case? >+ mListenerInfoList.Clear(); >+ mListenerInfoList.Compact(); compact doesn't do a clear for you? > nsresult nsDocLoaderImpl::GetMaxTotalProgress(PRInt32 *aMaxTotalProgress) <snip> >- mListenerList->Compact(); >+ mListenerInfoList.Compact(); If compact doesn't do a clear, should you be clearing here too? >- mListenerList->Compact(); >+ mListenerInfoList.Compact(); same here. >- mListenerList->Compact(); >+ mListenerInfoList.Compact(); and here >- mListenerList->Compact(); >+ mListenerInfoList.Compact(); and here >- mListenerList->Compact(); >+ mListenerInfoList.Compact(); and here >Index: uriloader/base/nsIWebProgressListener.idl >=================================================================== >RCS file: /cvsroot/mozilla/uriloader/base/nsIWebProgressListener.idl,v Document that impls now should not return NS_ERROR_NOT_IMPLEMENTED? r=valeski after return val doc'ing and if everything else I raised is ok.

r=adamlock too.

answers to vidur's comments: a) passing the onStatusChange notification up the parent tree *is* new. i'm sure we should be very scared of this!!! tho i haven't seen any problems... i did this for api consistancy -- because without it there is no way to propagate status notifications without listening to *every* docshell... b) Since nsIWebBrowser::AddBrowserListener(...) is a generic registration method, it's semantics are nsIWebPProgress::NOTIFY_ALL... Since AddBrowserListener is merely a convienence method. i thought that this was acceptable in the general case. If filtering *is* necessary, it is still possible by bypassing this convinience function and registering directly with nsIWebProgress... c) i've fixed up all the casts to use NS_STATIC_CAST :-)

answers to jud's comments... ============================ a) if you look at the current code, it appears that the progress listener is being registered TWICE!. Of course, fixing this bug could cause other problems :-) i'm CC'ing aaron to give him a 'heads up'. b) yes, NS_OK maps to 0. nsMsgCompose has overloaded this argument in a VERY unpleasent way... but in this instance, the '0' being passed out is MEANT to represent NS_OK ;-) c) if i understand nsVoidArray::Compact() correctly, it frees up any 'unused' memory by reallocing the array to be exactly 'size'. i believe that nsVoidArray::Clear() just sets 'size' to 0 (without compacting). So, calling them together effectly frees up ALL of the memory associated with the array... while calling compact() alone merely frees up unused space... d) you are correct... bug #88460 is tracking the issue about nsIWebProgressListener methods returning NS_ERROR_XXX rather than NS_OK when not implemented... I'll add comments to the interface.

I forgot to mention that this patch is not meant to freeze the nsIWebProgress and nsIWebProgressListener idl files. I plan to make another documentation pass before i mark them frozen.

Comment on attachment 60547 [details] [diff] [review] changes to nsIWebProgress and nsIWebProgressListener r=aaronl for the parts affecting accessibility

I talked to Rick about cleaning up the masking for security flags in nsIWebProgressListener.idl (used in OnSecurityChange()). STATE_IS_INSECURE and STATE_SECURE_HIGH are both 0 (0x00000000). Suggest starting at 1 in low end.

note: a new nsIWebProgressListener impl is on it's way in http://bugzilla.mozilla.org/show_bug.cgi?id=72906

Comment on attachment 60547 [details] [diff] [review] changes to nsIWebProgress and nsIWebProgressListener the new patch is basically the same as this one... however, it has the API changes to onSecurityChange(...) and changes the security bit-masks so they are all non-zero values...

r=kaie on the changes to files in mozilla/security and on changing the numeric values of STATE_IS_INSECURE and STATE_SECURE_HIGH.

Comment on attachment 83091 [details] [diff] [review] new patch w/ security changes too... - some wierd indentation in embedding/tests/mfcembed/BrowserImplWebPrgrsLstnr.cpp - are the changes to netwerk/protocol/file/src/nsFileChannel.cpp really intended? they seem fine to me, fwiw. - how large does mListenerInfoList grow typically? will GetListenerInfo ever be costly? otherwise, this patch looks good to me. sr=darin

If this is supposed to be in mozilla1.0, we need r= and drivers' buy-in. Jud, can you drive this one? /be

hey brendan, how many r= do i need? i've got one from vidur, valeski and adam on the entire patch and aaron and kai on the pieces that changed (above and beyond argument changes)... do i need more? i was just about to ask drivers for approval... now that darin has sr'ed it (and i will address darin's issues with the patch). -- rick

hey darin, thanks for catching the nsFileChannel changes :-) those will NOT be part of the check in... I've fixed up the indenting in BrowserImplWebPrgrsLstnr.cpp (as best i could :-)) The mListenerInfoList contains an entry for each registered nsIWebProgressListener (on that particular WebProgress). Currently, that is a fairly small number (< 8) so lookups should be cheap... However, if it turns out table lookups are too expensive, it can be optimized later... -- rick

Appear to have missed tabbrowser.xml (bug 145181)

The chages have been checked into the trunk.

Comment on attachment 83091 [details] [diff] [review] new patch w/ security changes too... a=valeski,chomann,brendan,etc please check in to 1.0 branch asap for inclusion in rc3. -thanks

checked into the branch too ! closing out.

verified patch against Mozilla 1.0.0 Gecko 20020603 build. Main changes are: 1) nsIWebProgress->AddProgressListener() now has notification flags. 2) nsIWebProgressListener->OnStateChange(), stateFlags param now PRUint32, status param now nsresult 3) nsIWebProgressListener->OnSecurityChange(), state param now PRUInt32 4) returning NS_OK for all notifications (except for failures) 5) nsIWebProgressListener flags remapped to start at 1 bit at low end Everything looks fine, except for a few cases (which probably is just added code since the patch checkin). Would be helpful if the concerned parties could comment on these: 1) in /mozilla/accessible/src/base/nsRootAccessible.cpp: * added NOTIFY_STATE_DOCUMENT as an OR condition in AddProgressListener() * NS_NOTREACHED replaced by scroll position listener code in OnStateChange(). 2) in /mozilla/editor/composer/src/nsEditingSession.cp * Replaced NOTIFY_STATE_ALL with NOTIFY_STATE_NETWORK | NOTIFY_STATE_DOCUMENT in AddProgressListener(), inside nsEditingSession::PrepareForEditing(). 3) under /mozilla/embedding/components/printingui/src, /gtk/ now changed to /unixshared directory. * in /mozilla/embedding/components/printingui/src/unixshared/nsPrintProgress.cpp, OnLocationChange() still returns NS_ERROR_NOT_IMPLEMENTED. 4) in /mozilla/netwerk/protocol/file/src/nsFileChannel.cpp: * the patch has mFileTransport->SetNotificationCallbacks(this, ((mLoadFlags & LOAD_BACKGROUND) ? nsITransport::DONT_REPORT_PROGRESS : 0)). But the source file has mFileTransport->SetNotificationCallbacks(mCallbacks, mLoadFlags & LOAD_BACKGROUND)) 5) in /mozilla/uriloader/base/nsDocLoacer.cpp: * the patch uses NOTIFY_STATUS for NS_STATIC_CAST in nsListenerInfo *. Source file uses NOTIFY_LOCATION.

verified.

*** Bug 88460 has been marked as a duplicate of this bug. ***