253550 - Frames in pages should send progress notifications when loaded as part of a page

Status: VERIFIED INVALID

(Core :: DOM: Navigation, defect)

Description

[timeless]

this is needed for our embedding app.

The current behavior doesn't make much sense and is really inflexible, it
filters out useful events instead of letting the app logic decide what's important.

The state of the world is something like this:

step 1, load:
data:text/html,<frameset cols="*,*"><frame name=left src="data:text/html,&lt;a
target=right href=about:&gt;click me"><frame name=right src="about:mozilla">

you get progress notifications for the frameset, but not for the frames.

step 2, click 'click me'

you get progress notifications for the right frame.

this is problematic, especially since certain top level pages have a tendency to
never finish loading. we need to know which things have started to load and
which have finished, not just the page that the user asked the browser to load
(we generally already know which page that is...).

Comment 1

[timeless]

Attachment: send notifications for non top level loads, remove is_document/is_state_network as appropriate

this patch was written by another developer @cenzic

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 154661 [details] [diff] [review]
send notifications for non top level loads, remove is_document/is_state_network as appropriate

I'm not doing srs in code I don't know, remember?

Comment 3

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

Comment on attachment 154661 [details] [diff] [review]
send notifications for non top level loads, remove is_document/is_state_network as appropriate

+  else if (!mIsLoadingDocument && !bJustStartedLoading) {

Since this function does above:
      bJustStartedLoading = PR_TRUE;
      mIsLoadingDocument = PR_TRUE;
which is the only time bJustStarted... is set to TRUE, the following is valid:
  bJust => mIsLoading
that means:
  !bJust <= !mIsLoading

So, if !mIsLoading is true, !bJust is always true. Which means you can get rid
of the second part of the if.
(gotta love logic classes...)

Comment 4

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

Comment on attachment 154661 [details] [diff] [review]
send notifications for non top level loads, remove is_document/is_state_network as appropriate

-  // The rule is to remove this bit, if the notification has been passed
-  // up from a child WebProgress, and the current WebProgress is already
-  // active...
+  // The rule is to remove this bit, if the request is not a channel,
+  // and hence not really a network request.
   //
   if (mIsLoadingDocument &&
       (aStateFlags & nsIWebProgressListener::STATE_IS_NETWORK) && 
       (this != aProgress)) {
+    nsCOMPtr<nsIChannel> channel(do_QueryInterface(aRequest));
+    if (!channel) {
     aStateFlags &= ~nsIWebProgressListener::STATE_IS_NETWORK;

so you removed the old comment, but the code it described is still there. why?

Comment 5

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

Comment on attachment 154661 [details] [diff] [review]
send notifications for non top level loads, remove is_document/is_state_network as appropriate

ok, description of what this patch does, which took me some time to figure out:

First hunk:
- fire notifications even if !mLoadingDocument, because otherwise no
notifications for a redirect in an iframe are sent
- pass request to doStartDocumentLoad, to support sending start notifications
for non-document requests

hunk 2:
- use passed-in request for notification rather than mDocumentRequest. Only
send STATE_IS_DOCUMENT for the toplevel load, i.e. not for subframes.

hunk 3:
this is sorta unrelated, and basically just filters out STATE_IS_NETWORK for
requests that are no channels (dummy requests from layout/parser)

Comment 6

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

OK. Before I review either this patch or bug 99639, I want it made clear what
STATE_IS_DOCUMENT means. Bonus points if you explain STATE_IS_NETWORK too.

because this patch uses an (imo) unexpected meaning of _DOCUMENT, namely that
it's set only for the toplevel load. the upside is, maybe, that this is more
compatible w/ existing code because it makes subdocuments look like say <img> or
<script>. but that makes it hard to tell whether the current thingy is an own
document or not.

Comment 7

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

Comment on attachment 154661 [details] [diff] [review]
send notifications for non top level loads, remove is_document/is_state_network as appropriate

r-, see comment 3 and comment 4.

I would love to see the STATE_IS_NETWORK change separated in a different patch.
bug 242267 may be a good place for it (if it fixes that bug, at least)

same for the sending of stuff even if !mIsLoadingDocument.

r=me on the rest, provided darin agrees, and provided that darin's
documentation agrees ;). but please attach a new patch for that.

Comment 8

[Darin Fisher]

(In reply to comment #6)
> OK. Before I review either this patch or bug 99639, I want it made clear what
> STATE_IS_DOCUMENT means. Bonus points if you explain STATE_IS_NETWORK too.

see the newly added documentation for nsIWebProgressListener.idl. 
STATE_IS_NETWORK | STATE_START means the start of a frameset load, and
STATE_IS_NETWORK | STATE_STOP means the end of a frameset load.


> because this patch uses an (imo) unexpected meaning of _DOCUMENT, namely that
> it's set only for the toplevel load.

STATE_IS_DOCUMENT must be set for each frame.


> the upside is, maybe, that this is more
> compatible w/ existing code because it makes subdocuments look like say <img> 
> or <script>.

i don't think we want to do this.  it breaks compatibility.

Comment 9

[Darin Fisher]

So, I am not able to reproduce this bug.  I'll attach a testcase in which I have
a simple XUL file attach a nsIWebProgressListener instance to a <browser> via
addProgressListener.

onStateChange is called as it should be for each of the frames as well as the
frameset.

Comment 10

[Darin Fisher]

Attachment: testcase

here's my testcase.  to try it out,

1) unpack the .tgz file

2) edit installed-chrome.txt and add this line:
  
   content,install,url,file:///path/to/xul_test/

3) then start the browser, and load chrome://test/content/test.xul

4) click on the "load test" button

Comment 11

[Darin Fisher]

this bug is INVALID

if you filter on STATE_IS_NETWORK then you will get the behavior observed in
comment #0.  that is by design.  if you filter on STATE_IS_DOCUMENT, then you
will observe onStateChange for each document that is loaded.

Comment 12

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

which also means that the change to send STATE_IS_NETWORK only for nsIChannel
requests is wrong.

vrfy. yes, this is a nonobvious meaning of STATE_IS_NETWORK - the current
documentation should help, though.