75745 - Remove nsIPrompt impls from embedding chrome.

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Conrad Carlen (not reading bugmail)]

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

Comment 1

[Conrad Carlen (not reading bugmail)]

Attachment: boilerplate for prompt component

Comment 2

[Conrad Carlen (not reading bugmail)]

Attachment: diffs to PPEmbed to use new scheme

Comment 3

[Conrad Carlen (not reading bugmail)]

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.

Comment 4

[Christopher Blizzard (:blizzard)]

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?

Comment 5

[Christopher Blizzard (:blizzard)]

Attachment: patch for gtk to disable the internal dialogs

Comment 6

[Judson Valeski]

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 :-).

Comment 7

[Conrad Carlen (not reading bugmail)]

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.

Comment 8

[Dan M]

Attachment: mfcembed using new scheme, and hook up new scheme (unhooking the old one)

Comment 9

[Dan M]

  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.

Comment 10

[Conrad Carlen (not reading bugmail)]

> 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.

Comment 11

[Judson Valeski]

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.

Comment 12

[Christopher Blizzard (:blizzard)]

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.

Comment 13

[Chak Nanga]

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. 

Comment 14

[Judson Valeski]

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?

Comment 15

[Conrad Carlen (not reading bugmail)]

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.

Comment 16

[Christopher Blizzard (:blizzard)]

I can sr when everyone is all reviewed.

Comment 17

[Dan M]

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.

Comment 18

[Christopher Blizzard (:blizzard)]

I don't want to ship 0.9 with broken dialogs.  People actually use it.

Comment 19

[Conrad Carlen (not reading bugmail)]

Attachment: ppEmbed full patch

Comment 20

[Conrad Carlen (not reading bugmail)]

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.

Comment 21

[Christopher Blizzard (:blizzard)]

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?

Comment 22

[Conrad Carlen (not reading bugmail)]

Did you apply Dan's patch to nsDocShell.h/.cpp?

Comment 23

[Conrad Carlen (not reading bugmail)]

Whoops - ignore that. I meant nsDocShellTreeOwner.h/.cpp.

Comment 24

[Christopher Blizzard (:blizzard)]

Not yet.  Let me give that a spin.

Comment 25

[Conrad Carlen (not reading bugmail)]

Chris - hopefully, that'll do it. If so, and we're going to check this in, can
you sr my patch?

Comment 26

[Dan M]

Attachment: docshelltreeowner, cache prompts rather than making a new one each time

Comment 27

[Christopher Blizzard (:blizzard)]

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.

Comment 28

[Dan M]

.

Comment 29

[Michael Dunn]

Correction: Changing QA contact for the Embed API bugs to David Epstein.

Comment 30

[Dan M]

Attachment: winEmbed: remove nsIPrompt, implement ShowAsModal

Comment 31

[Dan M]

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.)

Comment 32

[Dan M]

  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.

Comment 33

[Conrad Carlen (not reading bugmail)]

PowerPlant code checked in.

Comment 34

[chris hofmann]

over to blizzard.  danm thinks he is done...
chris, do you have code for this?

Comment 35

[Dan M]

Attachment: implement intrinsically sized chrome windows in winEmbed

Comment 36

[Dan M]

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.

Comment 37

[Conrad Carlen (not reading bugmail)]

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...

Comment 38

[Conrad Carlen (not reading bugmail)]

Since what I mentioned probably won't happen anyway and it's much better than
what we have, r=ccarlen

Comment 39

[Dan M]

  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.

Comment 40

[Dan M]

winEmbed intrinsic sizing code is checked in

Comment 41

[Christopher Blizzard (:blizzard)]

Attachment: patch for gtk

Comment 42

[Christopher Blizzard (:blizzard)]

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.

Comment 43

[Christopher Blizzard (:blizzard)]

Attachment: final patch

Comment 44

[Christopher Blizzard (:blizzard)]

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.

Comment 45

[Conrad Carlen (not reading bugmail)]

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.

Comment 46

[Christopher Blizzard (:blizzard)]

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.

Comment 47

[Conrad Carlen (not reading bugmail)]

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.

Comment 48

[Christopher Blizzard (:blizzard)]

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.

Comment 49

[Brendan Eich [:brendan]]

I dunno why widget/public is considered a harmful embedding API.  Can someone
fill me in?

/be

Comment 50

[Conrad Carlen (not reading bugmail)]

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.

Comment 51

[Conrad Carlen (not reading bugmail)]

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.

Comment 52

[Christopher Blizzard (:blizzard)]

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

Comment 53

[Conrad Carlen (not reading bugmail)]

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).

Comment 54

[Conrad Carlen (not reading bugmail)]

After looking at the permanently spinning cursor on Mac, it can be easily fixed
if need be. I'm testing a patch.

Comment 55

[Christopher Blizzard (:blizzard)]

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?

Comment 56

[Dan M]

  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.

Comment 57

[Christopher Blizzard (:blizzard)]

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.

Comment 58

[Christopher Blizzard (:blizzard)]

Opened as:

http://bugzilla.mozilla.org/show_bug.cgi?id=78421

Comment 59

[Christopher Blizzard (:blizzard)]

Attachment: patch with ifdefs

Comment 60

[Conrad Carlen (not reading bugmail)]

Cool, opening bug 78421 was the thing to do. In the meanwhile, r=ccarlen on
latest patch. 

Comment 61

[Dan M]

I don't understand how Java gets along without wonderful ifdefs. With the 0.9 
rush and all, it's fine by me.

Comment 62

[Christopher Blizzard (:blizzard)]

I think that Java runs out of process on unix.

Comment 63

[tor]

sr=tor

Comment 64

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to 0.9

Comment 65

[Christopher Blizzard (:blizzard)]

Attachment: patch with grab

Comment 66

[tor]

Modal dialogs that are actually modal - imagine that!

sr=tor

Comment 67

[Christopher Blizzard (:blizzard)]

OK, the gtk/linux code is checked in.  Is there anything left?

Comment 68

[Conrad Carlen (not reading bugmail)]

We've got winEmbed, mfcEmbed, PPEmbed, and now gtkWidget. That leaves activex.
CC'ing Adam. I thought he was at some point...

Comment 69

[Christopher Blizzard (:blizzard)]

Hot potatoe bug!

Comment 70

[Christopher Blizzard (:blizzard)]

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.

Comment 71

[Dan M]

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.

Comment 72

[Adam Lock]

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.

Comment 73

[Adam Lock]

Attachment: ActiveX control changes to implement prompt service

Comment 74

[Adam Lock]

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.

Comment 75

[Conrad Carlen (not reading bugmail)]

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.

Comment 76

[Christopher Blizzard (:blizzard)]

+    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

Comment 77

[Adam Lock]

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.

Comment 78

[Conrad Carlen (not reading bugmail)]

> I need the web browser object
You're right - looking at what CMozillaBrowser::MessageBox does (which I should
have done before).
r=ccarlen

Comment 79

[Adam Lock]

Thanks guys fix is checked in.

Marking bug FIXED

Comment 80

[David Epstein]

reassigned qa contact to dsirnapalli. He's working with nsIPrompt

Comment 81

[Adam Lock]

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.

Comment 82

[dsirnapalli]

-- verified.