Closed
Bug 259038
Opened 20 years ago
Closed 20 years ago
Embedding component for Qt
Categories
(Core Graveyard :: Embedding: APIs, defect)
Core Graveyard
Embedding: APIs
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: zack, Assigned: adamlock)
References
Details
Attachments
(1 file, 3 obsolete files)
User-Agent: Mozilla/5.0 (X11; U; Linux ppc; en-US; rv:1.7) Gecko/20040823 Firefox/0.9.3 Build Identifier: Mozilla/5.0 (X11; U; Linux ppc; en-US; rv:1.7) Gecko/20040823 Firefox/0.9.3 This bug includes the bz2 with the embedding/browser/qt directory. The patch allows to embed Gecko in Qt apps (go figure :) ). Reproducible: Always Steps to Reproduce:
Reporter | ||
Comment 1•20 years ago
|
||
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•20 years ago
|
Summary: Embedding component for Qt → Embedding component for Qt
This needs someone familiar with QT to validate if it looks okay. Out of interest, does this wrapper offer a similar API to the Konq part? It would be very nice if it did for porting existing code and whatnot. The next step after that would require an enterprising Mac developer to write an Objective C Cocoa framework and a patcher for Safari...
Reporter | ||
Comment 3•20 years ago
|
||
Well, if I and Lars did something wrong on the Qt side it would mean KDE and Trolltech have some serious problems ;) It'd be nice if someone familiar with Mozilla embedding took a look at it. I wrote it in about a day so it can't be flawless. The API simulating our own KHTMLPart is in a KPart code which will be available in our KDE CVS. Making KMozillaPart more/less SC (methods for fetching DOM::Document of course can't be SC) with KHTMLPart is about a day of work.
Comment 4•20 years ago
|
||
> grep -r WithConversion ~/mozilla/embedding/browser/qt/
/home/chb/mozilla/embedding/browser/qt/src/EmbedWindow.cpp:
ctagname.AssignWithConversion(tagname);
/home/chb/mozilla/embedding/browser/qt/src/EmbedWindow.cpp:
attr.AssignWithConversion("href");
/home/chb/mozilla/embedding/browser/qt/src/EmbedWindow.cpp:
attr.AssignWithConversion("src");
/home/chb/mozilla/embedding/browser/qt/src/EmbedProgress.cpp:
tmpString.AssignWithConversion(uriString);
Please don't use these functions, they are obsolete and will be removed "soon".
I know that a lot of mozilla code still uses it, but I'm working on that.
if you have a literal, use .AssignLiteral("..."), if it's any ascii string
.AssignASCII.
URIs in Mozilla are UTF-8, use CopyUTF8toUTF16(uriString, tmpString) here. is
there a reason for using a char** in RequestToURIString's signature, instead of
nsACString&? string classes support string sharing, so you could avoid a copy.
hmm, tmpString seems to be only used in an #if 0...
virtual ~EmbedProgress();
virtual dtors are often not needed and cost codesize, avoid them if you can
(yes, mozilla code often uses them unnecessarily...)
EmbedProgress.cpp:
if ((aStateFlags & STATE_IS_NETWORK) &&
(aStateFlags & STATE_START))
{
qDebug("net start");
emit mOwner->netStart();
}
// qDebug("progress: aStateFlags = %x", aStateFlags);
if ((aStateFlags & (STATE_START|STATE_IS_NETWORK)) ==
(STATE_START|STATE_IS_NETWORK)) {
// qDebug("progress: --start");
emit mOwner->netStart();
}
How is this second if different from the first one?
QString message = QString::fromUcs2(aMessage);
PRUnichar strings are UTF-16, not UCS-2...
Makefile.in:
ifdef MOZ_ENABLE_QT
EXTRA_DSO_LDOPTS = \
$(MOZ_COMPONENT_LIBS) \
$(NULL)
endif
why the ifdef?
qgeckoembed.cpp
window = new EmbedWindow();
windowGuard = NS_STATIC_CAST(nsIWebBrowserChrome *, window);
seems to me like window could be an nsRefPtr<EmbedWindow>, to avoid the need for
windowGuard
since you ignore the result of all Init methods, why not make them constructor
arguments and remove Init?
// We set this flag here instead of on success. If it failed we
// don't want to keep trying and leaking window creator objects.
you wouldn't be leaking them, the nsCOMPtr would destroy them (since objects
start with a refcount of zero)
EmbedWindowCreator *creator = new EmbedWindowCreator();
nsCOMPtr<nsIWindowCreator> windowCreator;
windowCreator = NS_STATIC_CAST(nsIWindowCreator *, creator);
why not:
nsCOMPtr<nsIWindowCreator> windowCreator = new EmbedWindowCreator();
nsCOMPtr<nsIURIContentListener> uriListener;
uriListener = do_QueryInterface(contentListenerGuard);
webBrowser->SetParentURIContentListener(uriListener);
why not avoid the cast and do SetParentURIContentListener(contentListener); ?
nsIWebBrowser *webBrowser;
d->window->GetWebBrowser(&webBrowser);
it seems like you are leaking the webBrowser here. also, this function returns
an addrefed object; since this ensures a leak once someone assigns it into a
comptr, please make it return already_AddRefed(nsIDOMDocument)
you are also returning garbage if you have no window or if getdocument fails
QString QGeckoEmbed::url() const
{
nsIURI *uri;
d->navigation->GetCurrentURI(&uri);
nsCString acstring;
uri->GetSpec(acstring);
this leaks uri; also, nsCAutoString is possibly more efficient (in cases where
GetSpec doesn't take advantage of string sharing)
QString QGeckoEmbed::resolvedUrl(const QString &relativepath) const
{
nsIURI *uri;
d->navigation->GetCurrentURI(&uri);
nsCString rel;
rel.Assign(relativepath.utf8().data());
nsCString resolved;
uri->Resolve(rel, resolved);
this leaks uri. rel should be nsCAutoString, which contains a 64-byte buffer to
avoid heap allocations. same for resolved. I assume the .data() result does not
need to be freed?
EmbedEventListener.cpp:
// return FALSE to this function to mark this event as not
// consumed...
don't think this comment is true... this function returns an nsresult, which
means: FALSE == NS_OK. note: I may be wrong, I don't know the dom event handling
stuff
hm... your not implementing global history means, I think, that all uris will
show as unvisited in the browser...
EmbedStream.cpp:
#include <nsIPipe.h>
usually mozilla headers are included as "nsIPipe.h".
nsCOMPtr<nsIInputStream> bufInStream;
nsCOMPtr<nsIOutputStream> bufOutStream;
rv = NS_NewPipe(getter_AddRefs(bufInStream),
getter_AddRefs(bufOutStream));
if (NS_FAILED(rv)) return rv;
mInputStream = bufInStream;
mOutputStream = bufOutStream;
Hm... why not directly assign into mInputStream (i.e. pass
getter_AddRefs(mInputStream) to NS_NewPipe, same for output)? do you want to
avoid overwriting the streams when newpipe fails?
NS_STATIC_CAST(nsIInputStream *, this),
is the explicit cast needed?
that said... aren't you using nsWebBrowser anyway, and could use its
nsIWebBrowserStream methods? (I don't know much about our embedding apis...)
EmbedWindow.cpp:
nsCString ctagname;
ctagname.AssignWithConversion(tagname);
if (!strcasecmp(ctagname.get(), "a")) {
if you used nsString::LowerCaseEqualsLiteral, you could avoid the conversion
qgeckoglobals.cpp
QGeckoGlobals::setProfilePath(const char *aDir, const char *aName)
you don't really need to bother with nsMemory, as you are only handling this
variable internally
Reporter | ||
Comment 5•20 years ago
|
||
(In reply to comment #4) > > grep -r WithConversion ~/mozilla/embedding/browser/qt/ > /home/chb/mozilla/embedding/browser/qt/src/EmbedWindow.cpp: > ctagname.AssignWithConversion(tagname); > /home/chb/mozilla/embedding/browser/qt/src/EmbedWindow.cpp: > attr.AssignWithConversion("href"); > /home/chb/mozilla/embedding/browser/qt/src/EmbedWindow.cpp: > attr.AssignWithConversion("src"); > /home/chb/mozilla/embedding/browser/qt/src/EmbedProgress.cpp: > tmpString.AssignWithConversion(uriString); > > > Please don't use these functions, they are obsolete and will be removed "soon". > I know that a lot of mozilla code still uses it, but I'm working on that. k, fixed. > there a reason for using a char** in RequestToURIString's signature, instead > of nsACString&? Nope. Not a good one. Fixed that. > virtual ~EmbedProgress(); > > virtual dtors are often not needed and cost codesize, avoid them if you can > (yes, mozilla code often uses them unnecessarily...) This is something from other code. I assumed that each of the interfaces does already have a virtual destructor. Besides to be honest this the last of my worries right now, with paint event storms one more virtual in a class with full of them is not really a problem. Thanks for noticing though it made me clean it up now instead of later. > EmbedProgress.cpp: > if ((aStateFlags & STATE_IS_NETWORK) && > (aStateFlags & STATE_START)) > { > qDebug("net start"); > emit mOwner->netStart(); > } > > // qDebug("progress: aStateFlags = %x", aStateFlags); > if ((aStateFlags & (STATE_START|STATE_IS_NETWORK)) == > (STATE_START|STATE_IS_NETWORK)) { > // qDebug("progress: --start"); > emit mOwner->netStart(); > } > > How is this second if different from the first one? heh, I'll have to ask Lars once he's back ;) fixed. > QString message = QString::fromUcs2(aMessage); > PRUnichar strings are UTF-16, not UCS-2... It makes no difference for this sucka. > Makefile.in: > ifdef MOZ_ENABLE_QT > EXTRA_DSO_LDOPTS = \ > $(MOZ_COMPONENT_LIBS) \ > $(NULL) > endif > > why the ifdef? Fixed. > qgeckoembed.cpp > > window = new EmbedWindow(); > windowGuard = NS_STATIC_CAST(nsIWebBrowserChrome *, window); > seems to me like window could be an nsRefPtr<EmbedWindow>, to avoid the > need for windowGuard > > since you ignore the result of all Init methods, why not make them > constructor arguments and remove Init? I was contemplating adding checks later but now that I was looking over the code I decided that it indeed can be replaced so I did that. > // We set this flag here instead of on success. If it failed we > // don't want to keep trying and leaking window creator objects. > you wouldn't be leaking them, the nsCOMPtr would destroy them (since objects > start with a refcount of zero) Good point. > > EmbedWindowCreator *creator = new EmbedWindowCreator(); > nsCOMPtr<nsIWindowCreator> windowCreator; > windowCreator = NS_STATIC_CAST(nsIWindowCreator *, creator); > why not: > nsCOMPtr<nsIWindowCreator> windowCreator = new EmbedWindowCreator(); Fixed. > nsCOMPtr<nsIURIContentListener> uriListener; > uriListener = do_QueryInterface(contentListenerGuard); > webBrowser->SetParentURIContentListener(uriListener); > > why not avoid the cast and do SetParentURIContentListener(contentListener); ? No reason. fixed. > nsIWebBrowser *webBrowser; > > d->window->GetWebBrowser(&webBrowser); > > it seems like you are leaking the webBrowser here. also, this function returns > an addrefed object; since this ensures a leak once someone assigns it into a > comptr, please make it return already_AddRefed(nsIDOMDocument) Nah, I really don't want to be doing that. I fixed what I could in that method, but there's no way that I'll be willingly exposing nsCOMPtr to Qt/KDE programs. I need to do something else. The reference counting schema in Mozilla is so vastly different than anything in our api that it will just cause mass confusion. I might decide to keep the nsidomdocument reference in the dptr in the qgeckoembed and switch it transparently when documents change. I'm not sure yet. I have to see use cases first. > QString QGeckoEmbed::url() const > { > nsIURI *uri; > d->navigation->GetCurrentURI(&uri); > nsCString acstring; > uri->GetSpec(acstring); > this leaks uri; also, nsCAutoString is possibly more efficient (in > cases where GetSpec doesn't take advantage of string sharing) Yeah, I read the strings document. I must say that I never seen that one malloc making a difference so I just didn't bother. I switched it now. > QString QGeckoEmbed::resolvedUrl(const QString &relativepath) const > { > nsIURI *uri; > d->navigation->GetCurrentURI(&uri); > nsCString rel; > rel.Assign(relativepath.utf8().data()); > nsCString resolved; > uri->Resolve(rel, resolved); > > this leaks uri. Fixed. > I assume the .data() result does not need to be freed? Correct. > EmbedEventListener.cpp: > // return FALSE to this function to mark this event as not > // consumed... > don't think this comment is true... this function returns an nsresult, which > means: FALSE == NS_OK. note: I may be wrong, I don't know the dom event > handling stuff It is true. It referrs to the QGeckoEmbed functions which follow the Qt event handling semantics. > hm... your not implementing global history means, I think, that all uris > will show as unvisited in the browser... For embedding component it's ok. I'll sit down on global history sometime this week. > EmbedStream.cpp: > #include <nsIPipe.h> > > usually mozilla headers are included as "nsIPipe.h". There's lots and lots of them. I don't feel like sed'ing today. I'll do that later. > nsCOMPtr<nsIInputStream> bufInStream; > nsCOMPtr<nsIOutputStream> bufOutStream; > > rv = NS_NewPipe(getter_AddRefs(bufInStream), > getter_AddRefs(bufOutStream)); > > if (NS_FAILED(rv)) return rv; > > mInputStream = bufInStream; > mOutputStream = bufOutStream; > > Hm... why not directly assign into mInputStream (i.e. pass > getter_AddRefs(mInputStream) to NS_NewPipe, same for output)? do you want to > avoid overwriting the streams when newpipe fails? Yeah, that was the idea. > NS_STATIC_CAST(nsIInputStream *, this), > is the explicit cast needed? hmm, doesn't seem to be. > that said... aren't you using nsWebBrowser anyway, and could use its > nsIWebBrowserStream methods? (I don't know much about our embedding apis...) > > EmbedWindow.cpp: > nsCString ctagname; > ctagname.AssignWithConversion(tagname); > if (!strcasecmp(ctagname.get(), "a")) { > > if you used nsString::LowerCaseEqualsLiteral, you could avoid the conversion Great tip thanks. > qgeckoglobals.cpp > > QGeckoGlobals::setProfilePath(const char *aDir, const char *aName) > > you don't really need to bother with nsMemory, as you are only handling this > variable internally At some point I want to let devs pick the profile paths and names it will come in handy then.
Reporter | ||
Comment 6•20 years ago
|
||
Attachment #158690 -
Attachment is obsolete: true
Comment 7•20 years ago
|
||
>> // return FALSE to this function to mark this event as not
>> // consumed...
ah sorry. I read "to" as "from" in that sentence.
Reporter | ||
Comment 8•20 years ago
|
||
EmbedWindow can't have Init in the constructor because then we don't yet have the nsCOMPtr with a addref for it and Init method AddRef's/Release's it causing it to be deleted before we're even finished with constructing. Fixing that. Plus disabling our history code for now. BTW, is there any way to make nsDirectoryService spit out the files it couldn't locate. I'm getting: WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file nsGlobalHistory.cpp, line 2514 WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(OpenDB())) failed, file nsGlobalHistory.cpp, line 1062 which are a little irritating.
Reporter | ||
Updated•20 years ago
|
Attachment #159366 -
Attachment is obsolete: true
Reporter | ||
Comment 9•20 years ago
|
||
Sets profile paths correctly to stop all the assertions in the TestQGeckoEmbed.
Attachment #159452 -
Attachment is obsolete: true
Comment 10•20 years ago
|
||
Comment on attachment 160168 [details] embedding/browser/qt directory hmm, where is the implementation of QGeckoEmbed::startURIOpen? I can only find its decl and a call to it... so you couldn't use nsIWebBrowserStream? also, what about my suggestion: > seems to me like window could be an nsRefPtr<EmbedWindow>, to avoid the > need for windowGuard ? nsIDOMDocument* QGeckoEmbed::document() const I still think this is asking for a leak of the document, but ok. sPrefs seems unused. since nsIPref is deprecated, it would be great if you would remove this variable r=me with these changes (although I don't know the embedding parts very well)
Attachment #160168 -
Flags: review+
Reporter | ||
Comment 11•20 years ago
|
||
(In reply to comment #10) > (From update of attachment 160168 [details]) > hmm, where is the implementation of QGeckoEmbed::startURIOpen? I can only find > its decl and a call to it... It's a Qt signal. Implemented in the moc file. > so you couldn't use nsIWebBrowserStream? I don't know. What would that give me? > also, what about my suggestion: > > seems to me like window could be an nsRefPtr<EmbedWindow>, to avoid the > > need for windowGuard > ? Bad suggestion :) During EmbedWindow contruction we can't yet hold a nsCOMPtr to the window, and since I call setitemitem on a nsIDocShellTreeItem which is held in a nsCOMPtr window is deleted before the nsCOMPtr could ever grab ahold of it in qgeckoembed. > > nsIDOMDocument* > QGeckoEmbed::document() const > > I still think this is asking for a leak of the document, but ok. Yeah, I'm absolutely fine with that. Like I said this method is probably going to change give or take billion times in the next weeks. > sPrefs seems unused. since nsIPref is deprecated, it would be great if you > would remove this variable Sure, I can do that. Thanks for the comments.
Comment 12•20 years ago
|
||
> I don't know. What would that give me? well, you wouldn't need all the code in EmbedStream. see: http://lxr.mozilla.org/seamonkey/source/embedding/browser/webBrowser/nsIWebBrowserStream.idl it can be used by qi'ing the nsWebBrowser. (added in bug 205425 comment 38) hmm, I think this also obsoletes gtk's EmbedStream. added a comment in that bug to that effect.
Reporter | ||
Comment 13•20 years ago
|
||
Committed the code.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•