Test case showing blocking bug (Mozilla 1.0 - Mozilla 1.3+) of parent frame accidentally substitued for subframe in BeforeNavigate2 event
6.71 KB, application/octet-stream
6.14 KB, patch
Adam Lock: review+
|Details | Diff | Splinter Review|
In embedding situations, the embedders may need to open non gecko windows, when encountering events or JS code that tries to open windows. There should be a way to hook into the window opening mechanism and give embedders a chance to handle it. The window watcher component seems like a likely spot. But would like to get input as to a better place. I think bug 108455 is one example, although not so much an embedding example.
In particular, we've already encountered a scenario where an embedding customer is using Gecko to render their e-mail content area (not the thread or folder panes, just the view area) as well as their instant messaging conversations (they also use Gecko for editting, but that is besides the point here ...) What this embeddor wants to do is respect a user's default browser OS setting, and handle all clicks on hyperlinks that Gecko renders via an external application. They have no interest in becoming a browser themselves, just want to do mail, IM, etc. I can actually see them requesting more granular control: a mechanism to decide which clicks to handle internally via their Gecko usage, and which ones to send to an outside app.
It looks like the original intent was for embedders to implement this component themselves. But with the GRE and shared components, I wonder if this is still possible.
Perhaps nsIWindowCreator2 (which isn't frozen yet) should have a new method, by which the service can be asked (supplied with the URL if there is one) whether the new window should be created. Note that things like window.open might supply an empty URL so the implementation might have to deal with that possibility. I shall raise a new bug to cover the broader issue of trapping all targetted link clicks in addition to JS window.open commands.
I guess to me, the concepts here are a bit mixed, watcher vs ceator and what they do. But I guess these are frozen now and there's not much we can do about that. We could either change nsIWindowCreator2, since it's not frozen, or yet add another one. In any case I think rather than creating yet another function to call, I think it would be better to just ask it to create it as it does now and then return a status success code that says I've done all that's needed don't bother doing anything else. I wonder if in some instance, though, would window watcher want to keep track of these windows as well for some embedding. I guess what I'm wanting to guard against is adding to these interfaces if we don't need to. This secondary interface isn't frozen so I'd rather see the method change if that creates a simpler solution.
david - having a GRE does not prevent adding a callback similar to what adam mentioned.
Dougt, what I was curious about is that the window watcher code mentions that it expects embedding clients to implement their own windowwatcher component. But the contract ID is used all over the place. Meaning that the embedder would have to reuse the contract ID. If the user downloads Mozilla with one implementation of the component and then uses the embedders software and the GRE is shared, I would think there might be potential problems there. I guess an embedder could use the same contract ID, but have a different class ID, but I'm not sure that gets around all the issues.
The embedder would register their own service at runtime, replacing the default window watcher with their own one using the same contract ID. It wouldn't require any changes to the component registry and would only affect that one app. It's similar to how embedders may register their own prompt or directory provider service already. With that said, the window watcher is substantial and complicated object touching all kinds of private interfaces and I can't see any embedder wanting to reimplement it. It would be better to factor out any bits that they might like to override and leave the core alone.
Created attachment 116440 [details] [diff] [review] rough cut at one possible solution This is one possible way. I'm kind of torn between modifying create window vs window watcher. Changing this, doesn't have any secondary impacts. Changing create window would probably require any embeddors, if there really are any, that have implemented their own window watcher to modify their version to handle this new behavior. Embedders would create their own component that implements nsIWindowWatcherHook This was a quick draft, it compiles, and it runs, although I haven't actually exercised the hook.
Assigning to new owner, as per today's API review. Nominating topembed.
Discussed in edt; plussing.
Created attachment 118661 [details] Test case showing blocking bug (Mozilla 1.0 - Mozilla 1.3+) of parent frame accidentally substitued for subframe in BeforeNavigate2 event
Created attachment 119738 [details] [diff] [review] add url and cancel parameters to window creator interface Adam, David and I have talked about this and I think everyone can be happy with this patch, which simply sends the url of the window about to be opened (from the window.open call in script, for example) to the nsIWindowCreator method an embeddor implements to create the new window. I've also added an explicit |out boolean cancel| parameter and made it clear that window creation can be canceled. This is based on Microsoft's WebBrowser API method NewWindow2. We can't emulate it exactly because in our system there's no default window creation mechanism should the embeddor refuse. In our codebase, if the WindowCreator cancels window creation it generally ends up an exception being thrown in script, so the rest of the function is aborted. Works fine in the tests I've tried. Note though that Mozilla's window creator never cancels. Adam, David: what you say?
Comment on attachment 119738 [details] [diff] [review] add url and cancel parameters to window creator interface A couple of nits: Is there a reason for using the character string url when there is a 'uriToLoad' variable in OpenWindowJS that could supply it as an nsIURI instead? Documentation for nsIWindowCreator2 should use capitalisation. Also window watcher should set 'cancel' to PR_FALSE just to be on the safe side.
Created attachment 119836 [details] [diff] [review] add url and cancel parameters to window creator interface all those things
Comment on attachment 119836 [details] [diff] [review] add url and cancel parameters to window creator interface r=adamlock
Comment on attachment 119836 [details] [diff] [review] add url and cancel parameters to window creator interface sr=blizzard