Closed Bug 205425 Opened 21 years ago Closed 6 years ago

turn the embedding widget into a gre client

Categories

(Core Graveyard :: Embedding: GTK Widget, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: blizzard, Assigned: mpgritti)

References

Details

Attachments

(4 files, 11 obsolete files)

167.81 KB, patch
Details | Diff | Splinter Review
10.63 KB, patch
Details | Diff | Splinter Review
77.47 KB, patch
Details | Diff | Splinter Review
17.97 KB, patch
Details | Diff | Splinter Review
I need to turn the embedding widget into a gre client.  This would allow clients
to link against only the embedding library and then be able to use the gre to
load components instead of the old XPCOM way.
I've got a patch that does this but it breaks the crap out of static builds. 
Does anyone still use those?  I don't even know how to test them.
Attached patch patch (obsolete) — Splinter Review
This patch moves the embedding widget out into its own directory.  The only
thing that the embedding code knows how to do is start up the gre and talk to
the backend component.	The backend component is basically what the
EmbedPrivate class used to do, accessed through an interface instead of
accessing the public members of the class directly.
the static builds are important.  They give us a relative measurement of how
small  we can make mozilla browser.  Maybe the best approach would be to make
the static build disable using the GRE for TestGtkEmbed.  

To test the static build, in your mozconfig file add these options:

ac_add_options --disable-shared
ac_add_options --enable-static

Doug and I were looking into the fact that with this patch I can't view https
pages.  I've traced it back to a problem in nsNSSComponent::Init:

http://lxr.mozilla.org/seamonkey/source/security/manager/ssl/src/nsNSSComponent.cpp#1283

The call:

  if (!mPref) {
    mPref = do_GetService(NS_PREF_CONTRACTID);
    NS_ASSERTION(mPref, "Unable to get pref service");
  }

actually fails, and things go downhill from there.  Something is pretty fishy.
Woops, I take that back.  That call is actually succeeding.  It can't find the
user's profile dir (in InitializeNSS:)

    rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
                                getter_AddRefs(profilePath));
    if (NS_FAILED(rv)) {
      PR_LOG(gPIPNSSLog, PR_LOG_ERROR, ("Unable to get profile directory\n"));
      return rv;
    }

I've messed with the directory provider.  I've probably screwed something up.
Attached patch patch #2 (obsolete) — Splinter Review
This patch fixes clients who call one of the global widget functions before
creating a widget.
Attachment #123081 - Attachment is obsolete: true
Attached patch patch #3 (obsolete) — Splinter Review
Include recent accessibility changes and make it work with gtk 1.2 builds as
well.
Attachment #123151 - Attachment is obsolete: true
Attachment #123988 - Flags: review?(dougt)
Comment on attachment 123988 [details] [diff] [review]
patch #3

Please continue to support the static build for TestGtkEmbed.

Please verify the dates on the licenses used.

You may want to consider moving the methods ifdef'ed by MOZ_ACCESSIBILITY_ATK
in nsIGtkEmbedStub.h to the end of that declaration.  

embedding/browser/gtk/tests/Makefile.in

You should see if you can try to remove these string libs.  Doing so will
reduce your app size by ~100k however, it will restrict you to using only
nsEmbed(C)String.

+		$(DIST)/lib/$(LIB_PREFIX)string_s.$(LIB_SUFFIX) \
+		$(DIST)/lib/$(LIB_PREFIX)string_obsolete_s.$(LIB_SUFFIX) \


Are these files suppose to be the same?  Could you just attach a diff of those
changes?

embedding/browser/gtk/widget/gtkmozembed2.cpp|h and
embedding/browser/gtk/src/gtkmozembed2.cpp|h

This patch will still require the embedder to have their library path pointing
at the gtkwidget.  It might have been nice to completely decouple the widget
from the GRE.  Is that planned?
> Please continue to support the static build for TestGtkEmbed.

I'm not even sure how to build the thing, although looking at the makefiles it
looks like an abomination.  Can someone point me at a doc or something?

> embedding/browser/gtk/tests/Makefile.in
> 
> You should see if you can try to remove these string libs.  Doing so will
> reduce your app size by ~100k however, it will restrict you to using only
> nsEmbed(C)String.
> 
> +		$(DIST)/lib/$(LIB_PREFIX)string_s.$(LIB_SUFFIX) \
> +		$(DIST)/lib/$(LIB_PREFIX)string_obsolete_s.$(LIB_SUFFIX) \
> 

I can't.  In fact, I just wasted the last two hours trying.  It's far worse than
you could possibly imagine.

TestGtkEmbed.cpp includes two interfaces:

#include "nsIDOMKeyEvent.h"
#include "nsIDOMMouseEvent.h"

which through various twists and turns requires nsAString.h which pulls in
nsReadableUtils.h and then you end up with a link error:

../../../../dist/bin/libgtkembedmoz.so: undefined reference to
`Distance(nsReadingIterator<unsigned short> const&, nsReadingIterator<unsigned
short> const&)'
../../../../dist/bin/libgtkembedmoz.so: undefined reference to
`Distance(nsReadingIterator<char> const&, nsReadingIterator<char> const&)'

The only way that I've even got things linking is to pull in string_s and
string_obsolete_s into the embedding shared lib and then making sure that the
final app links against embedstring_s.  Otherwise, you get multiple definitions
of various symbols or it doesn't link because of missing symbols.

In fact, using just about any interface header requires in the end pulling in
our string headers and then our world turns to shit.  If what I've just
experienced is even a taste of what our embedders are in for, they are going to
roast us.  I had to remove all of the nsAString& arguments from nsIGtkEmbedStub
because of the linking problems from just that simple interface.

Why have we been working on this for 5 years and our string classes _still_ have
not been flushed out where something like this would be easy?  It's a 4 inch
curb and we're tripping over it again and again.

> 
> Are these files suppose to be the same?  Could you just attach a diff of those
> changes?
> 
> embedding/browser/gtk/widget/gtkmozembed2.cpp|h and
> embedding/browser/gtk/src/gtkmozembed2.cpp|h

They are pretty hacked up.  It's easier to just look at the new file.

> 
> This patch will still require the embedder to have their library path pointing
> at the gtkwidget.  It might have been nice to completely decouple the widget
> from the GRE.  Is that planned?

The widget will be the interface to the GRE.  That's the idea.
to build a static-build, set the following:
ac_add_options --disable-shared
ac_add_options --enable-static



You should be able to #include nsAString just use the embed library -- that is
its whole point.  Are you defining XPCOM_GLUE when building?
Attached patch patch #4Splinter Review
Just checkpointing changes.  Lots of good fixes in here, but still incomplete.
Attachment #123988 - Attachment is obsolete: true
Attachment #123988 - Flags: review?(dougt)
blizzard: FYI, the packaging branch that I am going to land immediately upon
open of 1.7a simplifies and makes sane the static build process. In addition,
the epiphany folks are very interested about getting gtkembed to be a GRE client
so that they can layer on top of the GRE.

--BDS
There's code here to do it, ugly as it is.  The biggest problem with doing this,
of course, is that strings aren't frozen so you can't use them in the widget
itself and that adds a whole mess of code.

The embedding people in question rely heavily on our strings impl and it will be
quite a shock to them when it's no longer available.

Also, I never did manage to get a working static build, so I wasn't ever able to
fix problems related to trying to the static bits in the makefile.  That's what
kept me from really landing it.
Does the recent string changes improve the situation to be able to fix this one ?
http://groups.google.it/groups?dq=&hl=it&lr=&ie=UTF-8&oe=UTF-8&threadm=mailman.1077263223.23581.mozilla-xpcom%40mozilla.org&prev=/groups%3Fdq%3D%26num%3D25%26hl%3Dit%26lr%3D%26ie%3DUTF-8%26oe%3DUTF-8%26group%3Dnetscape.public.mozilla.xpcom%26start%3D25

Since nsEmbedString is API frozen it should be possible to use it in the widget
(and in gtkmozembed users too).
I experimented a bit with the new string api and found
http://bugzilla.mozilla.org/show_bug.cgi?id=238928.
Maybe it should be marked as a dependency of this bug.
Attached patch Attempt at a different approach (obsolete) — Splinter Review
It depends on 238928 to be checked in. It works but I had to strip out several
things, see below.
The final target is to depend only on the SDK. That way we dont need the
intermediate interface of Blizzard patch.

What it does:

- Use only nsEmbedC?String. To transfrom between unichar and uft8 strings use
glib native API.
- Use xpcom glue and GRE startup API

Problems:

- We need a way to convert from utf8/unichar to gtk1 strings (locale ?)
- nsProfileDirServiceProvider.h is not part of the sdk. It looks like we should
use nsIProfileService instead.
- We need to port the focus code to use nsIWebBrowserFocus.
http://bugzilla.mozilla.org/attachment.cgi?id=126558&action=view does it but
these is the issue with toplevel active state.
- We need a way to register dom listeners without passing through PIDOMWindow.
- We cant use nsIWidget (see 239132). So we need a different implementation of
ShowTooltip. Also I'm not sure what's the deal is with the extra screen window
stuff, though that needs to be solved differently too.
- Admittely the string code is not optimal, especially the utf8/utf16
conversion stuff. Suggestions are welcome.

Anyway, is there interest in a use-only-SDK approach ? If so I can keep working
on this and try to solve the problems. I'd be grateful for any, positive or
negative feedback.
I'm not sure what you mean by that.  If you're asking if the embedding widget
should only use SDK-exported items, I think the answer is no.  It's part of the
Mozilla build and should probably stay that way.  It can use internal interfaces
since in theory it should be revved whenever the rest of the runtime is revved.
blizzard, the problem is that using not SDK api you inevitably include internal
string implementation and hence you cant use nsEmbedString.
At that point I dont see other possible solution to this bug then the interface
you added in your patch. Though that adds a lot of code and you dont seem to be
happy with it.
Is there any way we can move the work on this bug further ? I dont mind if it
takes time but atm it's just completely stalled. Apparently there is no plan to
freeze string api, apart the nsEmbedString bits. And we cant use nsEmbedString
if we want more than the SDK.
well, it would be theoretically nice for gtkembed/mfcembed to use only frozen
interfaces and exports, but that is currently impractical. Let's focus on
getting them working with the GRE at all, first... then we can look at what
might need to be frozen to make them frozen-happy.
I wasn't all that worried about using the interface split.  That seemed to work
fine.

I was worried about the lack of decent strings and it's impact on things like
epiphany and galeon and other embedding clients.  Suddenly not having those
classes around was going to suck because the GRE doesn't include them.  But if
you don't mind, I don't mind.  To me it was just kind of intractable.
> I was worried about the lack of decent strings and it's impact on things like
> epiphany and galeon and other embedding clients.  Suddenly not having those

Hopefully, we will have solved most of the problems w/ nsEmbedString by the time
1.7 final ships.  Here's some things that should help: bug 238088, bug 239303.
As per Darin comment I think nsEmbedString is shaping up in an acceptable way
for epiphany. I'm a bit worried about some internal apis that we are using (and
that will likely drag in nsString), I'll check ASAP what are the problems there.

I think we have two possible ways of doing the port. One is your interface
thing. The other is starting to move towards frozen interface usage and use
nsEmbedString. Darin agreed that we could fix 239132 to ease the process a bit.
That would allow to use nsIWidget that seem the only unsolvable problem with my
patch. Using nsIProfileService doesnt seem an issue. Focus behavior is borked in
any way but IHMO nsIWebBrowserFocus behavior is slightly better. I can cleanup
the string code with the changes darin is working on. The remaining issue that
I'd need to look in is DOM events.
The approach of my patch has the advantage to be moving gradually towards a more
stable API. If we introduce an interface in the middle, and make big changes to
the code for it, then we will be forced to maintain it forever.

Though that's your call. If you prefer we can just complete your patch (i.e. fix
static build). And I can see if there are problems with it in epiphany and how
we can solve them.
FWIW the DOM events problem needs to be solved for epiphany, with your interface
or without, if we dont want to regress functionalities.
I don't have any personal stake in using my patch vs. not using my patch. 
You're doing the work at this point, so I suggest you do what you think is
right.  In general, I think that moving to using embedding strings is the right
way to go.  As for the stability of the interface, it hasn't changed much in the
last couple of years and I don't expect that it will change all that much going
forward.  When I have changed things, I've changed them in such a way as to
maintain backwards compatibility whenever possible.
>In general, I think that moving to using embedding strings is the right
>way to go.

Ok, I think I'll deal with it this way.

1 Port gtkmozembed to use nsEmbedString
2 Port epiphany to use nsEmbedString
3 make gtkmozembed a GRE client
Depends on: 239789
Sounds like a good plan to me.  But boy are the galeon people gonna be pissed. :)
-> marco since he's working on it.
Assignee: blizzard → marco
(In reply to comment #26)
> 1 Port gtkmozembed to use nsEmbedString

What exactly does that mean?  Isn't the gtkmozembed widget part of Mozilla?  Why
does it need to use nsEmbedString?  Or what do you mean by use?

Why shouldn't it be implemented so its interfaces use a forward-declared
nsA[C]String, the implementation uses the internal stuff, and the clients using
the interfaces can use nsEmbedString?
Ok, I talked this with blizzard and I think I have a more clear idea of what we
need.
                                                                               
                                              
The final target is to api freeze libgtkmozembed.so and decouple it from the GRE
(see dougt comment about this too). It will be installed in the system library
and will be our interface to the GRE. Users will just need to link to it to be
able to use mozilla SDK.
                                                                               
                                              
I can think to two ways to do this:
                                                                               
                                              
1 Have an intermediate, api stable, GRE component (see patch #4) that
libgtkmozembed.so use to interface to other mozilla apis
2 Use frozen only api in libgtkmozembed.so itself
                                                                               
                                              
My points against 1 are:
                                                                               
                                              
- it's a poor excuse to not be able to create an usable mozilla SDK :)
- it's much more difficult to use frozen api in the libgtkmozembed.so users code
(epiphany and galeon as examples) than in gtkmozembed itself. In fact they
currently depend on a lot of private api while gtkmozembed use mostly public
stuff. I'm pretty sure that by the time epiphany and galeon can work without
internal apis, there will be no problems to do the same for gtkmozembed.
At that point what will be the advantage to use internal, not frozen api ?
                                                                               
                                              
If we go for 2, we need to use api frozen strings at some point (nsEmbedString).
                                                                               
                                              
Now, even hoping to be able to implement something like epiphany or galeon with
frozen api is unrealistic on the short term. So we need a migration path.
I agreed with Blizzard that using a versioned libgtkmozembed.so can work well
enough for now. We could also have versioned .pc files. Starting from there we
can gradually move users (galeon/epiphany/devhelp basically) to nsEmbedString
and frozen interface. Once they are ready it will be easy to make libgtkmozembed
itself api stable.
So my revised "roadmap" is:
                                                                               
                                              
- make gtkmozembed a GRE client
- install it in system lib path, with a version
- port epiphany/galeon to frozen api
- port gtkmozembed to frozen api
                                                                               
                                              
The last step will allow us to remove the version from the library name and to
not link anymore string_s.la in gtkmozembed. Since gtkmozembed is pretty simple
code I dont think powerfull mozilla strings will be very useful, not worth the
added code size to the lib IHMO. Also, linking string_s in libgtkmozembed.so
allow embedders to use internal string api -> libgtkmozembed is no more api stable.

I hope this reply to your questions dbaron. Does it make more sense ?

Btw I have epiphany working with a gtkmozembed GRE client, with string_s linked
to libgtkmozembed.so. I had to remove some code that drag in xpcom pieces
(nsVoidArray for example) and so cause undefined references. They looks all
easily fixable though.
>The last step will allow us to remove the version from the library name

nit-pick: If libgtkmozembed.so is going to live in /usr/lib, then it should
definitely be versioned, and it should surely remain versioned forever :-)
>nit-pick: If libgtkmozembed.so is going to live in /usr/lib, then it should
>definitely be versioned, and it should surely remain versioned forever :-)

Oh yeah. The confusion rise from the fact that I was talking of two different
levels of versioning. blizzard suggested something like libgtkmozembed-1-6.so
(for mozilla 1.6) to deal with the not yet frozen interfaces issue. I'm guessing
the reason to not use sonames here is parallel installation.

From http://www106.pair.com/rhp/parallel.html :

    Why aren't library sonames good enough?

    Library sonames only address the problem of runtime linking 
previously-compiled applications. They don't address the issue of compiling
applications that require a previous version

The same document also show how dealing with the different versions can be made
painless by using pkg-config.

So, until we have a not frozen api we could use
libgtkmozembed-$(major)-$(minor).so.0.0.0
When we will have stable api libgtkmozembed.so.$(soname)

One issue is how to deal with frozen api revision. At least I'm assuming frozen
api will not be frozen forever and, for example, GRE 2.0 could have incompatible
changes. In that case we will certainly want to keep the ability to install in
parallel. But it's just matter to find a naming scheme that accomodates that too.
Just to clarify something about what I said; I really want the embedding widget
to use the GRE instead of firing things up by hand.  I think that it should be
versioned as well, but that's second to using the GRE instead of how it does
things now.  I also don't really care if the embedding widget uses internal
interfaces (although this means it should really be versioned.)  I don't want to
introduce bogo-modularity here.  It's just not appropriate.
what does this mean to uses that statically link the embedding widget and the
rest of the GRE goop?
Attached patch use the GRE (obsolete) — Splinter Review
The code is still rough but I think it gives an idea of what I want to do.
Comments would be very appreciated.

I had to change the profiles code to use nsIProfile because
nsProfileDirServiceProvider use nsIAtom stuff which is not part of the xpcom
glue. So it was causing undefined refs. Also I think using nsIProfile make
sense because it doesnt require to link a static library, it's supposed to be
public and it's what other embedding implementations use.

Unfortunately I've not been able to make EmbedStream works. Not linking
libxpcom breaks it because of nsIPipe. Pipe is not registered as an xpcom
component so I cant create the instance that way either. Any suggestion on how
to deal with this ?
Attachment #145167 - Attachment is obsolete: true
(In reply to comment #34)
> what does this mean to uses that statically link the embedding widget and the
> rest of the GRE goop?

I guess the static build could continue to work as before. The changes I'm
making are 1 fixup the code that requires not-glue xpcom to be linked 2 use the
GRE startup/shutdown 3 move libgtkmozembed in system directory and version it.
They would not seem to affect the static build to me. It's possible I'm missing
something though, since my understanding of the static build is limited.
fwiw bug 99338 vetod exposing nsPipe
Attached patch use the GRE (complete) (obsolete) — Splinter Review
I'm adding a nsIWebBrowserStream interface. I think it make sense because it's
something generally useful for embedding. In fact it's used in two of our
internal implementations (photon and gtkmozembed), there are GNOME applications
using it through gtkmozembed and there are requests in the newsgroups for such
functionality.

The patch should be more or less complete now. I'm missing scrollbars/prompts
strings in epiphany now, though that's very likely a problem with my
installation. Will verify before finalizing the patch and asking a review.
Attachment #145690 - Attachment is obsolete: true
can you "cvs add" the file, and then diff -N so we can see the new interface? Or
if you don't have CVS access, can you just attach the new .idl?
you can use cvsdo <http://viper.haque.net/~timeless/redbean/> if you don't have
cvs access to add the file (as long as you don't need to create a directory,
which you shouldn't need to do).
(In reply to comment #39)
> can you "cvs add" the file, and then diff -N so we can see the new interface? Or
> if you don't have CVS access, can you just attach the new .idl?

The .idl is at the bottom of the attached patch (145736). I did get it using
diff -u /dev/null nsIWebBrowserStream.idl
Attached patch use the GRE (ready for review) (obsolete) — Splinter Review
Attachment #145736 - Attachment is obsolete: true
Comment on attachment 145798 [details] [diff] [review]
use the GRE (ready for review)

Not sure how to deal with the new interface review. If you cant review it
please let me know and I'll split that part out and ask review to someone else.

webBrowser/nsEmbedStream.cpp/h is just a renamed copy of
gtk/src/EmbedStream.cpp.
The s/nsMemory::Alloc/g_strdup changes are not gratouitous. The GRE need to be
running for nsMemory to work. I could not think of another way to fix it
without forcing a startup call.
Attachment #145798 - Flags: review?(blizzard)
Comment on attachment 145798 [details] [diff] [review]
use the GRE (ready for review)

>Index: embedding/browser/gtk/src/EmbedPrivate.cpp
>@@ -387,6 +384,49 @@

>+  if (!sAppFileLocProvider) {
>+    nsCOMPtr<nsIDirectoryServiceProvider> directoryProvider;
>+    EmbedDirServiceProvider *embedProvider;
>+
>+    embedProvider = new EmbedDirServiceProvider(nsDependentCString(sProfileDir));
>+    directoryProvider = do_QueryInterface(embedProvider);
>+    SetDirectoryServiceProvider(directoryProvider);
>+  }

This can be a lot simpler, because you already know the type of
EmbedDirServiceProvider:
nsCOMPtr<nsIDirectoryServiceProvider> dirProvider = new
EmbedDirServiceProvider(...);
SetDirectoryServiceProvider(dirProvider);

>Index: embedding/browser/gtk/src/Makefile.in

> SHARED_LIBRARY_LIBS= \
>-		$(DIST)/lib/libembed_base_s.$(LIB_SUFFIX) \
>-		$(DIST)/lib/libprofdirserviceprovider_s.$(LIB_SUFFIX) \
>+		$(DIST)/lib/$(LIB_PREFIX)xpcomglue.$(LIB_SUFFIX) \
>+		$(DIST)/lib/$(LIB_PREFIX)string_s.$(LIB_SUFFIX) \
>+		$(DIST)/lib/$(LIB_PREFIX)embed_base_s.$(LIB_SUFFIX) \
> 		$(NULL)

Linking against string_s is going to cause problems... we don't NS_HIDDEN all
of the string implementations. How much trouble would it be to convert all of
gtkmozembed to use embedstring; what exactly is being linked in here?

>--- /dev/null	2004-02-23 22:02:56.000000000 +0100
>+++ embedding/browser/gtk/src/EmbedDirServiceProvider.cpp	2004-04-10 10:35:56.114747344 +0200

>+NS_IMETHODIMP
>+EmbedDirServiceProvider::GetFile(const char *prop, PRBool *persistant, nsIFile **_retval)

>+  if (strcmp(prop, NS_APP_APPLICATION_REGISTRY_DIR) == 0)

>+  else if(strcmp(prop, NS_APP_APPLICATION_REGISTRY_FILE) == 0)

>+  else if(strcmp(prop, NS_APP_USER_PROFILES_ROOT_DIR) == 0)

>+  else if(strcmp(prop, NS_GRE_DIR) == 0)

Does this actually work?  The profiledirserviceprovider includes all sorts of
"sub-profile" keys such as the prefs file, localstore file, etc, and most code
is not designed to deal with these keys not being defined.  This is a known
issue, which we probably need to discuss.

>+++ embedding/browser/webBrowser/nsEmbedStream.cpp	2004-04-09 01:06:03.000000000 +0200

>+nsEmbedStream::Init(void)

>+  rv = NS_NewPipe(getter_AddRefs(bufInStream),
>+                  getter_AddRefs(bufOutStream));
>+                                                                                                                              
>+  if (NS_FAILED(rv)) return rv;

Check whitespace here... the patch has a line full of blanks.

>+nsEmbedStream::ReadSegments(nsWriteSegmentFun aWriter, void * aClosure,
>+			    PRUint32 aCount, PRUint32 *_retval)
>+{
>+  char *readBuf = (char *)nsMemory::Alloc(aCount);
>+  PRUint32 nBytes;

Why use nsMemory::Alloc instead of malloc()? This buffer does not change
ownership across XPCOM boundaries, so using nsMemory is unnecessary.
> This can be a lot simpler, because you already know the type of
> EmbedDirServiceProvider:
> nsCOMPtr<nsIDirectoryServiceProvider> dirProvider = new
> EmbedDirServiceProvider(...);
> SetDirectoryServiceProvider(dirProvider);

Nice, fixed.
 
> Linking against string_s is going to cause problems... we don't NS_HIDDEN all
> of the string implementations. How much trouble would it be to convert all of
> gtkmozembed to use embedstring; what exactly is being linked in here?

There are two independent problems here:

1) We would not expose private string api to the gtkmozembed users anymore.

I think it's ok to expect they port to nsEmbedString since the api is shaping up
nicely. Though some of the private interfaces they use drag in nsString.h. I
know private interfaces should not be used but ... both epiphany and galeon use
some of them and we should probably provide a way to migrate gradually.
Do you expect problems if they keep linking to libxpcom at compile time until
they ported to nsEmbedString ? The GRE startup seem to handle the case where
libxpcom is already linked (GRE_HOME == ".")

2) We need to port gtkmozembed to use nsEmbedString.
I have that almost done in bug 239789. To complete it we need to checkin bug
239132 and to fix bug 81835 and bug 239716 (these two are necessary to port
epiphany/galeon too). Then we can either use nsIWebBrowserFocus instead of our
own implementation (my favourite solution, see bug 226708) or to make
nsPIDOMWindow.h nsEmbedString friendly. It currently includes nsIDocument and
nsString but both appear to be leftover headers?

> >+  else if(strcmp(prop, NS_APP_USER_PROFILES_ROOT_DIR) == 0)
> 
> >+  else if(strcmp(prop, NS_GRE_DIR) == 0)
> 
> Does this actually work?  The profiledirserviceprovider includes all sorts of
> "sub-profile" keys such as the prefs file, localstore file, etc, and most code
> is not designed to deal with these keys not being defined.  This is a known
> issue, which we probably need to discuss.

Yeah it seem to work fine. prefs.js for example is created in the right dir and
saved correctly. FWIW this is also what all other embedding clients and camino
are doing. If you could do an example of what you fear would not work I can
verify it.

> >+++ embedding/browser/webBrowser/nsEmbedStream.cpp	2004-04-09
01:06:03.000000000 +0200
> 
> >+nsEmbedStream::Init(void)
> 
> >+  rv = NS_NewPipe(getter_AddRefs(bufInStream),
> >+                  getter_AddRefs(bufOutStream));
> >+                                                                           
                                                  
> >+  if (NS_FAILED(rv)) return rv;
> 
> Check whitespace here... the patch has a line full of blanks.
> 
> >+nsEmbedStream::ReadSegments(nsWriteSegmentFun aWriter, void * aClosure,
> >+			    PRUint32 aCount, PRUint32 *_retval)
> >+{
> >+  char *readBuf = (char *)nsMemory::Alloc(aCount);
> >+  PRUint32 nBytes;
> 
> Why use nsMemory::Alloc instead of malloc()? This buffer does not change
> ownership across XPCOM boundaries, so using nsMemory is unnecessary.
> 

These was part of the original EmbedStream.cpp. Fixed them too.
Comment on attachment 145798 [details] [diff] [review]
use the GRE (ready for review)

Please upload your new patch.
Attachment #145798 - Flags: review?(blizzard) → review-
Attachment #145798 - Attachment is obsolete: true
Comment on attachment 145959 [details] [diff] [review]
port to nsEmbedString and address reviewer comments

Since finding a good api to attach listeners is problematic I made current code
work with nsEmbedString just with headers cleanups for now. I still plan to
look at it though.
I had to move the xpcom initialization in gtkmozembed2.cpp otherwise we cant
use nsEmbedString variable in the class.
The CloneString helpers repeated in several files are really ugly. We can
remove them once bug 239716 is fixed though.
Attachment #145959 - Flags: review?(bsmedberg)
Forgot to mention that AFAIK this should still work with current galeon/epiphany
code as long as they link with libxpcom.so at compile time. Obviously the whole
point of the GRE is missed that way, but I think it's good to not suddenly break
that code.
Attachment #145959 - Flags: superreview?(blizzard)
marco pointed out that attachment 145959 [details] [diff] [review] should fix bug 225952 too.
Blocks: 225952
Attachment #145959 - Flags: superreview?(blizzard)
Attachment #145959 - Flags: review?(bsmedberg)
Attachment #145959 - Attachment is obsolete: true
Comment on attachment 146611 [details] [diff] [review]
Use the new embed string clone api -> get rid of helpers

I'd really like to get this one in 1.8a so that we can address possible issue
in the 1.8 cycle. If there is anything I can do to ease the review (for example
split in stream interface addition/headers cleanups/gtkmozembed patch) please
let me know.
Attachment #146611 - Flags: review?(bsmedberg)
Comment on attachment 146611 [details] [diff] [review]
Use the new embed string clone api -> get rid of helpers

>Index: embedding/browser/gtk/src/EmbedContentListener.cpp
> #include "nsIURI.h"
>+#include <nsMemory.h>
> 

>-    nsXPIDLCString value;
>+    char *value;
>     rv = catMgr->GetCategoryEntry("Gecko-Content-Viewers",
>-				  aContentType, 
>-				  getter_Copies(value));
>+				  aContentType, &value);
> 
>     // If the category manager can't find what we're looking for
>     // it returns NS_ERROR_NOT_AVAILABLE, we don't want to propagate
>@@ -134,6 +133,8 @@
> 
>     if (value && *value)
>       *_retval = PR_TRUE;
>+
>+    nsMemory::Free(value);

nsMemory::Free is not null-safe, unfortunately. You should do a null-check
here.

>Index: embedding/browser/gtk/src/EmbedPrivate.cpp
>===================================================================

More angle-brackets.

>+  nsCOMPtr<nsILocalFile> binDir; 
>+  if (sCompPath) {
>+    rv = NS_NewNativeLocalFile(nsEmbedCString(sCompPath), 1, getter_AddRefs(binDir));
>+    if (NS_FAILED(rv)) return rv;
>+  }

Use PR_TRUE instead of magic '1'.

I would encourage using NS_ENSURE_SUCCESS(rv, rv) for new error-checking which
isn't supposed to fail. You get useful debug spew if things go wrong.

>+  rv = NS_InitEmbedding(binDir, sAppFileLocProvider);
>+
>+  NS_RELEASE(sAppFileLocProvider);
>+  sAppFileLocProvider = nsnull;

NS_RELEASE will automatically set the var to null, you don't need the second
assignment.

But why do we have sAppFileLocProvider if we're just going to use it like a
local comptr?

>@@ -541,65 +542,46 @@

>+  nsCOMPtr<nsIWebBrowser> webBrowser;
>+  mWindow->GetWebBrowser(getter_AddRefs(webBrowser));
>+  nsCOMPtr<nsIWebBrowserStream> wbStream = do_QueryInterface(webBrowser);
>+  return wbStream->OpenStream(aBaseURI, aContentType);

Defensive coding nit: might you need to do a null-check on wbStream? (Here and
many places just like it below.)

>+    // Set the profile which the control will use
>+    nsCOMPtr<nsIProfile> profileService;
>+    profileService = do_GetService(NS_PROFILE_CONTRACTID, &rv);
>+    if (NS_FAILED (rv)) return rv;

NS_ENSURE_SUCESS() here and below

>Index: embedding/browser/gtk/src/Makefile.in
> 		  pref \
>+                  profile \
> 		  windowwatcher \
>-		  profdirserviceprovider \
>+		  profile \
> 		  $(NULL)

You've got profile twice, once with mismatched indenting.

>@@ -1223,7 +1237,7 @@
>   embedPrivate = (EmbedPrivate *)embed->data;
>                    
>   if (embedPrivate->mWindow)
>-    retval = ToNewUnicode(embedPrivate->mWindow->mTitle);
>+    retval = NS_StringCloneData(embedPrivate->mWindow->mTitle);
>                    
>   return retval;

mismatched allocators here? aren't this and others below supposed to be
g_malloced?

>Index: embedding/browser/gtk/tests/Makefile.in

> EXTRA_LIBS	+= \
>+		-lplds4 -lplc4 -lnspr4 -lpthread -ldl \
> 		$(TK_LIBS) \

use $(NSPR_LIBS)

>Index: embedding/browser/webBrowser/nsWebBrowser.h

>@@ -108,6 +110,7 @@
>                      public nsIWebProgressListener,
>+		     public nsIWebBrowserStream,
>                      public nsSupportsWeakReference

tab alert

Finally, we have to worry about using nsIProfile. I am in the process of moving
libprofile.so out of the GRE, because the new toolkit isn't going to be using
it... since gtkmozembed wasn't using the profile impl (I don't think) it wasn't
an issue I worried about, but this patch changes that. I really want to wean
everyone off using the old profile code if possible, and I'm afraid this is a
step in the wrong direction.
Attachment #146611 - Flags: review?(bsmedberg) → review-
Depends on: 241384
No longer depends on: 239789, 241384
The profile problem is being a blocker. We cannot use the old toolkit profile
dir service static library as is because it uses nsString. We would have to
build two separate libraries from the same code (defined) see bug 241384.
Additionally I'm not convinced binding embedding to the old toolkit is a good
idea: for one it would be nice to have compreg.dat per user as in the new toolkit.
I tend to think embedding should either be able to reuse new toolkit profile
stuff (bug 239929) or to have his own.
Attached patch address review comments (obsolete) — Splinter Review
Attachment #146611 - Attachment is obsolete: true
> >Index: embedding/browser/gtk/src/EmbedPrivate.cpp
> >===================================================================
> 
> More angle-brackets.

I fixed all the inclusion in that file while at it.

> But why do we have sAppFileLocProvider if we're just going to use it like a
> local comptr?

Not sure what you mean exactly, it can set by SetDirectoryProvider method so
it's not local. Anyway, my new patch doesnt touch that code anymore so there
should not be problems.

> >@@ -1223,7 +1237,7 @@
> >   embedPrivate = (EmbedPrivate *)embed->data;
> >                    
> >   if (embedPrivate->mWindow)
> >-    retval = ToNewUnicode(embedPrivate->mWindow->mTitle);
> >+    retval = NS_StringCloneData(embedPrivate->mWindow->mTitle);
> >                    
> >   return retval;
> 
> mismatched allocators here? aren't this and others below supposed to be
> g_malloced?

I'm not sure if it's worth to fix these. I think they are just kept for
compatibility. Gtk1 didnt support utf8 and these was used to be able to convert
to locale. Clients may even be using nsMemory::Free, prolly not worth changing
this now.

> Finally, we have to worry about using nsIProfile. I am in the process of moving
> libprofile.so out of the GRE, because the new toolkit isn't going to be using
> it... since gtkmozembed wasn't using the profile impl (I don't think) it wasn't
> an issue I worried about, but this patch changes that. I really want to wean
> everyone off using the old profile code if possible, and I'm afraid this is a
> step in the wrong direction.

I had to resort doing my own implementation of profile dir service + lock, it's
based on your toolkit patch. In fact nsIProfile store the data in a unique
directory and keep it in the register. I think it's better for gtkmozembed to
have a single profile implementation, changing this back later would involve
some sort of importing pain ...
Attachment #147012 - Attachment is obsolete: true
Comment on attachment 147014 [details] [diff] [review]
actually fix the last two comments

For EmbedPrivate.cpp you may want to look at the -w diff attached. There are
some changes in it in respect to the last patch you reviewed because of the new
profile approach. It's less intrusive though.
Attachment #147014 - Flags: review?(bsmedberg)
Comment on attachment 147014 [details] [diff] [review]
actually fix the last two comments

Yick, I hate forking the profile code yet again, but until we have a better
solution it will have to do.
Attachment #147014 - Flags: review?(bsmedberg) → review+
Attachment #147014 - Flags: superreview?(blizzard)
Why must nsProfileLock.cpp be copied?  How about just moving it to a common place?

/be
QA Contact: pavlov → brendan
brendan: we cannot link code using nsEmbedString and the "real" nsAString into
the same module; we would have to recompile that code using #defines to switch
between nsCString (for libprofile.so) and nsEmbedCString (for statically-linked
embedding consumers).

It's possible to do that for both nsProfileDirServiceProvider and nsProfileLock.
I'm a C hacker, I like macros ;-).  Do what is right, but try not to duplicate
hunks of code of this size -- experience suggests we'll regret doing so.

/be
Depends on: 241384
Comment on attachment 147014 [details] [diff] [review]
actually fix the last two comments

sr=blizzard

This all looks fine.  However, I have one request and that's that you rename
the PushStartup and PopStartup methods in the class, because you've moved that
code down into the actual widget code.	They don't actually push or pop
anything anymore.
Attachment #147014 - Flags: superreview?(blizzard) → superreview+
although i probably said it enough times, please do not break the static build.
Thanks for the reminder, before posting the final patch I'll actually do a
static build and verify (I'd not expect it to break but ...). Though I want to
get bug 241384 solved before anything else.
Looks like problems with this bug are never going to finish. There are two 
issues with the static build:

1 nsStaticComponent.h drags in nsString
2 according to Darin embed and private strings cannot be mixed in the same 
DSO. Since the static build link everything in one executable I guess this is 
going to be an issue.

1 should not be too hard to fix but I cant think to any good solution to 2. 
AFAIK the new toolkit semi-single-profile code has the same issue since it use 
embed string in the dir service provider.
> 2 according to Darin embed and private strings cannot be mixed in the same 
> DSO. Since the static build link everything in one executable I guess this is 
> going to be an issue.

You can mix them in the same DSO provided you ensure that all the nsAC?String
methods (defined in nsStringAPI.h) are inlined.  In an optimized GCC build this
is the case; however, in the debug builds we use -fno-inline, so the problem
would show up then.

Another option is to limit this code to use only the C-style API (i.e., the
NS_C?String* functions in nsStringAPI.h).
Can you combine a different underlying name (different symbol names) with a
macro (same code) to make (2) go away?
> Can you combine a different underlying name (different symbol names) with a
> macro (same code) to make (2) go away?

Possibly.  Care to spell out what you had in mind?
Could you spell out exactly what the problem is, or point to something that does?
Er, I think I see what it is.  So my proposed solution would be to reverse the
#defines here:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/string/public/nsStringAPI.h&rev=1.6&mark=635-636#620

so that any compiler that didn't inline the inline methods inlined them with a
different name.
Also, while there, it might not be a bad idea to do something like adding this
to nsStringAPI.h:

#if !defined(NS_STRINGAPI_IMPL) && defined(nsAString_h___)
#error "Cannot include both nsStringAPI.h and nsAString.h"
#endif

and this to nsAString.h:

#if !defined(NS_STRINGAPI_IMPL) && defined(nsStringAPI_h__)
#error "Cannot include both nsStringAPI.h and nsAString.h"
#endif

(perhaps with a comment on the include guards in the opposite files that they're
used in such a manner)
This idea has the disadvantage that it would require including nsStringAPI.h
before including any headers that forward-declared nsAString/nsACString.
I filed bug 243109 regarding improvements to nsStringAPI.h/nsAString.h (such as
what dbaron has suggested here).
Depends on: 243169
Making this depend on the external/internal strings definition problem even if 
I'm not experiencing issues on a debug static build on linux.

Darin, is the problem supposed to show only on some platforms or is it broken 
in a more subtle way than simply build and run?
Depends on: 243109
> Darin, is the problem supposed to show only on some platforms or is it broken 
> in a more subtle way than simply build and run?

Are you building with -fno-inline?  I think the problem only shows up if the
nsAString methods declared in nsStringAPI.h are _NOT_ inlined.
> Are you building with -fno-inline?  I think the problem only shows up if the
> nsAString methods declared in nsStringAPI.h are _NOT_ inlined.

Yeah I am. You can see a log of the TestGtkEmbed build here:
http://gnome.org/~marco/testgtkembed_log
The dependent patches landed. We are left with the internal/external strings
conflict problem.
If no one is opposed I'd ask review for the latest changes and land it. My linux
static debug build compile and works fine.
If that's not ok please just speak up and I'll wait bug 243109 to be fixed.
now that nsEmbedStream exists, and nsIWebBrowserStream, in cross-platform code -
can /embedding/browser/gtk/src/EmbedStream.* be cvs removed?
Well, until I can actually land my patch (still blocked on bug 243109)
EmbedStream is in use, so it should not be removed.
Comment 81 sounded like you could land this anyway, if nobody objected (nobody
did), but I don't see the review requests.  The internal/external string thing
sucks, no doubt, but I'm not certain that it needs to block this progress based
on my reading of the bug and the patch.

What am I missing?
The patch is at this point bitrotten. Parts of it are already in the tree
though. The only part left is the actual embed string port, which will be easier
once bug 264274 is done.
Depends on: 264274
No longer depends on: 243109
Blocks: 268520
Depends on: 270110
The actual cvs mozilla-minimo source compilation results in a "Could not find a compatible GRE" when running the TestGtkEmbed. I'm posting it here because it is  related to the TestGtkEmbed and the GRE, I think you can help a lot woth this!
I'm not sure that there's anything left to do in this bug: gtkmozembed clients can be GRE clients as of bug 299988. The "could not find a compatible GRE" warning is because you haven't either registered your build in /etc/gre.d as a GRE or set GRE_HOME envvar to override.
(In reply to comment #87)
> I'm not sure that there's anything left to do in this bug: gtkmozembed clients
> can be GRE clients as of bug 299988. The "could not find a compatible GRE"
> warning is because you haven't either registered your build in /etc/gre.d as a
> GRE or set GRE_HOME envvar to override.
> 

But I'm compiling statically, then there is no xpcom to set the GRE_HOME, neither a /etc/gre.d directory.
You're compiling what statically? gtkmozembed is part of libxul now, so if there's no libxul there's no chance that there's an available gtkmozembed.
QA Contact: brendan → gtk-widget
Product: Core → Core Graveyard
Embedding: GTK Widget isn't a thing, closing.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.