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)
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.
Reporter | ||
Comment 1•21 years ago
|
||
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.
Reporter | ||
Comment 2•21 years ago
|
||
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•21 years ago
|
||
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
Reporter | ||
Comment 4•21 years ago
|
||
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.
Reporter | ||
Comment 5•21 years ago
|
||
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.
Reporter | ||
Comment 6•21 years ago
|
||
This patch fixes clients who call one of the global widget functions before creating a widget.
Attachment #123081 -
Attachment is obsolete: true
Reporter | ||
Comment 7•21 years ago
|
||
Include recent accessibility changes and make it work with gtk 1.2 builds as well.
Attachment #123151 -
Attachment is obsolete: true
Reporter | ||
Updated•21 years ago
|
Attachment #123988 -
Flags: review?(dougt)
Comment 8•21 years ago
|
||
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?
Reporter | ||
Comment 9•21 years ago
|
||
> 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•21 years ago
|
||
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?
Reporter | ||
Comment 11•21 years ago
|
||
Just checkpointing changes. Lots of good fixes in here, but still incomplete.
Attachment #123988 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #123988 -
Flags: review?(dougt)
Comment 12•21 years ago
|
||
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
Reporter | ||
Comment 13•21 years ago
|
||
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.
Assignee | ||
Comment 14•20 years ago
|
||
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).
Assignee | ||
Comment 15•20 years ago
|
||
Hrm sorry this is the correct link to Darin mail: http://groups.google.it/groups?hl=it&lr=&ie=UTF-8&oe=UTF-8&frame=right&th=40b3bf4645da3719&seekm=c0v0sv%24p954%40ripley.netscape.com And this is the bug: http://bugzilla.mozilla.org/show_bug.cgi?id=231995
Assignee | ||
Comment 16•20 years ago
|
||
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.
Assignee | ||
Comment 17•20 years ago
|
||
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.
Reporter | ||
Comment 18•20 years ago
|
||
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.
Assignee | ||
Comment 19•20 years ago
|
||
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•20 years ago
|
||
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.
Reporter | ||
Comment 21•20 years ago
|
||
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•20 years ago
|
||
> 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.
Assignee | ||
Comment 23•20 years ago
|
||
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.
Assignee | ||
Comment 24•20 years ago
|
||
FWIW the DOM events problem needs to be solved for epiphany, with your interface or without, if we dont want to regress functionalities.
Reporter | ||
Comment 25•20 years ago
|
||
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.
Assignee | ||
Comment 26•20 years ago
|
||
>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
Reporter | ||
Comment 27•20 years ago
|
||
Sounds like a good plan to me. But boy are the galeon people gonna be pissed. :)
(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?
Assignee | ||
Comment 30•20 years ago
|
||
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•20 years ago
|
||
>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 :-)
Assignee | ||
Comment 32•20 years ago
|
||
>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.
Reporter | ||
Comment 33•20 years ago
|
||
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•20 years ago
|
||
what does this mean to uses that statically link the embedding widget and the rest of the GRE goop?
Assignee | ||
Comment 35•20 years ago
|
||
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 ?
Assignee | ||
Updated•20 years ago
|
Attachment #145167 -
Attachment is obsolete: true
Assignee | ||
Comment 36•20 years ago
|
||
(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•20 years ago
|
||
fwiw bug 99338 vetod exposing nsPipe
Assignee | ||
Comment 38•20 years ago
|
||
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.
Assignee | ||
Updated•20 years ago
|
Attachment #145690 -
Attachment is obsolete: true
Comment 39•20 years ago
|
||
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•20 years ago
|
||
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).
Assignee | ||
Comment 41•20 years ago
|
||
(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
Assignee | ||
Comment 42•20 years ago
|
||
Attachment #145736 -
Attachment is obsolete: true
Assignee | ||
Comment 43•20 years ago
|
||
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 44•20 years ago
|
||
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.
Assignee | ||
Comment 45•20 years ago
|
||
> 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.
Reporter | ||
Comment 46•20 years ago
|
||
Comment on attachment 145798 [details] [diff] [review] use the GRE (ready for review) Please upload your new patch.
Attachment #145798 -
Flags: review?(blizzard) → review-
Assignee | ||
Comment 47•20 years ago
|
||
Attachment #145798 -
Attachment is obsolete: true
Assignee | ||
Comment 48•20 years ago
|
||
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)
Assignee | ||
Comment 49•20 years ago
|
||
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.
Assignee | ||
Updated•20 years ago
|
Attachment #145959 -
Flags: superreview?(blizzard)
Comment 50•20 years ago
|
||
marco pointed out that attachment 145959 [details] [diff] [review] should fix bug 225952 too.
Blocks: 225952
Assignee | ||
Updated•20 years ago
|
Attachment #145959 -
Flags: superreview?(blizzard)
Attachment #145959 -
Flags: review?(bsmedberg)
Assignee | ||
Comment 51•20 years ago
|
||
Attachment #145959 -
Attachment is obsolete: true
Assignee | ||
Comment 52•20 years ago
|
||
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 53•20 years ago
|
||
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-
Assignee | ||
Updated•20 years ago
|
Assignee | ||
Comment 54•20 years ago
|
||
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.
Assignee | ||
Comment 55•20 years ago
|
||
Attachment #146611 -
Attachment is obsolete: true
Assignee | ||
Comment 56•20 years ago
|
||
Assignee | ||
Comment 57•20 years ago
|
||
> >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 ...
Assignee | ||
Comment 58•20 years ago
|
||
Attachment #147012 -
Attachment is obsolete: true
Assignee | ||
Comment 59•20 years ago
|
||
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 60•20 years ago
|
||
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+
Assignee | ||
Updated•20 years ago
|
Attachment #147014 -
Flags: superreview?(blizzard)
Comment 61•20 years ago
|
||
Why must nsProfileLock.cpp be copied? How about just moving it to a common place? /be
QA Contact: pavlov → brendan
Comment 62•20 years ago
|
||
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•20 years ago
|
||
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
Reporter | ||
Comment 64•20 years ago
|
||
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+
Comment 65•20 years ago
|
||
although i probably said it enough times, please do not break the static build.
Assignee | ||
Comment 66•20 years ago
|
||
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.
Assignee | ||
Comment 67•20 years ago
|
||
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•20 years ago
|
||
> 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?
Comment 70•20 years ago
|
||
> 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.
Comment 75•20 years ago
|
||
I filed bug 243109 regarding improvements to nsStringAPI.h/nsAString.h (such as what dbaron has suggested here).
Assignee | ||
Comment 76•20 years ago
|
||
Attachment #147014 -
Attachment is obsolete: true
Assignee | ||
Comment 77•20 years ago
|
||
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
Comment 78•20 years ago
|
||
> 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.
Assignee | ||
Comment 79•20 years ago
|
||
> 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
Assignee | ||
Comment 80•20 years ago
|
||
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.
Assignee | ||
Comment 81•20 years ago
|
||
Comment 82•20 years ago
|
||
now that nsEmbedStream exists, and nsIWebBrowserStream, in cross-platform code - can /embedding/browser/gtk/src/EmbedStream.* be cvs removed?
Assignee | ||
Comment 83•20 years ago
|
||
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?
Assignee | ||
Comment 85•20 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
(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•18 years ago
|
||
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.
Updated•15 years ago
|
QA Contact: brendan → gtk-widget
Updated•12 years ago
|
Product: Core → Core Graveyard
Comment 90•6 years ago
|
||
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.
Description
•