nsIWebProgressListener changes.

VERIFIED FIXED in mozilla1.0

Status

()

Core
Embedding: APIs
P1
normal
VERIFIED FIXED
18 years ago
16 years ago

People

(Reporter: Judson Valeski, Assigned: rpotts (gone))

Tracking

({topembed+})

Trunk
mozilla1.0
x86
Windows 98
topembed+
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta3-] [driver:valeski])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

18 years ago
-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.
(Reporter)

Updated

18 years ago
Keywords: embed

Updated

18 years ago
Keywords: nsbeta3

Comment 1

18 years ago
I think Rick should own this one.
Assignee: warren → rpotts
(Reporter)

Updated

18 years ago
Whiteboard: [nsbeta3+]
(Reporter)

Updated

18 years ago
Target Milestone: --- → M18
(Reporter)

Updated

18 years ago
Priority: P3 → P1

Comment 2

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

Comment 3

18 years ago
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.
Whiteboard: [nsbeta3+] → [nsbeta3-]
(Assignee)

Comment 4

18 years ago
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 :-)

(Assignee)

Comment 5

18 years ago
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

18 years ago
Updating QA Contact
QA Contact: jrgm → mdunn
(Reporter)

Updated

18 years ago
Blocks: 70229

Updated

18 years ago
Target Milestone: M18 → ---
(Reporter)

Updated

17 years ago
Target Milestone: --- → mozilla0.9

Comment 7

17 years ago
Correction: Changing QA contact for the Embed API bugs to David Epstein.
QA Contact: mdunn → depstein
(Assignee)

Comment 8

17 years ago
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...

Target Milestone: mozilla0.9 → mozilla0.9.1
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.1 → mozilla0.9.2
As per discussions with Rick, I think this can wait 'til 0.9.3.  
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Comment 10

17 years ago
Missed the 0.9.3 train.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
(Reporter)

Comment 11

17 years ago
and the 0.9.4 train.
Target Milestone: mozilla0.9.4 → mozilla0.9.5
(Assignee)

Comment 12

17 years ago
-> 0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
(Assignee)

Comment 13

17 years ago
-> 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
(Assignee)

Updated

17 years ago
Blocks: 99639
(Assignee)

Comment 14

17 years ago
Created attachment 60547 [details] [diff] [review]
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

17 years ago
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.
Keywords: nsbeta3

Comment 16

17 years ago
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.
(Reporter)

Comment 17

17 years ago
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

17 years ago
r=adamlock too.
(Assignee)

Comment 19

17 years ago
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 :-)

(Assignee)

Comment 20

17 years ago
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.
(Assignee)

Comment 21

17 years ago
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

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

r=aaronl
for the parts affecting accessibility
(Reporter)

Updated

17 years ago
Attachment #60547 - Flags: review+

Comment 23

17 years ago
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.
(Reporter)

Updated

17 years ago
Keywords: embed → topembed
Target Milestone: mozilla0.9.7 → mozilla0.9.9
(Reporter)

Updated

17 years ago
Blocks: 88460
(Reporter)

Updated

17 years ago
Keywords: topembed → topembed+
Target Milestone: mozilla0.9.9 → mozilla1.0
(Reporter)

Comment 24

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

Updated

17 years ago
Keywords: mozilla1.0+
(Assignee)

Comment 25

16 years ago
Created attachment 83091 [details] [diff] [review]
new patch w/ security changes too...
(Assignee)

Comment 26

16 years ago
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...
Attachment #60547 - Attachment is obsolete: true

Comment 27

16 years ago
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

16 years ago
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
Attachment #83091 - Flags: superreview+
If this is supposed to be in mozilla1.0, we need r= and drivers' buy-in.  Jud,
can you drive this one?

/be
Whiteboard: [nsbeta3-] → [nsbeta3-] [driver:valeski]
(Assignee)

Comment 30

16 years ago
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
(Assignee)

Comment 31

16 years ago
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

16 years ago
Appear to have missed tabbrowser.xml (bug 145181)
(Assignee)

Comment 33

16 years ago
The chages have been checked into the trunk.
(Reporter)

Updated

16 years ago
Attachment #83091 - Flags: approval+

Comment 34

16 years ago
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
(Assignee)

Comment 35

16 years ago
checked into the branch too !

closing out.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 36

16 years ago
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

16 years ago
verified.
Status: RESOLVED → VERIFIED
(Reporter)

Comment 38

16 years ago
*** Bug 88460 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.