Closed Bug 342715 Opened 18 years ago Closed 15 years ago

Need an API to allow extensions to hook "document load started" events

Categories

(Core Graveyard :: Embedding: APIs, defect)

Product:
Core Graveyard
Component:
Embedding: APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: roc, Unassigned)

References

Details

(Whiteboard: [firebug-p1])

Attachments

(2 obsolete files)

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.
Attached patch patch (obsolete) — — Splinter Review
the aforementioned patch
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".
Or to summarize, I suspect it's better to just introduce a new flag for this.
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?
> 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?
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?
but it's good that you're adding this, this is occasionally asked for in various newsgroups
(In reply to comment #6) > you are thinking of C++ extensions I suppose? You can use this from JS. Greasemonkey does.
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.
I see what you mean. The version problems still exist for existing C++ users.
darin, can you comment here?
Status: NEW → ASSIGNED
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.
(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?
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.
The lock appears to appear and disappear correctly.
Attachment #227032 - Flags: superreview?(darin)
Attachment #227032 - Flags: review?(bzbarsky)
roc, I'm not going to get to this in a reasonable timeframe. Maybe ask biesi?
Comment on attachment 227032 [details] [diff] [review] patch sure
Attachment #227032 - Flags: review?(bzbarsky) → review?(cbiesinger)
Comment on attachment 227032 [details] [diff] [review] patch seems ok, I think... but shouldn't this new flag combination be documented somewhere?
Attachment #227032 - Flags: review?(cbiesinger) → review+
Sure. Where, in the comments in nsIWebProgressListener?
that's probably the best place, yeah
Attached patch updated patch (obsolete) — — Splinter Review
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.
Attachment #227032 - Attachment is obsolete: true
Attachment #232440 - Flags: superreview?(darin)
Attachment #232440 - Flags: review?
Attachment #227032 - Flags: superreview?(darin)
Attachment #232440 - Flags: review? → review?(cbiesinger)
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.
Attachment #232440 - Flags: review?(cbiesinger) → review-
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.
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.
Depends on: 83265
Attachment #232440 - Attachment is obsolete: true
Attachment #232440 - Flags: superreview?(darin)
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.
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.
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.
Blocks: 391177
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.
No longer blocks: 391177
Whiteboard: [firebug-p2]
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.
Flags: blocking1.9.1?
I like your plan in comment 28: both DOM event and progress notification.
At this point, I don't see this blocking the release as we're adding an API. blocking1.9.1-. Renom if you disagree.
Flags: blocking1.9.1? → blocking1.9.1-
Trying to get this on the 1.9.2 todo list.
Flags: wanted1.9.2?
Assignee: roc → nobody
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.
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.
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.
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?
> Boris, John: what was the outcome from the discussion mentioned in c#36? John never responded.
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.
> 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.
(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#
Seems unrelated to the unload issue, but if you want me to hijack that thread... fine. Let's take up the unload issue there.
Whiteboard: [firebug-p2] → [firebug-p1]
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.
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?
Status: ASSIGNED → NEW
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.
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?
Depends on: 549539
I think dependency is the way to go. And I don't think we want progress listener hooks.
(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?
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).
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.
Ok thanks. Just to remind Boris, we use 'unload' event handlers now to know when to unhook, which potentially interferes with cache behavior.
Yes, fixing that is still on my plate.
Comment 50 paragraph 2 is basically bug 484710. Is there anything else to do here?
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.
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.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: