Oleg Romashin (:romaxa) Assignee
	Description • 18 years ago

Current version of GtkMozEmbed widget does no have any interface for providing prompt signals to external application...

After applying of this patch will be possible to receive PROMPT signals if defined "MOZ_NO_GECKO_UI_FALLBACK_1_8_COMPAT".

	timeless
	Comment 2 • 18 years ago

Comment on attachment 234642 [details] [diff] [review]
Patch for extending GtkMozEmbed Interface.

I'm not currently a peer for this.

The design is basically:
If there is a signal listener, ask it to manage the dialog, if not, behave as the current code does.

An exception is made for a specially built system which wouldn't do this. It is expected that normal Linux distributions would *not* turn this on in general at least not until all imaginable applications that they ship have implemented the interface. At that point if some Linux distribution wishes to do something like that, it's on their heads when users install applications which don't support it.

The minimal embedding configuration for devices which control their own destiny will turn on that flag sooner.

Note that this is still a sync API, an asynchronous one where Gecko spins up the loop and waits is possible, but the results aren't particularly good as you end up with windows freezing waiting for the js stack to unwind. I am trying to construct a proposal for that, but I need it to be more developed before I consider trying to implement it.

If people would rather the API we implement here be asynchronous with the Gecko pushing an event loop, we can do that. Keep in mind that users won't actually benefit from it because the js stack will not unwind.

	Christian Persch (GNOME) (away; not receiving bug mail)
	Comment 3 • 18 years ago

(In reply to comment #2)
> The design is basically:
> If there is a signal listener, ask it to manage the dialog, if not, behave as
> the current code does.
What exactly is the application for this? The only thing I can think of would be an
embedded system where you don't want to pop up dialogues but present the alert/prompt
embedded in your main UI somehow...

> An exception is made for a specially built system which wouldn't do this. It is
> expected that normal Linux distributions would *not* turn this on in general at
> least not until all imaginable applications that they ship have implemented the
> interface. 
I don't think any regular desktop program would want to use this. Or, if they really
wanted to override the prompts, they'd provide their own nsIPromptService
implementation.
So I guess one other goal of this path is to avoid the client the need to program
an XPCOM component. If that's the case, I think gtkmozembed should also handle the
other things that you have to do here: if we assume that the alert/prompt signal
handler is going to run a recursive mainloop (it has to, unless it always wants to
return a predefined answer), gtkmozembed should push a NULL JS context and set the
window modal state around the signal emission.
 
> Note that this is still a sync API, an asynchronous one where Gecko spins up
> the loop and waits is possible, but the results aren't particularly good as you
> end up with windows freezing waiting for the js stack to unwind.
[...]
> If people would rather the API we implement here be asynchronous with the Gecko
> pushing an event loop, we can do that. Keep in mind that users won't actually
> benefit from it because the js stack will not unwind.

I think this is ok to be sync, as long as real async isn't possible.

	Christian Persch (GNOME) (away; not receiving bug mail)
	Comment 4 • 18 years ago

Review comments for attachment 234642 [details] [diff] [review]

	LeoZ
	Comment 5 • 18 years ago

(In reply to comment #4)
> Created an attachment (id=234662) [edit]
> review
> 
> Review comments for attachment 234642 [details] [diff] [review] [edit]
> 

[.......]

>+  gboolean (* select)          (GtkMozEmbed *embed, const char *title, const char *text,
>+                                GList *list, gint *selected_item);

> This will break the gtkmozembed ABI by changing the struct size of GtkMozEmbedClass.
> What's the mozilla policy on that; is this allowed?

I'd like to apologies if I'd eventually miss interpret the mozilla policy, but since the changes
above suppose to be applied to mozilla trunk the ABI compatibility is not an issue because
it's going to change anyway.

> Also, do we really need class closures for these signals? The gtk_signal_handler_pending
> checks in GtkPromptService will emit the signal only when there are handlers connected; it
> will _not_ check for class closures.

I didn't quite get this point but there are no any new classes added to the ones already presented in gtkmozembed.

>+typedef enum
>+{
[...]
>+} GtkMozEmbedDialogButtons;

> Is this enum used anywhere?

It is going to be used in future ;-)
But it can be remove it if it harms anything.

[.......]

>Index: EmbedTools.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/embedding/browser/gtk/src/EmbedTools.cpp,v
>retrieving revision 1.63
>diff -ruN -p -u8 EmbedGtkTools.cpp.orig EmbedGtkTools.cpp
>--- EmbedGtkTools.cpp.orig	1970-01-01 02:00:00.000000000 +0200
>+++ EmbedGtkTools.cpp	2006-08-18 21:07:21.000000000 +0300

> Is this EmbedTools.cpp or EmbedGtkTools.cpp ?

Typo: EmbedTools.cpp.

>@@ -0,0 +1,76 @@
>+ * The Initial Developer of the Original Code is
>+ * Oleg Romashin. Portions created by Oleg Romashin are Copyright (C) Oleg Romashin.  All Rights Reserved.

> This code is just moved here from GtkPromptService, isn't it?

Yes, but this is a change that shall be documented, isn't it so?

>+GtkWidget * GetGtkWidgetForDOMWindow(nsIDOMWindow* aDOMWindow)
>+{
>+    nsCOMPtr<nsIWindowWatcher> wwatch = do_GetService("@mozilla.org/embedcomp/window-watcher;1");
>+
>+    if (!aDOMWindow)
>+        return NULL;
>+
>+    nsCOMPtr<nsIWebBrowserChrome> chrome;
>+    wwatch->GetChromeForWindow(aDOMWindow, getter_AddRefs(chrome));
>+    nsCOMPtr<nsIEmbeddingSiteWindow> siteWindow = do_QueryInterface(chrome);

> I know this is just moved code, but while I'm at this, have you tested this for frames?
> Because Epiphany uses GetTop on the DOM window before trying GetChromeForWindow.

This is original gtkmozembed implementation and it was not actually changed.
If there is a need to tweak it on a way how it handled, for example, in Epiphany, than, I think,
the fastest way to submit a separate patch.

>Index: EmbedTools.h
>+#include <nsCOMPtr.h>
>+#include <nsString.h>
>+#include <nsIDOMWindow.h>
>+#include <nsIWindowWatcher.h>
>+#include <nsIWebBrowserChrome.h>
>+#include <nsIEmbeddingSiteWindow.h>
>+#include <nsIServiceManager.h>
>+#include <gtk/gtk.h>
>+
>+#ifndef __EmbedTools_h
>+#define __EmbedTools_h

> Most of these includes are unnecessary, and they should be inside the #ifndef.

Thanks, it is going to be fixed.

[.......]

The rest of the script it to technical for me since I'm, well, just an average user.
But as every average user I have an opinion ;-)

	timeless
	Comment 6 • 18 years ago

Comment on attachment 234662 [details]
review

> This will break the gtkmozembed ABI by changing the struct size of GtkMozEmbedClass.
> What's the mozilla policy on that; is this allowed?

https://bugzilla.mozilla.org/attachment.cgi?id=10707&action=view is the best example i can find of this.

I'm not sure how this stuff works for gtk. for windows structures the size of the structure is part of the structure and a caller and callee agree on how to interpret it based on that size field.

Is the concern that someone might have derived a "clazz" of their own from gtkmozembed and that they will therefore be unable to interact with this extended interface?

if so, what will actually happen? (sorry, i really don't understand gtk)
will the app crash, or refuse to run or will we just not dispatch the signals?

the gecko xpcom way has been to say that in general you can not dervive an interface from someone else's unfrozen interface (and that in general you shouldn't dervive an interface from any interface you don't own with the exception of nsISupports). In this way, multiple inheritance is always the fault of a single module's poor design.

note that gtkmozembed isn't like gecko, it's like plugins, changes made are flash frozen and must to my knowledge be supported forever.

>Also, do we really need class closures for these signals? The gtk_signal_handler_pending
>checks in GtkPromptService will emit the signal only when there are handlers connected; it
>will _not_ check for class closures.

I don't understand enough about gtk to comment on this.

I have no opinion about how to do the dispatch so long as we can recognize that it wasn't handled. I believe we'll adopt your suggestion.

>These are only used for gtk1 and this won't work since those marshalers simply don't exist
>in gtk1. I assume this was never tested on gtk1, right?

indeed only gtk2 was tested. :(

> Is gecko 1.9 even targetting gtk1?

i'm not sure about gecko, we should probably include the equivalent gtk1 changes, i don't want to be responsible for breaking gtk1.

> If we can drop gtk1 support here,

see above.

> we should use |g_signal_new[v]| which will let
> you define a signal accumulator (which is required for the TRUE return value
> to be correctly handled). gtk_signal_new doesn't support accumulators on gtk2
> (it's automatic on gtk1, iirc). (See also bug 297303.)

fun

> Also, what happens for NULL |parentWidget|? I think there still may be some places
> where prompts don't have parents.

i warned about this. my suggestion, which i thought was going to be implemented was to desperately search for a mozgtkembed window and dispatch to it. If you have a better suggestion, we're open to it.

> I'm not sure that typeof(PRBool) == typeof(gboolean) on every platform?
> [Same for the other instances of this.]

Yeah, we definitely shouldn't make these PR[x] == g[x] assumptions.

	Benjamin Smedberg
	Comment 7 • 18 years ago

I will take responsibility for breaking gtk+ for gtkmozembed; I'm pretty sure I already have and that the makefiles no longer build gtkmozembed with gtk+.

	Benjamin Smedberg
	Comment 9 • 18 years ago

Comment on attachment 238489 [details] [diff] [review]
Updated patch

This is timeless' baby now ;-)

	timeless
	Comment 10 • 17 years ago

Comment on attachment 238489 [details] [diff] [review]
Updated patch

something like this has happened....

	Oleg Romashin (:romaxa) Assignee
	Comment 11 • 17 years ago

Fixed

	Oleg Romashin (:romaxa) Assignee
	Comment 12 • 17 years ago

VERIFIED   

	Christian Persch (GNOME) (away; not receiving bug mail)
	Comment 14 • 17 years ago

Seems no one ever answered my question in comment 3 about the reason for this change? And more, if one embedder wanted to implement nsIPromptService like this, why wouldn't they just write their own component instead ?

	Marco Pesenti Gritti
	Comment 15 • 17 years ago

I agree with Christian here. If an embedder really need to do this it can implement his own prompt service. Wrapping nsIPromptService in GObject signals (!) is ugly and unnecessary.

	Benjamin Smedberg
	Comment 16 • 17 years ago

In the moz2 timeframe XPCOM is "going away" (it's going to be unavailable to embedders). So it is a longer-term goal to have all the necessary embedding hooks available as a stable exported API.

I'm not sure about the actual implementation suggested here, though, since I don't know anything about GObject signals.

	Christian Persch (GNOME) (away; not receiving bug mail)
	Comment 17 • 17 years ago

(In reply to comment #16)
> In the moz2 timeframe XPCOM is "going away" (it's going to be unavailable to
> embedders). So it is a longer-term goal to have all the necessary embedding
> hooks available as a stable exported API.

Maybe rather inversely, the apparent need for some embedders to provide nonstandard implementations of those contracts should tell you NOT to remove this capability ?

	Marco Pesenti Gritti
	Comment 18 • 17 years ago

Benjamin, is there any documentation about the long-term plan? It's hard for me
to review/comment on this specific patch without knowing the future plans.

	Benjamin Smedberg
	Comment 19 • 17 years ago

I can't find a good overview... I will try to write one up: but basically all of XPCOM is moving from a stable binary interface based on reference counting to a conservative garbage collector; we are hoping to maintain similar code-level APIs, but there is no intention for the internal APIs to be stable, even among dot-releases.

What we really want, from an embedding standpoint, is a stable API at the edge, so that an embedder can register some sort of prompt service, security UI, etc (whatever hooks the embedder needs) and the embedding API impl will convert that into XPCOM contractid overrides or whatever the internal mechanism is.

	Marco Pesenti Gritti
	Comment 20 • 17 years ago

Thanks for the explanation, Benjamin.

Even in the mozilla 2.0 world it seem like we will need a way to expose public interfaces (being it xpcom or whatever) at a lower level then embedding. I'm thinking to DOM here or to advanced functionalities which are normally not required by embedding applications.

I don't think we should try to grow a huge, embedding specific API to cover all the possible use cases.

We have a default gtk nsIPromptService implementation which covers the the vast majority of the use cases (I still see no answer to Christian question of what an application would use the hooks for). Overriding it should be possible, but IHMO not through the high level embedding API.

Furthermore I think mozilla 2.0 related work should be done on the mozilla 2.0 branch. We can orient a bit the direction of the mozilla 1.9 development based on what we anticipate we will do in 2.0, but adding interfaces which are completely unnecessary in the 1.9 world doesn't seem like a good idea.

	Oleg Romashin (:romaxa) Assignee
	Comment 21 • 17 years ago

I think it is not very good idea to keep UI inside gtk embedding widget.
nobody is using this UI, and everybody overriding it.

EphyPromptService.cpp (kazehakaze, galeon, epyphany)

I guess it would be better remove EmbedPrompter from gtkmozmebed and move on UI side (TestGtkEmbed) by using signals interface.

	Oleg Romashin (:romaxa) Assignee
	Comment 22 • 17 years ago

Also current implementation of EmbedPrompter does not contain handling of BUTTON_DELAY_ENABLE flag.

	Marco Pesenti Gritti
	Comment 23 • 17 years ago

Full browser implementation needs are very specialized. There is no way you are going to cover all of them at the gtk embedding level. They will always need access to the lower level API. And, if we are talking about 1.9 here, implementing nsIPromptService is a perfectly good mechanism to override the default dialogs. And in fact, as you note, several applications are doing it already.

Christian can you clarify why you need custom prompt service, btw. It's a little suspect that it has been cut and pasted in other applications :)

	Marco Pesenti Gritti
	Comment 24 • 17 years ago

(In reply to comment #22)
> Also current implementation of EmbedPrompter does not contain handling of
> BUTTON_DELAY_ENABLE flag.
> 

Do you see any problem with fixing that?

	Christian Persch (GNOME) (away; not receiving bug mail)
	Comment 25 • 17 years ago

(In reply to comment #23)
> Christian can you clarify why you need custom prompt service, btw. It's a
> little suspect that it has been cut and pasted in other applications :)

I think the current gtkmozembed prompt service is fine for an application that uses this to show scripted html content from trusted origins (e.g. yelp, devhelp, etc), but not for a web browser.
Epiphany's prompt service implements the button delay for bug 162020, string length limiting for bug 318282, and also has fixes for bug 325639 and bug 334891. Also it fixes the i18n problem in bug 196517, is GNOME HIG compliant, and additionally implements nsINonblockingAlertService.

I guess the other embedders (galeon, kazehakase) are implementing their own prompt service for similar reasons.

	Marco Pesenti Gritti
	Comment 26 • 17 years ago

Do you see any problem with fixing all of these in the gtkmozembed implementation?

	Christian Persch (GNOME) (away; not receiving bug mail)
	Comment 27 • 17 years ago

They're fixable in gtkmozembed, except for the i18n problem.

	Oleg Romashin (:romaxa) Assignee
	Comment 28 • 17 years ago

This patch supposed to remove GTK UI code from gtkmozembed at all.

Next patch will add this UI somewhere in TestGTkEmbed

	Oleg Romashin (:romaxa) Assignee
	Comment 29 • 17 years ago

Removed alert_check and confirm_check handlers, used extended alert and confirm.
Fixed some crashes.

	Christopher Blizzard (:blizzard)
	Comment 30 • 17 years ago

The original reason why we had the prompt service in the widget was that I wanted to make sure that someone could start using the widget with very little code.  And prompts are a basic requirement of the web.  If there's an opportunity to pull in code that will fix them, or make them more GNOMEy, I would be very happy about that.  I don't want to see a widget that doesn't work with the web out of the box.

Looking at the bug it seems that localization is an issue, but we might be able to provide some hooks to fix that?  Or provide some callbacks?

	Oleg Romashin (:romaxa) Assignee
	Comment 31 • 17 years ago

TestEmbedPrompter patch
should be used together with 
"293869: Updated patch"

Contain Updated EmbedPrompter* and TestGtkEmbed.

	Marco Pesenti Gritti
	Comment 32 • 17 years ago

(In reply to comment #30)
> The original reason why we had the prompt service in the widget was that I
> wanted to make sure that someone could start using the widget with very little
> code.  And prompts are a basic requirement of the web.  If there's an
> opportunity to pull in code that will fix them, or make them more GNOMEy, I
> would be very happy about that.  I don't want to see a widget that doesn't work
> with the web out of the box.

Agreed.

And I'd also prefer to not see EphyPromptService (or similar) cut/pasted by all the gtkmozembed users.

> Looking at the bug it seems that localization is an issue

Is there any reason we can't use the normal mozilla localization? (I have no clue about it).

	Christopher Blizzard (:blizzard)
	Comment 33 • 17 years ago

The Mozilla localization system is pretty different than what people use in just about every other open source project on the planet.  i.e. it uses localization bundles instead of .po files.  We've got a huge number of translations now, but I'll bet there's a worry on the app side that those translations will be done for an app in one language but not for mozilla.  Or that translation won't be shipped by the distro or something.

A couple of options might be:

1. Hell, just use .po files for the embedding widget.  That would be different than the rest of moz.  But we're right at the border of cultures, so it might be worth it in this case to make things easier for people who want to use it in GNOME apps or in an embedding context.  (What do the Nokia folks think is good?  I'm curious.)
2. Do some measurements on the language coverage and try and get some guarantees about what language packs we should include for embedders.
3. See if we can come up with a fallback for apps to do the xlations?  Not even sure how this would work.  iirc, gettext is global and this might be very challenging.

	Benjamin Smedberg
	Comment 34 • 17 years ago

We are mainly talking about the "use the system XULRunner that comes with your distro" case, right? In that case, we should just encourage the distros to ship an all-locale XULRunner, which would be pretty simple.

In any case, if Mozilla doesn't have a localization that will cause more problems than just for prompts: there are many internal strings that are used throughout (e.g. the default "Submit" button title).

	Marco Pesenti Gritti
	Comment 35 • 17 years ago

I agree. The "Address not found" page for example, can be quite a problem if not localized:

http://dev.laptop.org/ticket/2520

I just need to figure out how to add an all-locale to the Fedora packages :)

	LeoZ
	Comment 36 • 17 years ago

While the localizations issues are quite important, they probably require a different bug report.

I'm still curious about this particular issue.
Would it be possible to get it integrated to the GtkMozEmbed?

	Doug Turner (:dougt)
	Comment 37 • 15 years ago

Comment on attachment 293869 [details] [diff] [review]
Updated patch

sorry, i haven't gotten to this review. I am probably not the right person for embedding since I haven't thought about it in a while.

we probably need to first figure out what the mozilla embedding story is....

	Benjamin Smedberg
	Comment 38 • 15 years ago

The group working on the replacement embedding API may have this, but I don't think we want to do this as-is.

	Shawn Wilsher :sdwilsh
	Comment 39 • 15 years ago

Comment on attachment 293869 [details] [diff] [review]
Updated patch

clearing review request on a closed wontfix bug