Closed Bug 75745 Opened 23 years ago Closed 23 years ago

Remove nsIPrompt impls from embedding chrome.

Categories

(Core Graveyard :: Embedding: APIs, defect)

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

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.1

People

(Reporter: ccarlen, Assigned: adamlock)

Details

(Whiteboard: want for 0.9 - missing ActiveX testbed impl)

Attachments

(13 files)

boilerplate for prompt component
7.97 KB, text/plain
Details
diffs to PPEmbed to use new scheme
2.27 KB, patch
Details | Diff | Splinter Review
patch for gtk to disable the internal dialogs
7.56 KB, patch
Details | Diff | Splinter Review
mfcembed using new scheme, and hook up new scheme (unhooking the old one)
58.52 KB, patch
Details | Diff | Splinter Review
ppEmbed full patch
30.78 KB, patch
Details | Diff | Splinter Review
docshelltreeowner, cache prompts rather than making a new one each time
5.22 KB, patch
Details | Diff | Splinter Review
winEmbed: remove nsIPrompt, implement ShowAsModal
18.21 KB, patch
Details | Diff | Splinter Review
implement intrinsically sized chrome windows in winEmbed
6.04 KB, patch
Details | Diff | Splinter Review
patch for gtk
10.48 KB, patch
Details | Diff | Splinter Review
final patch
11.54 KB, patch
Details | Diff | Splinter Review
patch with ifdefs
11.87 KB, patch
Details | Diff | Splinter Review
patch with grab
12.12 KB, patch
Details | Diff | Splinter Review
ActiveX control changes to implement prompt service
14.52 KB, patch
Details | Diff | Splinter Review 
All the embedding samples implement nsIPrompt which is gotten through a
GetInterface on their chrome. This model is going away. Now, the window watcher
supplies all prompts. The window watcher creates an nsIPromptService component
with a contract ID. If an embedding app wants it own native prompt
implemntation, it registers a factory to create the component with that contract
ID, replacing any existing one. What needs to happen is this:
(1) nsDocShellTreeOwner.cpp should not do a GetInterface to the embeddor's
chrome to get an nsIPrompt.
(2) The embedding samples need to move their prompt impls into a separate
component and then register it at startup. Boilerplate code for doing that is
coming up
The first attachment is an empty impl of nsIPromptService. Notice that
nsIPromptService is just like nsIPrompt with the addition of an nsIDomWindow as
the parent. Dan is working on something which will make it easy enough to get
from that DOMWindow to a native window. To make the conversion, just paste the
guts of each nsIPrompt method that you implement in your chrome into
nsIPromptService. Then strip out nsIPrompt from your chrome.
The second diffs are what I had to do in PPEmbed to override the component.
The other thing which needs to happen is to remove/change the code in
nsDocShellTreeOwner's GetInterface. That will be the final flip of the switch.
Target Milestone: --- → mozilla0.9
So I went ahead and did an implementation of this for gtk and then I thought
about it.  I think that by default I'd rather use the default dialogs ( the XUL
ones for now ) by default in the embedding widget.  The people who are going to
want to override it are the galeon folks and anyone who wants to remove XUL and
I don't really care about this.  I'm just going to attach a patch that
disconnects the EmbedPrompter widget.  Is that OK with you guys?
absolutely. it's up to the embeddor (the gtk embedding widget in this case). 
cool, our new model is already hard at work for us :-).
Cool - that patch makes my life easier. One less native nsIPrompt impl to worry
about in the tree. You still probably need to implement
nsIWebBrowserChrome::ShowAsModal in order for the XUL dialogs to work. The XUL
dialogs are put up by window watcher. Dan, is this right? I don't have my tree
setup right now to run with XUL prompts and see. I could say for sure later
tonight. Anyway, you should have ShowAsModal for the XUL dialogs put up by PSM2.
  Conrad/Chris: Yes. The window opening code (in WindowWatcher) notices a modal 
window, asks the docshell treeowner for an nsIWebBrowserChrome and calls 
ShowAsModal on it. In Mozilla that ends up at an nsXULWindow, which implements 
modality. Embedding guys need to implement that, too, for embedded XUL modal 
windows to work.
  A first approximation on gtk could be modeled after the Mozilla code. Just 
fall into a g_main_iteration loop until someone calls 
nsIWebBrowserChrome::ExitModalEventLoop.
> Just fall into a g_main_iteration loop until someone calls
> nsIWebBrowserChrome::ExitModalEventLoop
I found in my testing of this that ExitModalEventLoop never happened. What did
happen was the window was just destroyed which, as the API actually says :-), is
also supposed to terminate the modal loop.

r=valeski on the PPEmbed changes. We need to move nsIPromptService's CID and 
contract id in the nsIPromptService.idl though, so they're public.
hrm...when I tried it it didn't try to pop up a new window and as near as I 
could tell it never tried to create it.  I'll have to do some more digging.
r=chak on the mfcembed - with the following requests:

* Remove references to nsIPrompt from README.TXT, BrowserImpl.cpp/h etc.
* Add a couple of comment lines where we define/invoke OverrideComponents() on 
what exactly overriding components means - mainly to help current/future users 
who're basing their code on mfcembed. 
ok, so where are we on this one?
PPEmbed = r=valeski
mfcEmbed = r=chak
gtkEmbed = r=conrad???
winEmbed = dan's banging on it.
viewer?

bliz, can you sr this when all the above pieces are r'd?
I think that as far as comments go, since PPEmbed & mfcEmbed are example apps,
is this point: mfcEmbed does its overriding in a separate dll: HMODULE overlib =
::LoadLibrary(kComponentsLibname);
 whereas PPEmbed does it without a separate dll file. Both are valid and one way
might appeal to some people but not to others. It would unfortunate for somebody
to come across one way and not realize there's another. I'll put comments in
PPEmbed which point to the alternate method used by mfcEmbed and I think
mfcEmbed should do the same.

Chris, how goes the XUL dialog situation?

I also have coming up soon a complete patch for PPEmbed so that it does either
native or XUL dialogs. I didn't attach my native impl component last time.
I can sr when everyone is all reviewed.
I've added the comments Chak requested and Conrad suggested. When that patch is 
in, the current winEmbed native prompt stuff will be disabled, and is trivially 
removed. (Note winEmbed is still broken because, like gtkEmbed, it's having a 
hard time posing modal XUL windows.) But I argue that's a different (0.9.1) bug. 
(Still wants fixing, of course.) I'm ready to go.

I don't want to ship 0.9 with broken dialogs.  People actually use it.
Alright, here's the updated patch to PPEmbed. It handles, controlled by a
#define, both native prompts with an override, and XUL modal dialogs. I found
something ugly at the end that I hadn't seen until stepping through which caused
me to make quite a bit more change. Basically - problem was I was calling
nsIWebBrowser::SetContainerWindow() *after* calling nsIWebBrowser::Create().
This causes window watcher to have no chrome for my window because, at the point
at which AddWindow() gets called, it doesn't have one. That's changed now.
Another problem - in nsDocShellTreeOwner.cpp, every call to GetInterface() for
an nsIPrompt causes a whole new object to be made. This wasn't the case before
and is not in Seamonkey. Necko requests tons of them and we're making and
destroying a lot more prompts than we used to.
I'm trying to play with the modality and I can't even get a window to get
created.  Do I need to apply a patch somewhere?  Where should I start to trace
the code that creates a new window?
Did you apply Dan's patch to nsDocShell.h/.cpp?
Whoops - ignore that. I meant nsDocShellTreeOwner.h/.cpp.
Not yet.  Let me give that a spin.
Chris - hopefully, that'll do it. If so, and we're going to check this in, can
you sr my patch?
Argh.  So I can't set up ShowAsModal to work because it's usually called from
the event queue processor.  The end effect of this is that the event queue is
blocked because it's already being processed so the chrome dialog doesn't ever
finish loading.  Suck.
.
Assignee: valeski → danm
Correction: Changing QA contact for the Embed API bugs to David Epstein.
QA Contact: mdunn → depstein
Whiteboard: want for 0.9 check again on 4/20
NB: the above patch to winEmbed removes its nsIPrompt implementation (unused 
anyway, once the previous patch is checked in) and makes ShowAsModal work, which 
is what the default Gecko PromptService needs to function. It doesn't make the 
dialogs pretty. One of the missing bits is sizing to content, which is slated to 
be moved from a private interface to a public one (see bug 69922: slated for 0.9, 
but not checked in yet. Obviously no one is worrying about using this one just 
yet.)
Status: NEW → ASSIGNED
  For the love of GOD! One of the nice things about our shiny new XP codebase is 
that most features need be implemented only once. I'm happy to say it took me 
this long to go insane, working on the embedding project, because now most 
features need to be implemented five or six times: once for Mozilla and once for 
every single bloody embedding testbed. Thanks largely to Christopher and Conrad 
for pitching in.
  I was just about to gamely go update the viewer app too, when I discovered, as 
I should have known, that there are NINE more apps that need updating to support 
modal windows. If you were anywhere near me in the last ten minutes, you'd have 
gone deaf.
  Viewer has never implemented the nsIPrompt methods. If you look, they're all 
just stub functions. It's no worse now in the new world, once this patch is 
checked in. In fact, it's better. It'll at least open a window containing the 
nsIPrompt message. That's more than it's ever done before. I say we're not 
breaking Viewer by checking this patch in without first implementing it on 
viewer. I'm ready. Let's check in.

PowerPlant code checked in.
over to blizzard.  danm thinks he is done...
chris, do you have code for this?
Assignee: danm → blizzard
Status: ASSIGNED → NEW
Whiteboard: want for 0.9 check again on 4/20 → want for 0.9 -
I felt bad about big dumb looking browser-sized alerts in winEmbed, which uses 
the default (XUL) nsIPrompt. The above patch fixes that. It should be part of 
this bug. Looking for r= approval.
Dan, in WebBrowserChromeUI::SizeTo(), what if the browser is inset significantly
within the window frame? I don't think it will happen with chrome dialogs, but...
Since what I mentioned probably won't happen anyway and it's much better than
what we have, r=ccarlen
  You're right; it's coming through as a SizeBrowserTo and I'm just wrapping in 
in a window at that size. It's a safe assumption for chrome windows because I've 
made them use a special dialog resource with a browser that takes up the whole 
window content area.
  I have a real love/hate relationship with the embedding testbeds. Mostly hate. 
It doesn't make much sense to me to solve the general problem since the exact 
technique will no doubt not be applicable to any actual embedding app, even if 
it's based on winEmbed, and winEmbed itself won't suffer unless it picks up a lot 
more general polish, anyway.
  But the situation is worthy of a comment in the source. I'll do that.

winEmbed intrinsic sizing code is checked in
Whiteboard: want for 0.9 - → want for 0.9 - have win32 code, need in linux
OK, a patch is attached that fixes the problems in the window watcher service,
removes nsIPrompt from the gtk code and implements ShowAsModal.

The only problem is that the XUL dialogs are still coming up with the wrong size.
OK, this patch fixes the dialog resizing problems.  Turns out that I was
resizing the dialog before it was finished loading.  Silly me!

Looking for review and testing.
Looked good until I got to the end. Adding an nsIAppShell to windowwatcher is
not good. I thought the whole point of windowwatcher was to rid embedding apps
of appshell. Can you move that appshell business into your
ShowAsModal/ExitModalLoop? It seems to be a requirement just for you - the other
embedding apps have done this without having to add appshell into windowwatcher.
This is going to hurt anyone who is doing embedding on linux and it has to be
done anywhere where there is going to be a modal loop.  Besides, appshell is
part of the widget library which we will always pull in anyway.
But the question is: is it possible to put it in the embedding code
(ShowAsModal/ExitModalLoop)? If it's at all possible, please move it. It's not
needed on any other platform and, if it can't be moved, at least #ifdef it.
I suspect that it's not possible to move that code down to the widget code since
you will not get notifications of the XUL loading since the thread event queue
will get pushed and you will never get the dialog.

In any case this is supposed to get XP code and shouldn't be littered with
ifdefs for platform specific differences.  At first glance both the Qt port and
the Photon port need to have this implemented too.  Even though they are
relatively less important ports they shouldn't be left behind because we don't
want to include a file.
I dunno why widget/public is considered a harmful embedding API.  Can someone
fill me in?

/be
widget/public is not a harmful embedding API. Creating an instance of appshell
and spinning up an event loop with it from within an embedded app is harmful (at
least on Mac). I'll apply the patch and see.
After applying the patch, I get a permanently spinning cursor after the dialog
goes away. Creating the appshell instance has at least this as a side-affect -
not sure at this point what else. It's also a high overhead object to create.
Why is it a high overhead object?  It's just like another widget object.

So, why the spinning cursor?  Does something not finish loading?  Does the
window dismiss?  Does the appshell get destroyed?

For what it's worth this is roughly patterned after:

http://lxr.mozilla.org/seamonkey/source/xpfe/appshell/src/nsXULWindow.cpp#1285
It's high overhead because of this:
http://lxr.mozilla.org/seamonkey/source/widget/src/mac/nsAppShell.cpp#78
-making a new message pump, message sink, and memory cushion.

The spinning cursor seems to be just a cosmetic problem and has to do with the
creation of the msg pump or sink. If you really must do this, I'd have to fix
this problem on Mac.

Point is, ShowAsModal works without this patch on Mac & Windows. Adding this
appshell creation to windowwatcher, which affects all of us, to cover a problem
which exists only on gtk, is wrong (or at least a pain in the ass).

After looking at the permanently spinning cursor on Mac, it can be easily fixed
if need be. I'm testing a patch.
To your first point: unless we change the nsIWebBrowserChrome interface so that
we can push the event queue before we call ShowAsModal this has to live in the
window watcher code otherwise this won't work on linux.

Let me know about your mac patch.  Has anyone tried this on win32 yet?
  It's a peculiarity of the system we use to handle events in gtk that it needs 
appshell help when new queues are added. We could consider a lighter weight 
appshell for doing just this one thing, or maybe breaking up the initialization 
API into two methods (Init before (the current) Spinup, perhaps) that would do 
nothing for platforms that don't need this capability.
  About the way you've chosen to hook this up, though, Chris; it does make a 
dangerous assumption that only the WindowWatcher will ever push an event queue. 
There are a couple of other places this is done that could affect even embedding 
apps. There's one in nsStringBundle, for instance. That one is scary. But point 
is, it would be better to have a generalized algorithm for hooking up 
ListenToEventQueue, rather than building it straight into the WindowWatcher this 
way.
  Maybe another component inside the embedding components DLL that catches event 
queue creation and destruction in the same way that AppShellService does in 
Mozilla. And then maybe only register that component on platforms that need it.

Yeah, the more that I think about it the more that I think that this needs to be
generalized outside of the window watcher service.  This pattern of modality in
the event queues is used throughout the product so it's something that we should
fix properly.  The right place to do that might actually in the app shell
service in the call that pushes the thread event queue.

So what do we do here for now?  #ifdef it?  I need to make sure that we listen
to the new event queue before we start our document load for the dialog.  The
existing interfaces which I am loathe to change don't support it.  0.9 is
waiting on this so we need to find something quick.  I'd be OK with doing so
assuming that we open another bug about the other issue.
Cool, opening bug 78421 was the thing to do. In the meanwhile, r=ccarlen on
latest patch. 
I don't understand how Java gets along without wonderful ifdefs. With the 0.9 
rush and all, it's fine by me.

I think that Java runs out of process on unix.
sr=tor

a= asa@mozilla.org for checkin to 0.9
Modal dialogs that are actually modal - imagine that!

sr=tor

OK, the gtk/linux code is checked in.  Is there anything left?
We've got winEmbed, mfcEmbed, PPEmbed, and now gtkWidget. That leaves activex.
CC'ing Adam. I thought he was at some point...
Hot potatoe bug!
Assignee: blizzard → adamlock
Oh, does this need to be done for 0.9 or not?  I've got my fix in but I don't
know if anyone else has critical-must-fix-for-0.9 items in this bug.
Oh man. I know what *I* would do if somebody surprised me with a critical 0.9 bug 
two weeks after the tree closed for 0.9. Personally, I forgot about activeX. I 
think it's important to keep all 96 of the *&#$ embedding test beds updated, but 
we might as well bow to reality and give activeX over to 0.9.1. The only problem 
with that would come if someone was planning on building on the 0.9 release for 
their own activeX embedded widget. I don't know of any such projects (not that 
that's very telling). If there are any, I say we make apologetic noises and point 
them at the other testbeds for tips.
Whiteboard: want for 0.9 - have win32 code, need in linux → want for 0.9 - missing ActiveX testbed impl
This bug can go in the 0.9.1 pile since it's just me to do now. As long as 0.9 
compiles I'm happy for the control to be promptless for the time being.
Target Milestone: mozilla0.9 → mozilla0.9.1
Can I have a review on my patch please?

I've pretty much stuck the old prompting code into the new prompt boiler plate 
code. The only thing of note is that I've added a static member to 
CMozillaBrowser containing list of all open control instances. This allows the 
prompter to search for the correct browser instance from the provided DOM 
window.
Looks good. One question though - instead of maintaining your own list of
browser windows when windowwatcher is already doing that, can you just exploit
windowwatcher for this? In my equivalent to GetOwningBrowser, I did something
like this:

LCommander* CPromptService::GetParentCommander(nsIDOMWindow* aDOMWindow)
{

	nsCOMPtr<nsIWindowWatcher>
wwatch(do_GetService("@mozilla.org/embedcomp/window-watcher;1"));
	if (!wwatch) return nsnull;
	
	nsCOMPtr<nsIWebBrowserChrome> windowChrome;
	rv = wwatch->GetChromeForWindow(aDOMWindow, getter_AddRefs(windowChrome));
	if (NS_FAILED(rv)) return nsnull;
	
	nsCOMPtr<nsIEmbeddingSiteWindow> window(do_QueryInterface(windowChrome, &rv));
	if (NS_FAILED(rv)) return nsnull;
	
	WindowPtr macWindow;
	rv = window->GetSiteWindow(&macWindow);
	if (NS_FAILED(rv)) return nsnull;

Assuming your nsIEmbeddingSiteWindow::GetSiteWindow() returns an HWND, that
would work.


+    for (i = 0; i < CMozillaBrowser::sBrowserList.Count(); i++)
+    {
+        CMozillaBrowser *p = (CMozillaBrowser *) CMozillaBrowser::sBrowserList[i];
+        if (p->mWebBrowser)
+        {
+            nsIDOMWindow *domWindow = nsnull;
+            p->mWebBrowser->GetContentDOMWindow(&domWindow);
+            if (domWindow == parent)
+            {
+                NS_RELEASE(domWindow);
+                return p;
+            }
+            NS_RELEASE(domWindow);
+        }
+    }


Why not use nsCOMPtr<> there and use .get() to do the comparison?

Other than that and what conrad mentioned, r/sr=blizzard
I need the web browser object, not the native HWND, so I'm not sure if using 
window watcher would be any easier. 

I've changed the interface pointer code to smart pointers however.
> I need the web browser object
You're right - looking at what CMozillaBrowser::MessageBox does (which I should
have done before).
r=ccarlen
Thanks guys fix is checked in.

Marking bug FIXED
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
reassigned qa contact to dsirnapalli. He's working with nsIPrompt
QA Contact: depstein → dsirnapalli
If there is someone familiar with the issues still remaining about nsIAuthPrompt
and embedding, could they add an explanation to bug 82516 so I know what I'm
meant to do?

As far as I can tell, no embedding client uses nsIAuthPrompt any more so I'm a
wondering if this bug can be closed.
-- verified.
Status: RESOLVED → VERIFIED
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: