Last Comment Bug 342715 - Need an API to allow extensions to hook "document load started" events
: Need an API to allow extensions to hook "document load started" events
Status: RESOLVED FIXED
[firebug-p1]
:
Product: Core
Classification: Components
Component: Embedding: APIs (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
: Myk Melez [:myk] [@mykmelez]
Mentors:
: 424854 (view as bug list)
Depends on: 83265 549539
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-25 21:27 PDT by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2010-05-11 07:12 PDT (History)
29 users (show)
johnjbarton: wanted1.9.2?
dsicore: blocking1.9.1-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.29 KB, patch)
2006-06-25 21:30 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
cbiesinger: review+
Details | Diff | Splinter Review
updated patch (5.04 KB, patch)
2006-08-06 14:23 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
cbiesinger: review-
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (email my personal email if necessary) 2006-06-25 21:27:16 PDT
We (Novell) want to use Greasemonkey to munge a particular Web site to get it to work in Firefox. Our script needs to run before page script runs in <HEAD>. There seems to be currently no way to do this. nsIWebProgressListener::OnLocationChange fires at the right times, when it fires at all, but it is suppressed on subframe documents.

I've constructed a patch that fires nsIWebProgressListener::OnStateChange with STATE_TRANSFERRING|STATE_IS_NETWORK|STATE_IS_DOCUMENT just before nsDocShell::CreateContentViewer returns (right after it conditionally fires OnLocationChange). This works great for us. As far as I can tell it's compatible with the comments written in nsIWebProgress(Listener), but I don't know if it's really compatible with all users; it might not be. So I'm not 100% sure this is the right solution. If it isn't, I'd appreciate alternative suggestions.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-06-25 21:30:49 PDT
Created attachment 227032 [details] [diff] [review]
patch

the aforementioned patch
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2006-06-25 21:47:52 PDT
I suspect that an OnStateChange with a superset of whatever flags we currently pass is probably incompatible with existing users if it fires at a different time (or even if it fires when the users only expect to see one event with their flags).

The comments in nsIWebProgressListener unfortunately just document the behavior we currently have, not what the interface contract is.  :(  The latter is "whatever we do now".
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2006-06-25 21:48:31 PDT
Or to summarize, I suspect it's better to just introduce a new flag for this.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-06-26 02:36:54 PDT
Yeah, that's exactly what I was thinking.

But if I define a new STATE_ flag, I guess that would have the same problem, since users might not check their flags.

I could define a new STATE_ flag, say STATE_DOC_CREATED, and a new NOTIFY_STATE_DOC_CREATED flag that enables the new callbacks. Then the problem is, how does an extension know whether NOTIFY_STATE_DOC_CREATED will be honoured or not. I guess an extension would know that if STATE_DOC_CREATED didn't fire before, say, DOMContentLoaded on the document, then it wasn't ever going to fire and was not supported by this version of Gecko, and that's probably good enough. But maybe it's stretching the frozen-ness of the interface too far.

Perhaps the safest way to go is to create nsIWebProgress2, put the constants in there but no new methods, and if that interface is implemented then the methods support them?
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2006-06-26 09:25:22 PDT
> since users might not check their flags.

Then they're already screwed, no?  We fire OnStateChange a bunch of times for the same document, with different flags.

The nsIWebProgress2 thing may work; darin, what do you think?
Comment 6 Christian :Biesinger (don't email me, ping me on IRC) 2006-06-27 12:38:05 PDT
bug 83265 is kind of similar, in that it also adds a flag. although that one uses a function call rather than just a flag.

you are thinking of C++ extensions I suppose? what does the impl do with flags it doesn't recognize?
Comment 7 Christian :Biesinger (don't email me, ping me on IRC) 2006-06-27 12:48:03 PDT
but it's good that you're adding this, this is occasionally asked for in various newsgroups
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-06-27 14:48:08 PDT
(In reply to comment #6)
> you are thinking of C++ extensions I suppose?

You can use this from JS. Greasemonkey does.
Comment 9 Christian :Biesinger (don't email me, ping me on IRC) 2006-06-27 16:54:12 PDT
well, JS can find out trivially if this is supported (if ("YOUR_NEW_FLAG" in Components.interfaces.nsIWebProgress) { /* do whatever */ }) and in fact can not use the constant if the current gecko version does not have it.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-06-27 20:17:28 PDT
I see what you mean. The version problems still exist for existing C++ users.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-07-04 17:56:42 PDT
darin, can you comment here?
Comment 12 Darin Fisher 2006-07-13 15:58:04 PDT
Couple comments:

1) Not too worried about sending STATE_TRANSFERRING events earlier.  We already synthesize those in some cases.  Can they really be harmful?

2) Not too worried about defining new state flags since people who use == to test flags should be using one of the NOTIFY_STATE_xxx flags when they add their progress listener.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-07-13 16:13:27 PDT
(In reply to comment #12)
> Couple comments:
> 
> 1) Not too worried about sending STATE_TRANSFERRING events earlier.  We
> already synthesize those in some cases.  Can they really be harmful?

You mean you think we'd be OK with the patch in comment #1?
Comment 14 Darin Fisher 2006-07-13 19:43:55 PDT
Have you verified that this doesn't impact the lock icon state in any negative way?  The secure browser UI code watches for the first STATE_TRANSFERRING event to determine when the lock icon state should be updated.  Since your new event is occurring after OnLocationChange, that shouldn't be a concern, but somehow I'm not overly confident.
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-07-13 21:21:40 PDT
The lock appears to appear and disappear correctly.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2006-07-28 19:35:49 PDT
roc, I'm not going to get to this in a reasonable timeframe.  Maybe ask biesi?
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-07-28 20:32:38 PDT
Comment on attachment 227032 [details] [diff] [review]
patch

sure
Comment 18 Christian :Biesinger (don't email me, ping me on IRC) 2006-08-04 22:16:21 PDT
Comment on attachment 227032 [details] [diff] [review]
patch

seems ok, I think... but shouldn't this new flag combination be documented somewhere?
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-08-04 22:26:15 PDT
Sure. Where, in the comments in nsIWebProgressListener?
Comment 20 Christian :Biesinger (don't email me, ping me on IRC) 2006-08-05 16:23:08 PDT
that's probably the best place, yeah
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-08-06 14:23:54 PDT
Created attachment 232440 [details] [diff] [review]
updated patch

Updated comment.

Also, I changed the flags combination to STATE_TRANSFERRING|STATE_IS_REQUEST|STATE_IS_DOCUMENT. IS_NETWORK didn't seem appropriate, and IS_DOCUMENT needs IS_REQUEST as per the IDL comments.
Comment 22 Christian :Biesinger (don't email me, ping me on IRC) 2006-08-06 15:12:06 PDT
Comment on attachment 232440 [details] [diff] [review]
updated patch

surely this isn't working then? in the status filter, the else won't be reached for your new flag, because it does have IS_REQUEST set.
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-08-06 16:25:47 PDT
True.

Now, it turns out that nsDocLoader also fires onStateChanged with IS_REQUEST|IS_DOCUMENT, on its first OnProgress (which may be too late for me, since content could already have been added to the document by this time). So I think I need to add some new kind of flag.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-08-06 17:20:18 PDT
I think I'll just wait for bug 83265 to land, and build on that with a new function in nsIWebProgressListener2. I think adding new callback functions makes a lot more sense than messing around with state bits.
Comment 25 timeless 2006-11-26 19:18:14 PST
so. most people use 0xfff or whatever to add, because trying to use anything else is a huge waste of time since most people can't figure out anything that works better.

that said, using & is common, but that doesn't mean that one checks or asserts that all other bits are cleared. trying to assert all the other bit states for this stuff just leads past the madness that is usually entailed in trying to make something work with this stuff.
Comment 26 Nick Matsakis 2008-02-07 09:26:55 PST
I don't think the approach in attachment 232440 [details] [diff] [review] is the right way to address this problem.

The progressListener interface seems to be designed to keep UI informed of the various events that occur during page load, including redirects and security context changes.  Properly implementing the onStateChange() method requires checking a bunch of flags to ensure that the state change is the one you are interested in and in general it is not a friendly API to use. Greasemonkey's use of this API is a clever hack to get around the fact that Mozilla doesn't provide a clean way to do what it wants to do; extending NSIWebProgressListener2 doesn't make it any cleaner.

As an extension developer, I'd much prefer that the browser simply send a "DOMInitialized" event to interested parties.  This would be a complement to the existing Mozilla-specific "DOMContentLoaded" event and standard "load" event.  By definition it wouldn't be possible for a document's script to receive the DOMInitialized event, because it would have already fired before any code on the page could register as a listener for it.  Still, other components and chrome could use this event to Greasemonkeyish effect.
Comment 27 Mike Kaply [:mkaply] 2008-03-24 14:44:26 PDT
*** Bug 424854 has been marked as a duplicate of this bug. ***
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2008-07-30 16:06:17 PDT
This came up when talking to the firebug guys today.

Our initial thought when they described their requirements was to fire a DOM event from inside SetNewDocument() to the mChromeEventHandler with the following properties on the event:

1) The window in question
2) The old document
3) The new document
4) Whether we reused the inner window

We could dispatch an nsIWebProgressListener2 callback at the same time.  That's preferable to adding state bits, since it lets us pass in the those four items of information, and is somewhat preferable for embeddings, for which listening to the DOM event is a bit of a pain.
Comment 29 John J. Barton 2008-08-01 23:10:28 PDT
Just to add some context here, the old document/new document bit comes up in cases where one document drives another, eg XSLT (Bug 391177), and I think in other cases. As I understand it, old document would be either null (new browser tab) or "about:blank" if there was not a relation between the windows.
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2008-10-30 21:50:40 PDT
OK, this came up _again_ today.  jst mentioned that we already bind global objects from the global object category at the right point in our code, but setting those up involves C++ code at the moment.  So we need to send a notification at that point or change the initializer stuff to be usable from JS.

So I guess now the options are a way to set up global objects, a DOM event, or an nsIWebProgressListener2 notification.  I still think that doing the last two of these is the simple way to do this.  Doing the latter is nice because it would let someone easily watch all windows if desired (just register on the root docloader), but the event is easier if you want to watch a particular single window or some specific set of windows.
Comment 31 John J. Barton 2008-10-30 22:21:52 PDT
I like your plan in comment 28: both DOM event and progress notification.
Comment 32 Damon Sicore (:damons) 2008-11-12 21:50:51 PST
At this point, I don't see this blocking the release as we're adding an API.   blocking1.9.1-.  Renom if you disagree.
Comment 33 John J. Barton 2008-12-14 09:46:16 PST
Trying to get this on the 1.9.2 todo list.
Comment 34 John J. Barton 2009-10-10 08:50:21 PDT
Another reason to implement this: Firebug is now has to speculatively allocate objects in case network events will end up being used in pages the user wishes to debug. These events occur before onLocationChange, the place we currently decide if the page is debuggable. Managing these pre-load allocations is hacky. If this bug was implemented we should be able to move that test to this hook.
Comment 35 John J. Barton 2009-10-13 21:19:28 PDT
A related problem has been reported by user:
Issue 2379:  	 Adding unload handler in tabWatcher causes reload for pages with iframes.
http://code.google.com/p/fbug/issues/detail?id=2379

Firebug's unload handler changes the page semantics. This is the reverse problem: we want to unhook from the page because it is going away.
Comment 36 Boris Zbarsky [:bz] (still a bit busy) 2009-10-13 22:24:50 PDT
Why do you have an unload handler at all?  Let's take this up in the newsgroup; it's probably not the right conversation to be having in this bug.
Comment 37 Rob Campbell [:rc] (:robcee) 2009-10-28 04:10:18 PDT
Boris, John: what was the outcome from the discussion mentioned in c#36?

roc's original comment to have an additional state change notification should have been met with the landing of bug 83265. If it's possible to set an unload handler, that should accommodate C#35.

Is there work left to do for this bug and is it worth blocking 1.9.2 on or waiting for 1.9.3?
Comment 38 Boris Zbarsky [:bz] (still a bit busy) 2009-10-28 09:01:40 PDT
> Boris, John: what was the outcome from the discussion mentioned in c#36?

John never responded.
Comment 39 John J. Barton 2009-10-28 09:37:46 PDT
Sorry I did not see the newsgroup post. However the answer is short: we delete our 'context' for the window on unload. Firebug has a side table keyed by window and we need to clean it up. The operation dispatches to Firebug modules ('destroyContext') which may remove listeners and observers.
Comment 40 Boris Zbarsky [:bz] (still a bit busy) 2009-10-28 10:38:16 PDT
> Sorry I did not see the newsgroup post

I was sort of expecting you to make one....  Please do take this to the newsgroups?  I have a number of followup questions to your "short answer", and don't want to clutter the bug with them.  Not sure which newsgroup you prefer here; let me know once you post.
Comment 41 John J. Barton 2009-11-25 10:39:40 PST
(In reply to comment #40)
> > Sorry I did not see the newsgroup post
> 
> I was sort of expecting you to make one....  Please do take this to the
> newsgroups?  I have a number of followup questions to your "short answer", and
> don't want to clutter the bug with them.  Not sure which newsgroup you prefer
> here; let me know once you post.

How about:
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/d2c2e186b0439219#
Comment 42 Boris Zbarsky [:bz] (still a bit busy) 2009-11-25 10:59:29 PST
Seems unrelated to the unload issue, but if you want me to hijack that thread... fine.  Let's take up the unload issue there.
Comment 43 John J. Barton 2009-12-06 08:26:10 PST
Maybe I am reading something into this bug that does not exist. I assumed that any "document load started" work would be combined with "document unload completed". Anyway Firebug needs both of these together to be able to use either one.
Comment 44 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-01-08 14:11:45 PST
This bug is in ASSIGNED state, but assigned to nobody, clearly not a valid state.

Boris, this sounds like a great bug for you ;) Could you take it since you might be looking in to firebug stuff soonish, right?
Comment 45 Boris Zbarsky [:bz] (still a bit busy) 2010-01-08 14:29:13 PST
I was planning to, but it'll be at least a few weeks before I can possibly get to it.  At that point, I'll grab it.
Comment 46 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-03-02 09:43:13 PST
So the patch in bug 549539 adds a notification that can be used as a fix for this bug.

Do we want additional hooks? Such as nsIWebProgressListener ones? Or should this bug just be marked as a dependency or duplicate of that one?
Comment 47 Boris Zbarsky [:bz] (still a bit busy) 2010-03-02 10:22:23 PST
I think dependency is the way to go.  And I don't think we want progress listener hooks.
Comment 48 John J. Barton 2010-03-10 13:53:33 PST
(In reply to comment #46)
> So the patch in bug 549539 adds a notification that can be used as a fix for
> this bug.

(In reply to comment #47)
> I think dependency is the way to go.  And I don't think we want progress
> listener hooks.

I'm confused: is bug 549539 a solution for the problem described in this bug or no?
Comment 49 Boris Zbarsky [:bz] (still a bit busy) 2010-03-10 13:57:44 PST
Yes.  It adds an observer notification that fires immediately after the global scope for the document is set up.  At this point you can inject things into that scope as desired, and are guaranteed that no page scripts have run yet (though there may be properties on the global that got set up before the load started, of course).
Comment 50 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-03-10 14:41:22 PST
Bug 549539 will give you a notification that allows you to add APIs before any script runs. As Boris said in previous comment.

However there has been discussions in this bug about other things, like getting notifications before a document goes away in comment 35. Bug 549539 doesn't help with that in the least.
Comment 51 John J. Barton 2010-03-10 16:57:56 PST
Ok thanks. Just to remind Boris, we use 'unload' event handlers now to know when to unhook, which potentially interferes with cache behavior.
Comment 52 Boris Zbarsky [:bz] (still a bit busy) 2010-03-10 19:11:00 PST
Yes, fixing that is still on my plate.
Comment 53 Boris Zbarsky [:bz] (still a bit busy) 2010-05-07 22:38:53 PDT
Comment 50 paragraph 2 is basically bug 484710.  Is there anything else to do here?
Comment 54 John J. Barton 2010-05-11 07:06:39 PDT
Regarding comment 53, it's your call. All Firebug wants is an event whenever |unload| is fired, but which does not interfere with unload event handling.
Comment 55 Boris Zbarsky [:bz] (still a bit busy) 2010-05-11 07:12:24 PDT
Right.  Bug 484710 will give you that, effectively.  More precisely, it will give you a notification when an inner window with a given id is going away (whether that be due to its parent window being closed or due to navigating away without using bfcache or due to being evicted from bfcache).

This bug was fixed in bug 549539.

Note You need to log in before you can comment on or make changes to this bug.