46856 - nsIWebProgressListener changes.

Status: VERIFIED FIXED

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

Description

[Judson Valeski]

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

Comment 1

[Warren Harris]

I think Rick should own this one.

Comment 2

[Doug Turner (:dougt)]

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

Comment 3

[lchiang]

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.

Comment 4

[rpotts (gone)]

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 :-)

Comment 5

[rpotts (gone)]

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 :-)

Comment 6

[Paul Wyskoczka]

Updating QA Contact

Comment 7

[Michael Dunn]

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

Comment 8

[rpotts (gone)]

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

Comment 9

[Radha on family leave (not reading bugmail)]

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

Comment 10

[Blake Ross]

Missed the 0.9.3 train.

Comment 11

[Judson Valeski]

and the 0.9.4 train.

Comment 12

[rpotts (gone)]

-> 0.9.6

Comment 13

[rpotts (gone)]

-> 0.9.7

Comment 14

[rpotts (gone)]

Attachment: changes to nsIWebProgress and nsIWebProgressListener

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'

Comment 15

[vidur (gone)]

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 16

[vidur (gone)]

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 17

[Judson Valeski]

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.

Comment 18

[Adam Lock]

r=adamlock too.

Comment 19

[rpotts (gone)]

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 :-)

Comment 20

[rpotts (gone)]

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.

Comment 21

[rpotts (gone)]

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 22

[Aaron Leventhal]

Comment on attachment 60547 [details] [diff] [review]
changes to nsIWebProgress and nsIWebProgressListener

r=aaronl
for the parts affecting accessibility

Comment 23

[David Epstein]

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.

Comment 24

[Judson Valeski]

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

Comment 25

[rpotts (gone)]

Attachment: new patch w/ security changes too...

Comment 26

[rpotts (gone)]

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

Comment 27

[Kai Engert (:KaiE:)]

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 28

[Darin Fisher]

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

Comment 29

[Brendan Eich [:brendan]]

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

/be

Comment 30

[rpotts (gone)]

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

Comment 31

[rpotts (gone)]

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

Comment 32

[John P Baker]

Appear to have missed tabbrowser.xml (bug 145181)

Comment 33

[rpotts (gone)]

The chages have been checked into the trunk.

Comment 34

[Dawn Endico]

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

Comment 35

[rpotts (gone)]

checked into the branch too !

closing out.

Comment 36

[David Epstein]

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.

Comment 37

[David Epstein]

verified.

Comment 38

[Judson Valeski]

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