Freeze nsIWebProgress and nsIWebProgressListener Please refer to http://www.mozilla.org/projects/embedding/EmbedInterfaceFreeze.html for the issues to be addressed, if any, for this interface. Please follow the guidelines outlined in "How to mark an interface as frozen?" section of the document above.

->0.9.6

-> 0.9.7

See bug 99642 comment 13

making bug 86521 a dependency. need to decide what to do with nsIRequest parameter in nsIWebProgressListener::OnLocationChange().

mass reassign of various rpotts bugs to me

Would it be a good idea to change the parameters of onProgressChange to use 64-bit integers, rather than 32-bit ones, to support large files?

Looks like we may need this for accessibility -- unless I can get sr= for bug 208898

a number of other interfaces have been frozen with 32-bit file size limitations unfortunately :( complete support for 64-bit file lengths might have to be a mozilla-2.0 thing.

I think we may need to freeze these interfaces as is. Many extensions and embedding applications depend on these interfaces already. 64-bit support may need to happen via nsIWebProgressListener2 (perhaps with an onProgressChange64 method) :-/

An attempt at cleaning up some of the comments for nsIWebProgress.idl and nsIWebProgressListener.idl. Let's not pretend that we can actually fix snafus in these interfaces. They're de facto frozen. If anyone has any useful documentation to add to the IDL files, please speak up. Otherwise, let's get these stamped for 1.8.

erm. i have a change to the mozilla implementation of nsIWebProgress. i've lost track of where it is in the mozilla review process but i'm really uncomfortable with this stuff freezing before that lands. + * Called when the window being watched changes the location that is + * currently. is currently.. what? + * @return NS_OK should always be returned. that's not a return, lose it :)

further, while you're cleaning the documentation, it is still wrong.

> further, while you're cleaning the documentation, it is still wrong. uhm... care to elaborate? :-/

(In reply to comment #11) > erm. i have a change to the mozilla implementation of nsIWebProgress. i've lost track of where it is in > the mozilla review process but i'm really uncomfortable with this stuff freezing before that lands. bug 253550, waiting for my review...

OK, but the patch in bug 253550 doesn't change either of these interfaces. Sure, it may affect how they are used, but that's a matter of proper documentation. For example, I don't see any of the existing nsIWebProgressListener implementations being modified as part of the patch for bug 253550. Should they be?

the point is that i don't want those things frozen until my changes land. they may or may not change the behavior of the interface, and the consumers may or may not need changes to handle them, but they are critical to my product and i'm not going to wait 2 years for another interface which may happen 5 years from now.

I'm not likely to have a chance to give this the careful look it deserves until I get back into town a week and a half from now...

Comment on attachment 157221 [details] [diff] [review] v1 patch I am also opposed to doing behavioural changes to nsDocLoader after freezing webprogresslistener. But if 1.8beta is your target, that should be easily doable. As I recall addProgressListener requires the listener to also implement nsISupportsWeakReference... please document that too. const unsigned long STATE_IS_REQUEST = 0x00010000; const unsigned long STATE_IS_DOCUMENT = 0x00020000; const unsigned long STATE_IS_NETWORK = 0x00040000; const unsigned long STATE_IS_WINDOW = 0x00080000; The meaning of these flags is somewhat unclear... it'd be really good to document what they are supposed to do before freezing what they do.

Comment on attachment 157221 [details] [diff] [review] v1 patch darin is working on a new patch

Revised patch, including much improved documentation.

Comment on attachment 159664 [details] [diff] [review] v1.1 patch nsIWebProgress.idl * @status UNDER_REVIEW you aren't marking it frozen yet? for addProgressListener: there's a third return value it may return, NS_ERROR_OUT_OF_MEMORY... for completeness, it would probably be good to mention it too nsIWebProgressListener.idl + * These flags indicate the various states that documents and requests may + * transition through as they are being loaded. each of those flags occurs at most once, right? when STATE_REDIRECTING, will a STATE_STOP happen? events corresponding to a child + * nsIWebProgress instance. hmm... I'd probably make this "corresponding to a subdocument" - the interface does not require a tree of nsIWebProgress instances. (hm... ok, implicitly it does, because a subdocument will have a different domwindow) + * STATE_IS_WINDOW hmm, code looks like this will only be set for the stop onStateChange, not for the start one... + * @param aStatus + * This value is not an error code. Instead, it is a numeric value + * that indicates the current status of the request. ok, that's nice to know. it does not say how to find out what the possible values are or what they mean, though :) (it's probably nsITransport/nsISocketTransport status codes, right?) this is excellent documentation. thank you for doing this!

i can't look at this before next tuesday, but i definitely need to before it's frozen.

> you aren't marking it frozen yet? not in this patch. i want to get the big patch in quickly, and then take more time before marking it frozen. i don't want this patch hanging around in the bug waiting for final decisions, etc. we can always do a followup patch to revise anything that needs revising. > for addProgressListener: there's a third return value it may return, > NS_ERROR_OUT_OF_MEMORY... for completeness, it would probably be good to > mention it too maybe. NS_ERROR_OUT_OF_MEMORY is pretty much implied as a possible exception from any XPCOM method, right? i'd have to add mention of it to every XPCOM method declaration, which would seem like noise to me. > each of those flags occurs at most once, right? right. i'll clarify that in the docs. > when STATE_REDIRECTING, will a STATE_STOP happen? good question. the answer i believe is yes, but i should confirm and document the exact behavior. > events corresponding to a child > + * nsIWebProgress instance. > > hmm... I'd probably make this "corresponding to a subdocument" - the interface > does not require a tree of nsIWebProgress instances. (hm... ok, implicitly it > does, because a subdocument will have a different domwindow) ok > + * STATE_IS_WINDOW > > hmm, code looks like this will only be set for the stop onStateChange, not for > the start one... yeah, that's wierd. i don't fully grok the point of STATE_IS_WINDOW, nor the reason that we get two STATE_STOP events for every document that completes. perhaps we should add STATE_IS_WINDOW to the document STATE_START event. > ok, that's nice to know. it does not say how to find out what the possible > values are or what they mean, though :) > (it's probably nsITransport/nsISocketTransport status codes, right?) yes, that is true. i'm not sure yet exactly how we want to go about exposing those values. i'm not too happy with the fact that they collide with other nsresult values because it'd be nice to be able to use nsIErrorService (or something like it) to further explain any given nsresult code in the system.

I checked in the v1.2 patch on the trunk. If anyone has any further comments or documentation suggestions for these interfaces, please speak up. I plan to mark these interfaces frozen soon. I will be posting a similar message to n.p.m.embedding shortly.

Comment on attachment 160455 [details] [diff] [review] v1.2 patch > const unsigned long STATE_IS_REQUEST = 0x00010000; > const unsigned long STATE_IS_DOCUMENT = 0x00020000; > const unsigned long STATE_IS_NETWORK = 0x00040000; > const unsigned long STATE_IS_WINDOW = 0x00080000; I've had to deal with this set of states a few times, and I've never really understood them. These comments help some, but I'm not sure they're entirely correct, at least in terms of what we do now, and I'm skeptical that these descriptions are clear enough for freezing. For example: >+ * STATE_IS_REQUEST >+ * This flag indicates that the state transition is for a non-document >+ * request (e.g., an inline image request). >+ * >+ * STATE_IS_DOCUMENT >+ * This flag indicates that the state transition is for a document request, >+ * which includes documents in sub-frames. These descriptions make it sound like these two bits should be mutually exclusive, yet all but one of the places in nsDocLoader.cpp that pass STATE_IS_DOCUMENT also pass STATE_IS_REQUEST. >+ * STATE_IS_NETWORK >+ * This flag indicates that the state transition corresponds to a network >+ * level event. A nsIWebProgressListener attached to a nsIWebProgress will >+ * not see this flag for onStateChange events corresponding to a >+ * subdocument. Given this description, is it a bug that the very first bit of nsDocLoaderImpl::FireOnStateChange checks |mIsLoadingDocument|?

> I've had to deal with this set of states a few times, and I've never really > understood them. Yes, these flags in particular are poorly defined. > These descriptions make it sound like these two bits should be mutually > exclusive, yet all but one of the places in nsDocLoader.cpp that pass > STATE_IS_DOCUMENT also pass STATE_IS_REQUEST. You are correct. The documentation is flat out wrong. STATE_IS_REQUEST appears to indicate a state transistion corresponding to a nsIRequest instance, and each STATE_IS_DOCUMENT should (it would seem) be OR'd with STATE_IS_REQUEST. I don't understand why that is not the case for doStopDocumentLoad. > Given this description, is it a bug that the very first bit of > nsDocLoaderImpl::FireOnStateChange checks |mIsLoadingDocument|? No, I don't think so because it would appear that we only pass STATE_IS_NETWORK to FireOnStateChange from doStartDocumentLoad and doStopDocumentLoad, and in those cases mIsLoadingDocument should be true. Oh, wait.. hmm.. doStopDocumentLoad is called from DocLoaderIsEmpty, which sets mIsLoadingDocument to false before it calls doStopDocumentLoad. But, FireOnStateChange calls mParent->FireOnStateChange, which means that the parent may still have mIsLoadingDocument set to true. So, that is consistent with the comment I added to the IDL. Of course, DocLoaderIsEmpty calls mParent->DocLoaderIsEmpty, so it would seem that later on there might be OnStateChange events with STATE_IS_NETWORK for the parent webprogress instances. This code is such a mess :-( I'm not very happy with the current implementation. I'm not really sure why anyone would really care about STATE_IS_NETWORK and STATE_IS_WINDOW. Indeed, LXR shows that nothing in our codebase looks at STATE_IS_WINDOW. We could just remove it, or keep it and mark it as unused, like I did for STATE_NEGOTIATING (to avoid triggering exceptions for embedders or extension authors). STATE_IS_NETWORK seems to be used in a lot of places, but I'm not sure that the information it conveys could not be extracted in some other way. It seems to mean something to browser.js. Hmm...

I think there may be some significance to the fact that we have two OnStateChange calls inside doStopDocumentLoad, and it probably has to do with wanting to "complete" stuff in two phases: first stuff triggered off of STATE_IS_DOCUMENT and second stuff triggered off of STATE_IS_NETWORK. Perhaps STATE_IS_NETWORK, which only appears with STATE_START or STATE_STOP, is (or was) in some way intended to denote the starting and stopping of overall network activity. I don't think it denotes that today since we see STATE_IS_NETWORK sent for each subdocument start and stop.

This JS component hooks itself up to the toplevel webprogress and records all events. It's interesting to note when STATE_IS_NETWORK appears. Apparently, one can use it to determine when a frameset completes loading because STATE_STOP | STATE_IS_NETWORK only appears once all of the frames have finished loading. I think this is by design, and I should probably document it.

OK, STATE_IS_WINDOW appears to exist so that we have a STATE_IS_ flag when STATE_IS_NETWORK is stripped. Basically, whenever a document completes, we generate the following three calls to onStateChange: 1) STATE_STOP | STATE_IS_REQUEST 2) STATE_STOP | STATE_IS_DOCUMENT 3) STATE_STOP | STATE_IS_NETWORK | STATE_IS_WINDOW <or> STATE_STOP | STATE_IS_WINDOW For toplevel documents, #2 and #3 are delayed until all of the subdocuments have completed. STATE_IS_NETWORK is sent to a listener's onStateChange only when the webprogress instance, on which the listener is attached, reaches #3. In that way, a listener can attach itself to a webprogress object and expect to see STATE_STOP | STATE_IS_NETWORK when everything in that webprogress completes. STATE_IS_WINDOW appears to exist so that we have a STATE_IS_ bit to pass to onStateChange when STATE_IS_NETWORK is not set! I think it roughly indicates that the webprogress is completing but that it is not the one you (the listener) attached yourself to. I'd like to either remove STATE_IS_WINDOW, and not send the corresponding onStateChange event, or make sure that STATE_IS_WINDOW has a corresponding STATE_START event. Since the listener can choose to not receive STATE_IS_WINDOW events, I think I might prefer to keep the STATE_IS_WINDOW state and go ahead and add a corresponding STATE_START for STATE_IS_WINDOW. Does that make sense to everyone?

I can't even make a meaningful estimate of when I'll get to this review... I have some long reviews in my queue already, and term has started, so the time I have to spend on Mozilla is rapidly approaching 0.

Comment on attachment 160565 [details] [diff] [review] patch to make STATE_IS_WINDOW more meaningful OK, bz... thanks for the heads-up... I'll pick on dbaron since he expressed "interest" ;-)

Here's what the output of webprogresslistener.js looks like with the last patch applied to the tree. I'm thinking that it might be nice to include something like this in nsIWebProgressListener.idl

> Here's what the output of webprogresslistener.js looks like... I should have mentioned what the HTML files look like in this case: test.html: <html><body> <iframe src="test_frame.html"></iframe> </body></html> test_frame.html: <html><body> hello </body></html>

we really need to figure out how these changes affect us. please don't flash freeze, we're in crunch time and don't have time for anything :(

> we really need to figure out how these changes affect us. please don't flash > freeze, we're in crunch time and don't have time for anything :( timeless: no where in the tree was STATE_IS_WINDOW being referenced (except in logging code). adding STATE_IS_WINDOW to doStartDocumentLoad only makes sense to balance the STATE_IS_WINDOW in doStopDocumentLoad. grep your tree... i'd be surprised if you were using STATE_IS_WINDOW.

Comment on attachment 160565 [details] [diff] [review] patch to make STATE_IS_WINDOW more meaningful ok, this patch looks good; it's good that STATE_IS_WINDOW now has both a START and a STOP event. what I'm wondering, though... why does doStopDocumentLoad send two notifications, rather than one with all the flags...?

> what I'm wondering, though... why does doStopDocumentLoad send two > notifications, rather than one with all the flags...? I'm not sure, but that's not something I'd venture to change. I suspect the ordering is critical for some reason. NOTE: The interface doesn't promise that the final notification will be split like this. Maybe it should because someone may depend on it being that way.

ok, the "patch to make STATE_IS_WINDOW more meaningful" has been checked in on the trunk.

> 106 * STATE_IS_NETWORK > 107 * This flag indicates that the state transition corresponds to a toplevel > 108 * document, and is accompanied by either STATE_START or STATE_STOP. is wrong. toplevel document would be the root frame. but if i click in a subframe, then something still sends is_network. > 132 * STATE_IS_BROKEN > 133 * This flag indicates an unknown security state. This may mean that the > 134 * request is being loaded as part of a page in which some content was > 135 * received over an insecure channel. still absolutely sucks > 146 const unsigned long STATE_SECURE_HIGH = 0x00040000; > 147 const unsigned long STATE_SECURE_MED = 0x00010000; > 148 const unsigned long STATE_SECURE_LOW = 0x00020000; is still not documented > 187 * @param aCurTotalProgress > 188 * The current progress for all the requests being monitored. is meaningless. what keeps track of 'all the requests being monitored'? > 193 * NOTE: For download sizes that exceed the maximum value representable by > 194 * type long, -1 will be used. clarify that you mean signed long. > 250 * NOTE: These notification will only occur if a security package is > 251 * installed. notifications -- if some of these flags can grow in the future, this should be indicated (somehow).

> > 106 * STATE_IS_NETWORK > > 107 * This flag indicates that the state transition corresponds to a toplevel > > 108 * document, and is accompanied by either STATE_START or STATE_STOP. > > is wrong. > > toplevel document would be the root frame. but if i click in a subframe, then > something still sends is_network. toplevel in this context means the frame in which content is being loaded, where that frame may itself contain other frames. nothing precludes that toplevel frame from being itself part of another frameset. the point of this flag is to mark the start and stop of an entire load, including all subframe loads. if you are only navigating within a subframe, and if only that subframe loads content, then that subframe is considered toplevel for the duration of that load. perhaps this needs to be spelled out more clearly. > > 132 * STATE_IS_BROKEN ... > still absolutely sucks ok, care to explain why you feel that way? > > 146 const unsigned long STATE_SECURE_HIGH = 0x00040000; > > 147 const unsigned long STATE_SECURE_MED = 0x00010000; > > 148 const unsigned long STATE_SECURE_LOW = 0x00020000; > > is still not documented These flags are documented in the comment for STATE_IS_SECURE. I did not feel the need to add redundant documentation for these "sub"-flags. > > 187 * @param aCurTotalProgress > > 188 * The current progress for all the requests being monitored. > > is meaningless. what keeps track of 'all the requests being monitored'? > > > 193 * NOTE: For download sizes that exceed the maximum value representable by > > 194 * type long, -1 will be used. > > clarify that you mean signed long. Why do we need to spell out the fact that |long| means signed? I think we should assume some basic knowledge of XPIDL types when people are reading our .idl files. > > 250 * NOTE: These notification will only occur if a security package is > > 251 * installed. > > notifications Thanks > if some of these flags can grow in the future, this should be indicated (somehow). Indeed. "Any unspecified bits are reserved for future use."

I revised the comments in the IDL files slightly to address some of timeless's concerns. http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/uriloader/base&command=DIFF_FRAMESET&file=nsIWebProgress.idl&rev2=1.12&rev1=1.11 http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/uriloader/base&command=DIFF_FRAMESET&file=nsIWebProgressListener.idl&rev2=1.14&rev1=1.13

nsIWebProgress.idl: darin%meer.net: + * The nsIWebProgress interface is used to look at the progress of documents darin%meer.net: + * loading in a particular DOM window and any descendent DOM windows. hm... that does not seem entirely true. what if I get the nsIWebProgress service and register my listener there? then it catches all progress everywhere, not just in a particular window... darin%meer.net: + * These flags indicate the state transistions to observe, corresponding to darin%meer.net: + * nsIWebProgressListener::onStateChange. (not new) "transitions" I note you didn't address timeless's toplevel comment... can that be considered "obvious"?

> nsIWebProgress.idl: > darin%meer.net: + * The nsIWebProgress interface is used to look at the > progress of documents > darin%meer.net: + * loading in a particular DOM window and any descendent DOM > windows. > > hm... that does not seem entirely true. what if I get the nsIWebProgress service > and register my listener there? then it catches all progress everywhere, not > just in a particular window... ok, sure that is true. that is an exception though, and it should probably be documented separately. most users of this interface are either attaching themselves to a <browser> instance or hooking themselves up to the root webprogress. > darin%meer.net: + * These flags indicate the state transistions to observe, > corresponding to > darin%meer.net: + * nsIWebProgressListener::onStateChange. > > (not new) "transitions" I don't understand this comment. > I note you didn't address timeless's toplevel comment... can that be considered > "obvious"? My change to nsIWebProgress.idl was meant to address his toplevel comment (i.e., that when you attach your WPL to a WP instance you are asking to receive events for that WP and any child WP instances, corresponding to child DOM windows). Perhaps I should spell that out more clearly.

> darin%meer.net: + * These flags indicate the state transistions to observe, I meant that "transistions" is misspelled. > My change to nsIWebProgress.idl was meant to address his toplevel comment ah, hm... but http://lxr.mozilla.org/seamonkey/source/uriloader/base/nsIWebProgressListener.idl#106 still talks about "toplevel"...

If anyone has additional thoughts or comments on these interfaces, please let them be known ASAP. Thanks!

> toplevel in this context means the frame in which content is being loaded, where > that frame may itself contain other frames. nothing precludes that toplevel > frame from being itself part of another frameset. the point of this flag is to > mark the start and stop of an entire load, including all subframe loads. if you > are only navigating within a subframe, and if only that subframe loads content, > then that subframe is considered toplevel for the duration of that load. > perhaps this needs to be spelled out more clearly. It does. > 137 * STATE_IS_SECURE > 138 * This flag indicates that the data corresponding to the request was > 139 * received over a secure channel. The degree of security is expressed by > 140 * STATE_SECURE_HIGH, STATE_SECURE_MED, or STATE_SECURE_LOW. This is not documentation. I have no clue what it means and short of getting the wrong impression from the code, which is the original problem i ran into, this is not the way to do business. > > 187 * @param aCurTotalProgress > > 188 * The current progress for all the requests being monitored. > > is meaningless. what keeps track of 'all the requests being monitored'? you still haven't addressed this. -- general idl comment. if you comment the individual constants instead of the group, things like unstable.elemental.com/mozilla will be able to pull out documentation for the individual items.

> > toplevel in this context means the frame in which content is being loaded, I presume this was meant as a contribution to the documentation for nsIWebProgress.idl?? If so, then thanks. > This is not documentation. I have no clue what it means and short of getting It looks like documentation to me. How is the meaning not clear? > > is meaningless. what keeps track of 'all the requests being monitored'? > > you still haven't addressed this. The web progress object, which may correspond to a frameset or document, encompasses the loading of many requests. I guess you feel that that is somehow not clear. > general idl comment. if you comment the individual constants instead of the > group, things like unstable.elemental.com/mozilla will be able to pull out > documentation for the individual items. I looked at unstable, and I'm not unhappy with the current results there: http://unstable.elemental.com/mozilla/build/latest/mozilla/uriloader/dox/interfacensIWebProgressListener.html The fact that it groups the constants, and then provides documentation for that group is apparent, and IMO a good thing for clarifying that the grouping of related flags.

> I presume this was meant as a contribution to the documentation for nsIWebProgress.idl you wrote it. just so long as you stick something in there. > It looks like documentation to me. How is the meaning not clear? what is medium?

> you wrote it. just so long as you stick something in there. doh! brain-fart > > It looks like documentation to me. How is the meaning not clear? > > what is medium? something between high and low? seriously, i think it's intentionally meant to be a little bit vague. PSM or NSS is making a determination about the level of encryption, and it is reflected in this value. one might use the flag to color code an encryption indicator perhaps. to understand what it means precisely would require additional information from some other interface. so, is STATE_SECURE_MED the only thing that bothers you?

all three bother me. yes they were clearly meant to be intentionally vague. i think that when you're freezing an interface you should specify what they mean. the next round will have to use a new interface or new constants, so it's ok to freeze the meanings of all three.

> all three bother me. yes they were clearly meant to be intentionally vague. i > think that when you're freezing an interface you should specify what they > mean. I'm not sure I agree. I think it is appropriate and fine to have flags that offer a vague determination of security level, and then I think we should point consumers at the place to go to get more detailed security info. This way, applications do not have to decide for themselves, by inspecting the detailed security info, that the security level is low, medium, or high (which are states that an application may wish to expose in some GUI). > the next round will have to use a new interface or new constants, so it's ok > to freeze the meanings of all three. There only needs to be a next round if something is wrong with the interface. So far, the only thing that is really wrong with this interface is the fact that it cannot represent progress for 64-bit stream lengths. That doesn't necessarily mean an entirely new interface. It may only mean that we add a subclass of nsIWebProgressListener that has an onProgressChange64 method or something like that. At least, there are options there. What I see us doing in the future is offering better low-level interfaces to inspect the security state that is being reported here. Those interfaces can be in addition to nsIWebProgressListener.idl as it is today. (See nsIChannel::securityInfo, and nsITransportSecurityInfo, but that latter interface needs work if it is to be frozen.)

ok, revised patch with comment additions per feedback from timeless.

Comment on attachment 162628 [details] [diff] [review] mark interfaces frozen, move into SDK, tweak a few comments >Index: nsIWebProgressListener.idl Some things that need additional documentation or consideration, imo: 1) Will there be any progress notifications for requests that somehow fail (eg 404 replies, host not found, etc). If so, what kind? 2) (related) At what point will STATE_START fire? When we make the request? When we get the server response? 3) Which request is passed to STATE_REDIRECTING notifications? Will the STATE_STOP for the old request happen after the STATE_REDIRECTING? That's what the comment seems to say, but it may be worth being more explicit about event ordering here. 4) If STATE_NEGOTIATING is not used, couldn't we remove it? Without changing anything else in the API? 5) Is there ever a case when STATE_IS_REQUEST is not set? If so, when? If not, could we indicate that? 6) What exactly is the difference between a STATE_IS_DOCUMENT and a STATE_IS_WINDOW? That's not clear to me from reading this api. 7) What is "the document being loaded" in the onStateChange comments? 8) In the comments on onProgressChange, it's not clear what value will have -1 used if things don't fit in type long... 9) onLocationChange documentation talks about watching a window. Aren't we actually watching an nsIWebProgress instance? This goes back to the earlier confusion about windows vs documents. Is this interface usable with documents that have no window associated to them? (XBL, XSLT, SVG, random XML loaded via our XML-loading apis, etc all come to mind.) 10) Will onSecurityChange always have an nsIRequest? Do we make any promises on what that request will be? Could it be a request for an image inside a document that's loading, say? 11) Should this interface say anything about how exceptions thrown from the various methods will be handled? Or even whether those methods are allowed to throw exceptions? Maybe not necessary, I guess.... Comments on the patch itself: > * Notification that the progress has changed for one of the requests being >- * monitored. >+ * monitored by aWebProgress. A nsIWebProgress instance generally Maybe "aWebProgress generally corresponds to a document"? The documentation for onSecurityChange should say "Security State Flags and Security Strength Flags" where it now says "Security State Flags". >Index: nsIWebProgress.idl This interface makes it clearer that all this only works if we have a DOM window about... The actual implementation of AddProgressListener (in docloader) will throw NS_ERROR_FAILURE in some out-of-memory conditions. That should be fixed... The interface should probably document that NS_ERROR_OUT_OF_MEMORY can get thrown by this method. Accessing the DOMWindow property can throw. This should be documented (or better yet, fixed? But the root docloader is an nsIWebProgress... then again, nothing in this interface mentions the root docloader; people using it are just asking for trouble imo). Does isLoadingDocument correspond to just the document for DOMWindow, or any subdocument too? Overall, I think these interfaces are in pretty good shape... Marking sr- pending answers to the issues raised above, though. Next time I can do this much faster, now that I know what things are like here. I do believe this is the first time I've read these IDL files... ;)

> 1) Will there be any progress notifications for requests that somehow > fail (eg 404 replies, host not found, etc). If so, what kind? STATE_START | START_IS_REQUEST STATE_STOP | STATE_IS_REQUEST w/ aStatus set to a failure code > 2) (related) At what point will STATE_START fire? When we make the > request? When we get the server response? STATE_START fires for a request when the request is initiated. > 3) Which request is passed to STATE_REDIRECTING notifications? Will the request that is being redirected. > the STATE_STOP for the old request happen after the > STATE_REDIRECTING? yes. note: there isn't really a way with this API to link the old request to the new request. instead, listener's just see that a request is being redirected... and they expect to see another request start after that. > 4) If STATE_NEGOTIATING is not used, couldn't we remove it? Without > changing anything else in the API? i didn't want to remove it for two reasons: - some JS component may be referencing the constant, and removing it could cause unexpected exceptions. - if we ever define new bits for the state flags, i don't want us to reuse this bit, so keeping it in the API helps avoid that. > 5) Is there ever a case when STATE_IS_REQUEST is not set? If so, > when? If not, could we indicate that? yes, STATE_STOP | STATE_IS_WINDOW happens. > 6) What exactly is the difference between a STATE_IS_DOCUMENT and a > STATE_IS_WINDOW? That's not clear to me from reading this api. as discussed on irc, the difference is subtle. i think STATE_IS_WINDOW exists purely so that we have an object to speak of when we send the second STATE_STOP event corresponding to document completion. that's a very difficult thing to clearly explain. and i'm not sure exactly why the two events even exist. > 7) What is "the document being loaded" in the onStateChange comments? the webprogress... i think i need to replace all mentions of frame, document, and window with "webprogress" ... and make it clearer somewhere what a webprogress instance represents. > 8) In the comments on onProgressChange, it's not clear what value > will have -1 used if things don't fit in type long... how is the comment not clear? it says: "For download sizes that exceed the maximum value representable by type long, -1 will be used." ... what else could "download sizes" mean other than the parameters that indicate size in the function call? > 9) onLocationChange documentation talks about watching a window. > Aren't we actually watching an nsIWebProgress instance? This goes > back to the earlier confusion about windows vs documents. Is this > interface usable with documents that have no window associated to > them? (XBL, XSLT, SVG, random XML loaded via our XML-loading > apis, etc all come to mind.) apparently, yes... nsIWebBrowserPersist uses this interface. but, it passes null for the webprogress parameter, so maybe that's a bad example. clearly, the root docloader (nsIWebProgress instance) does not have a nsIDOMWindow associated with it. so, i think we should simply talk of webprogress instances in the comments for WPL, and then in the comments for WP, we can talk about the possible relation to things like documents and windows. > 10) Will onSecurityChange always have an nsIRequest? Do we make any > promises on what that request will be? Could it be a request for > an image inside a document that's loading, say? i don't think we make any promises about what any of the requests will be. they can even be null in some cases. i suppose it is probably the request that caused the security state to change. > 11) Should this interface say anything about how exceptions thrown > from the various methods will be handled? Or even whether those > methods are allowed to throw exceptions? Maybe not necessary, I > guess.... I guess we can just say that exceptions are ignored. > Maybe "aWebProgress generally corresponds to a document"? perhaps.. but now i prefer to not mention document and window in nsIWPL. > The documentation for onSecurityChange should say "Security State Flags and > Security Strength Flags" where it now says "Security State Flags". ok, thanks. ("Security Strength Flags" was a recent addition to the documentation.) > The actual implementation of AddProgressListener (in docloader) will throw > NS_ERROR_FAILURE in some out-of-memory conditions. That should be fixed... > The interface should probably document that NS_ERROR_OUT_OF_MEMORY can get > thrown by this method. any xpcom method may throw NS_ERROR_OUT_OF_MEMORY as well as a host of other generic exceptions. consider language bindings, and distributed xpcom, etc. the only exceptions worth mentioning are the ones that are not so generic. i don't think we should document NS_ERROR_OUT_OF_MEMORY on every xpcom method call... that would be overly verbose. > Accessing the DOMWindow property can throw. This should be documented (or > better yet, fixed? can't be fixed. should be documented. > But the root docloader is an nsIWebProgress... then again, > nothing in this interface mentions the root docloader; people using it are > just asking for trouble imo). we need to make the root docloader something that is frozen. people use it to attach global WPL instances, and we need to / should support that. > Does isLoadingDocument correspond to just the document for DOMWindow, or any > subdocument too? i think it just refers to the current webprogress instance, but i'll verify and document it. thanks bz for the thoughtful review :)

> nsIWebBrowserPersist uses this interface so does nsExternalAppHandler, btw... (i.e. the nsIDownload implementation is expected to implement nsIWPL)

> yes, STATE_STOP | STATE_IS_WINDOW happens. And the aRequest is null, in that case, I'm guessing? Document that, along with the other answers to my questions, please. > how is the comment not clear? It's not clear, for example, whether one of the values being out of range will force all the others to be -1 as well. That is, if we know that the max progress overflows a 32-bit int, will we always have -1 for the current progress, or will we have a real number for a while and then -1 until it's done? I suspect we do the latter, though the former is possibly less confusing for callers to deal with...

> And the aRequest is null, in that case, I'm guessing? Document that, along > with the other answers to my questions, please. no, it is not null in that case. it is in fact the same request that was passed to onStateChange(STATE_STOP|STATE_IS_DOCUMENT). i'm not sure that that makes much sense. i plan on documenting all of the stuff we've just discussed. ok, i see now why you thought the -1 stuff was unclear. thanks for clarifying.

Comment on attachment 163911 [details] [diff] [review] revised patch per bz's review comments + * @throw NS_ERROR_FAILURE + * Indicates that there is no associated DOM window. so, this will never be null when the method returns successfully?

it looks like in some very unexpected situations that it could end up returning NS_ERROR_NO_INTERFACE. but, i don't think it will ever return NS_OK and a NULL valued DOM window.

Comment on attachment 163911 [details] [diff] [review] revised patch per bz's review comments >Index: nsIWebProgressListener.idl >+ * For any given request, onStateChange is called once with the STATE_START >+ * flag, zero or more times with the STATE_TRANSFERRING flag or once with the >+ * STATE_REDIRECTING flag, and then finally once with the STATE_STOP flag. Except when it's called twice with the STATE_STOP flag... say "once or twice" here and reference the STATE_IS_WINDOW documentation? > * STATE_IS_NETWORK >+ this flag is only set when onStateChange's aWebProgress >+ * is the nsIWebProgress to which the nsIWebProgressListener was added or >+ * if the nsIWebProgress to which the nsIWebProgressListener was added is >+ * not busy loading any requests. I'm not sure what this last part means.... Perhaps say that this is set if this is the first STATE_START or last STATE_STOP associated with the nsIWebProgress instance being observed? Is that even true? In any case, this part needs a bit more work... Other than that, this looks much better!

> Except when it's called twice with the STATE_STOP flag... say "once or twice" > here and reference the STATE_IS_WINDOW documentation? well, it is once with STATE_IS_REQUEST... then again with STATE_IS_WINDOW if the request is a document request. yeah, i'll try to find a way to document that. > > * STATE_IS_NETWORK ... > I'm not sure what this last part means.... Perhaps say that this is set if > this is the first STATE_START or last STATE_STOP associated with the > nsIWebProgress instance being observed? Is that even true? In any case, this > part needs a bit more work... yeah, this is the most complicated thing to document. basically, if you are attached as a listener to a webprogress, you'll get STATE_IS_NETWORK|STATE_START to tell you when activity is beginning in your webprogress or possibly just in a webprogress corresponding to a subframe. in other words, your own webprogress may not be loading a document, but one of it's child webprogress instances may be loading a new document. in that case you'd see STATE_IS_NETWORK start and stop to mark the start and stop of that child webprogress. so, that's what i was trying to describe, but yeah... i should just be more verbose or something.

Yeah, I think just explaining it at length, if needed, is the way to go.

Comment on attachment 164034 [details] [diff] [review] revised patch per recent comments sr=bzbarsky

fixed-on-trunk

should you announce this on mozilla-api-announce? http://www.mozilla.org/community/developer-forums.html#mozilla-api-announce

So I just ran into something interesting... the two interfaces have methods with identical names, so implementing both on the same object, in JS say, can be pretty nontrivial...

Er, nevermind. Actually, what I ran into is nsIWebProgressListener colliding with nsISecurityEventSink....

created by diffing MOZILLA_1_7_BRANCH with HEAD for the three files and removing the REQUIRES change that happened.

Comment on attachment 166894 [details] [diff] [review] complete diff this is a collection of all of the above patches. it consists only of comment changes and has therefore no risk. in order to make the gecko sdk for the next branch release more useful, it would be nice to have this on the branch.

Comment on attachment 166894 [details] [diff] [review] complete diff a=mkaply

fixed on the 1.7 branch.