99639 - Freeze nsIWebProgress and nsIWebProgressListener

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Chak Nanga]

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.

Comment 1

[Chak Nanga]

->0.9.6

Comment 2

[rpotts (gone)]

-> 0.9.7

Comment 3

[Boris Zbarsky [:bzbarsky]]

See bug 99642 comment 13 

Comment 4

[David Epstein]

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

Comment 5

[Darin Fisher]

mass reassign of various rpotts bugs to me

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 7

[Aaron Leventhal]

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

Comment 8

[Darin Fisher]

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.

Comment 9

[Darin Fisher]

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

Comment 10

[Darin Fisher]

Attachment: v1 patch

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.

Comment 11

[timeless]

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

Comment 12

[timeless]

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

Comment 13

[Darin Fisher]

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

uhm... care to elaborate? :-/

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 15

[Darin Fisher]

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?

Comment 16

[timeless]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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 18

[Christian :Biesinger (don't email me, ping me on IRC)]

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 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 157221 [details] [diff] [review]
v1 patch

darin is working on a new patch

Comment 20

[Darin Fisher]

Attachment: v1.1 patch

Revised patch, including much improved documentation.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

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!

Comment 22

[timeless]

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

Comment 23

[Darin Fisher]

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

Comment 24

[Darin Fisher]

Attachment: v1.2 patch

Comment 25

[Darin Fisher]

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 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 27

[Darin Fisher]

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

Comment 28

[Darin Fisher]

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.

Comment 29

[Darin Fisher]

Attachment: webprogresslistener.js - test component

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.

Comment 30

[Darin Fisher]

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?

Comment 31

[Darin Fisher]

Attachment: patch to make STATE_IS_WINDOW more meaningful

Comment 32

[Boris Zbarsky [:bzbarsky]]

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 33

[Darin Fisher]

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" ;-)

Comment 34

[Darin Fisher]

Attachment: output of webprogresslistener.js with layout dummy requests stripped

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

Comment 35

[Darin Fisher]

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

Comment 36

[timeless]

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

Comment 37

[Darin Fisher]

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

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 39

[Darin Fisher]

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

Comment 40

[Darin Fisher]

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

Comment 41

[timeless]

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

Comment 42

[Darin Fisher]

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

Comment 43

[Darin Fisher]

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

Comment 44

[Darin Fisher]

Attachment: mark interfaces frozen, move into SDK

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

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"?

Comment 46

[Darin Fisher]

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

Comment 47

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 48

[Darin Fisher]

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

Comment 49

[timeless]

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

Comment 50

[Darin Fisher]

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

Comment 51

[timeless]

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

Comment 52

[Darin Fisher]

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

Comment 53

[timeless]

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.

Comment 54

[Darin Fisher]

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

Comment 55

[Darin Fisher]

Attachment: mark interfaces frozen, move into SDK, tweak a few comments

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

Comment 56

[Boris Zbarsky [:bzbarsky]]

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

Comment 57

[Darin Fisher]

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

Comment 58

[Christian :Biesinger (don't email me, ping me on IRC)]

> nsIWebBrowserPersist uses this interface

so does nsExternalAppHandler, btw... (i.e. the nsIDownload implementation is
expected to implement nsIWPL)

Comment 59

[Boris Zbarsky [:bzbarsky]]

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

Comment 60

[Darin Fisher]

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

[Darin Fisher]

Attachment: revised patch per bz's review comments

Comment 62

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 63

[Darin Fisher]

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 64

[Boris Zbarsky [:bzbarsky]]

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!

Comment 65

[Darin Fisher]

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

Comment 66

[Boris Zbarsky [:bzbarsky]]

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

Comment 67

[Darin Fisher]

Attachment: revised patch per recent comments

Comment 68

[Boris Zbarsky [:bzbarsky]]

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

sr=bzbarsky

Comment 69

[Darin Fisher]

fixed-on-trunk

Comment 70

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 71

[Boris Zbarsky [:bzbarsky]]

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

Comment 72

[Boris Zbarsky [:bzbarsky]]

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

Comment 73

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: complete diff

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

Comment 74

[Christian :Biesinger (don't email me, ping me on IRC)]

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 75

[Mike Kaply [:mkaply]]

Comment on attachment 166894 [details] [diff] [review]
complete diff

a=mkaply

Comment 76

[Christian :Biesinger (don't email me, ping me on IRC)]

fixed on the 1.7 branch.