Last Comment Bug 205425 - turn the embedding widget into a gre client
: turn the embedding widget into a gre client
Status: NEW
:
Product: Core Graveyard
Classification: Graveyard
Component: Embedding: GTK Widget (show other bugs)
: Trunk
: x86 Linux
: -- normal with 2 votes (vote)
: ---
Assigned To: Marco Pesenti Gritti
:
Mentors:
Depends on: 241384 243169 264274 270110
Blocks: gtk2 225952 268520
  Show dependency treegraph
 
Reported: 2003-05-12 16:42 PDT by Christopher Blizzard (:blizzard)
Modified: 2012-04-05 00:46 PDT (History)
19 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (157.99 KB, patch)
2003-05-12 16:48 PDT, Christopher Blizzard (:blizzard)
no flags Details | Diff | Splinter Review
patch #2 (158.92 KB, patch)
2003-05-13 10:17 PDT, Christopher Blizzard (:blizzard)
no flags Details | Diff | Splinter Review
patch #3 (163.98 KB, patch)
2003-05-22 12:04 PDT, Christopher Blizzard (:blizzard)
no flags Details | Diff | Splinter Review
patch #4 (167.81 KB, patch)
2003-06-10 10:31 PDT, Christopher Blizzard (:blizzard)
no flags Details | Diff | Splinter Review
Attempt at a different approach (46.84 KB, patch)
2004-03-31 02:33 PST, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review
use the GRE (16.85 KB, patch)
2004-04-08 10:33 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review
use the GRE (complete) (45.46 KB, patch)
2004-04-09 03:55 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review
use the GRE (ready for review) (37.23 KB, patch)
2004-04-10 01:53 PDT, Marco Pesenti Gritti
blizzard: review-
Details | Diff | Splinter Review
port to nsEmbedString and address reviewer comments (79.20 KB, patch)
2004-04-12 17:13 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review
Use the new embed string clone api -> get rid of helpers (85.93 KB, patch)
2004-04-20 12:44 PDT, Marco Pesenti Gritti
benjamin: review-
Details | Diff | Splinter Review
address review comments (104.98 KB, patch)
2004-04-25 15:31 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review
EmbedPrivate -w diff (10.63 KB, patch)
2004-04-25 15:33 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review
actually fix the last two comments (104.97 KB, patch)
2004-04-25 15:55 PDT, Marco Pesenti Gritti
benjamin: review+
blizzard: superreview+
Details | Diff | Splinter Review
Fix the static build, use the new standalone profile dir service, rename Push/PopStartup (77.47 KB, patch)
2004-05-17 02:11 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review
split out the headers cleanup and stream interface addition (17.97 KB, patch)
2004-05-29 09:59 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review

Description Christopher Blizzard (:blizzard) 2003-05-12 16:42:07 PDT
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.
Comment 1 Christopher Blizzard (:blizzard) 2003-05-12 16:46:34 PDT
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.
Comment 2 Christopher Blizzard (:blizzard) 2003-05-12 16:48:16 PDT
Created attachment 123081 [details] [diff] [review]
patch

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.
Comment 3 Doug Turner (:dougt) 2003-05-12 17:22:45 PDT
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

Comment 4 Christopher Blizzard (:blizzard) 2003-05-13 07:29:02 PDT
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.
Comment 5 Christopher Blizzard (:blizzard) 2003-05-13 09:04:45 PDT
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.
Comment 6 Christopher Blizzard (:blizzard) 2003-05-13 10:17:03 PDT
Created attachment 123151 [details] [diff] [review]
patch #2

This patch fixes clients who call one of the global widget functions before
creating a widget.
Comment 7 Christopher Blizzard (:blizzard) 2003-05-22 12:04:48 PDT
Created attachment 123988 [details] [diff] [review]
patch #3

Include recent accessibility changes and make it work with gtk 1.2 builds as
well.
Comment 8 Doug Turner (:dougt) 2003-05-23 14:41:31 PDT
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?
Comment 9 Christopher Blizzard (:blizzard) 2003-05-27 08:43:55 PDT
> 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.
Comment 10 Doug Turner (:dougt) 2003-05-27 09:47:08 PDT
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?
Comment 11 Christopher Blizzard (:blizzard) 2003-06-10 10:31:51 PDT
Created attachment 125322 [details] [diff] [review]
patch #4

Just checkpointing changes.  Lots of good fixes in here, but still incomplete.
Comment 12 Benjamin Smedberg [:bsmedberg] 2003-11-25 11:49:55 PST
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
Comment 13 Christopher Blizzard (:blizzard) 2003-11-29 17:53:41 PST
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.
Comment 14 Marco Pesenti Gritti 2004-03-15 07:24:36 PST
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).
Comment 16 Marco Pesenti Gritti 2004-03-27 16:30:42 PST
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.
Comment 17 Marco Pesenti Gritti 2004-03-31 02:33:32 PST
Created attachment 145167 [details] [diff] [review]
Attempt at a different approach

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.
Comment 18 Christopher Blizzard (:blizzard) 2004-04-01 10:50:44 PST
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.
Comment 19 Marco Pesenti Gritti 2004-04-01 11:06:52 PST
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.
Comment 20 Benjamin Smedberg [:bsmedberg] 2004-04-01 11:10:32 PST
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.
Comment 21 Christopher Blizzard (:blizzard) 2004-04-01 11:48:57 PST
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.
Comment 22 Darin Fisher 2004-04-01 11:55:39 PST
> 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.
Comment 23 Marco Pesenti Gritti 2004-04-01 12:12:32 PST
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.
Comment 24 Marco Pesenti Gritti 2004-04-01 12:19:40 PST
FWIW the DOM events problem needs to be solved for epiphany, with your interface
or without, if we dont want to regress functionalities.
Comment 25 Christopher Blizzard (:blizzard) 2004-04-01 13:13:24 PST
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.
Comment 26 Marco Pesenti Gritti 2004-04-06 03:01:56 PDT
>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
Comment 27 Christopher Blizzard (:blizzard) 2004-04-06 12:14:16 PDT
Sounds like a good plan to me.  But boy are the galeon people gonna be pissed. :)
Comment 28 Christopher Blizzard (:blizzard) 2004-04-06 12:15:13 PDT
-> marco since he's working on it.
Comment 29 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-04-06 13:06:03 PDT
(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?
Comment 30 Marco Pesenti Gritti 2004-04-06 19:10:41 PDT
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.
Comment 31 Darin Fisher 2004-04-06 21:04:13 PDT
>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 :-)
Comment 32 Marco Pesenti Gritti 2004-04-07 01:43:41 PDT
>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.
Comment 33 Christopher Blizzard (:blizzard) 2004-04-07 12:59:49 PDT
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.
Comment 34 Doug Turner (:dougt) 2004-04-07 22:02:54 PDT
what does this mean to uses that statically link the embedding widget and the
rest of the GRE goop?
Comment 35 Marco Pesenti Gritti 2004-04-08 10:33:13 PDT
Created attachment 145690 [details] [diff] [review]
use the GRE

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 ?
Comment 36 Marco Pesenti Gritti 2004-04-08 10:48:54 PDT
(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.
Comment 37 timeless 2004-04-08 20:22:59 PDT
fwiw bug 99338 vetod exposing nsPipe
Comment 38 Marco Pesenti Gritti 2004-04-09 03:55:51 PDT
Created attachment 145736 [details] [diff] [review]
use the GRE (complete)

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.
Comment 39 Alec Flett 2004-04-09 08:35:59 PDT
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?
Comment 40 timeless 2004-04-09 08:44:21 PDT
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).
Comment 41 Marco Pesenti Gritti 2004-04-09 09:51:31 PDT
(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
Comment 42 Marco Pesenti Gritti 2004-04-10 01:53:12 PDT
Created attachment 145798 [details] [diff] [review]
use the GRE (ready for review)
Comment 43 Marco Pesenti Gritti 2004-04-10 01:59:25 PDT
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.
Comment 44 Benjamin Smedberg [:bsmedberg] 2004-04-10 07:05:48 PDT
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.
Comment 45 Marco Pesenti Gritti 2004-04-11 09:32:02 PDT
> 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 46 Christopher Blizzard (:blizzard) 2004-04-12 13:37:45 PDT
Comment on attachment 145798 [details] [diff] [review]
use the GRE (ready for review)

Please upload your new patch.
Comment 47 Marco Pesenti Gritti 2004-04-12 17:13:50 PDT
Created attachment 145959 [details] [diff] [review]
port to nsEmbedString and address reviewer comments
Comment 48 Marco Pesenti Gritti 2004-04-12 17:19:13 PDT
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.
Comment 49 Marco Pesenti Gritti 2004-04-12 17:31:32 PDT
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.
Comment 50 neil@parkwaycc.co.uk 2004-04-17 03:35:35 PDT
marco pointed out that attachment 145959 [details] [diff] [review] should fix bug 225952 too.
Comment 51 Marco Pesenti Gritti 2004-04-20 12:44:00 PDT
Created attachment 146611 [details] [diff] [review]
Use the new embed string clone api -> get rid of helpers
Comment 52 Marco Pesenti Gritti 2004-04-20 12:47:52 PDT
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.
Comment 53 Benjamin Smedberg [:bsmedberg] 2004-04-22 12:07:43 PDT
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.
Comment 54 Marco Pesenti Gritti 2004-04-23 16:26:16 PDT
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.
Comment 55 Marco Pesenti Gritti 2004-04-25 15:31:52 PDT
Created attachment 147012 [details] [diff] [review]
address review comments
Comment 56 Marco Pesenti Gritti 2004-04-25 15:33:28 PDT
Created attachment 147013 [details] [diff] [review]
EmbedPrivate -w diff
Comment 57 Marco Pesenti Gritti 2004-04-25 15:44:05 PDT
> >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 ...
Comment 58 Marco Pesenti Gritti 2004-04-25 15:55:15 PDT
Created attachment 147014 [details] [diff] [review]
actually fix the last two comments
Comment 59 Marco Pesenti Gritti 2004-04-25 15:59:47 PDT
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.
Comment 60 Benjamin Smedberg [:bsmedberg] 2004-04-26 10:31:45 PDT
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.
Comment 61 Brendan Eich [:brendan] 2004-04-26 12:44:04 PDT
Why must nsProfileLock.cpp be copied?  How about just moving it to a common place?

/be
Comment 62 Benjamin Smedberg [:bsmedberg] 2004-04-26 12:55:37 PDT
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.
Comment 63 Brendan Eich [:brendan] 2004-04-26 13:50:17 PDT
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
Comment 64 Christopher Blizzard (:blizzard) 2004-05-04 10:53:23 PDT
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.
Comment 65 Doug Turner (:dougt) 2004-05-05 23:42:37 PDT
although i probably said it enough times, please do not break the static build.
Comment 66 Marco Pesenti Gritti 2004-05-06 01:53:19 PDT
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.
Comment 67 Marco Pesenti Gritti 2004-05-08 18:40:39 PDT
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.
Comment 68 Darin Fisher 2004-05-08 19:40:09 PDT
> 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).
Comment 69 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-05-08 23:41:42 PDT
Can you combine a different underlying name (different symbol names) with a
macro (same code) to make (2) go away?
Comment 70 Darin Fisher 2004-05-09 01:28:59 PDT
> 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?
Comment 71 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-05-09 11:01:20 PDT
Could you spell out exactly what the problem is, or point to something that does?
Comment 72 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-05-09 11:07:40 PDT
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.
Comment 73 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-05-09 11:17:02 PDT
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)
Comment 74 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-05-09 11:46:47 PDT
This idea has the disadvantage that it would require including nsStringAPI.h
before including any headers that forward-declared nsAString/nsACString.
Comment 75 Darin Fisher 2004-05-09 12:51:28 PDT
I filed bug 243109 regarding improvements to nsStringAPI.h/nsAString.h (such as
what dbaron has suggested here).
Comment 76 Marco Pesenti Gritti 2004-05-17 02:11:44 PDT
Created attachment 148650 [details] [diff] [review]
Fix the static build, use the new standalone profile dir service, rename Push/PopStartup
Comment 77 Marco Pesenti Gritti 2004-05-19 09:55:34 PDT
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?
Comment 78 Darin Fisher 2004-05-19 13:26:15 PDT
> 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.
Comment 79 Marco Pesenti Gritti 2004-05-19 15:20:29 PDT
> 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
Comment 80 Marco Pesenti Gritti 2004-05-26 12:20:23 PDT
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.
Comment 81 Marco Pesenti Gritti 2004-05-29 09:59:49 PDT
Created attachment 149585 [details] [diff] [review]
split out the headers cleanup and stream interface addition
Comment 82 Christian :Biesinger (don't email me, ping me on IRC) 2004-09-28 10:38:49 PDT
now that nsEmbedStream exists, and nsIWebBrowserStream, in cross-platform code -
can /embedding/browser/gtk/src/EmbedStream.* be cvs removed?
Comment 83 Marco Pesenti Gritti 2004-09-29 02:28:34 PDT
Well, until I can actually land my patch (still blocked on bug 243109)
EmbedStream is in use, so it should not be removed.
Comment 84 Mike Shaver (:shaver -- probably not reading bugmail closely) 2004-10-02 18:00:45 PDT
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?
Comment 85 Marco Pesenti Gritti 2004-11-14 02:06:03 PST
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.
Comment 86 André Pedralho 2006-03-06 07:03:03 PST
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!
Comment 87 Benjamin Smedberg [:bsmedberg] 2006-03-06 08:21:39 PST
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.
Comment 88 André Pedralho 2006-03-06 08:33:22 PST
(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.
Comment 89 Benjamin Smedberg [:bsmedberg] 2006-03-06 08:35:06 PST
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.

Note You need to log in before you can comment on or make changes to this bug.