259038 - Embedding component for Qt

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Zack Rusin]

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:

Comment 1

[Zack Rusin]

Attachment: embedding/browser/qt directory

Comment 2

[Adam Lock]

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...

Comment 3

[Zack Rusin]

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

[Christian :Biesinger (don't email me, ping me on IRC)]

> 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

Comment 5

[Zack Rusin]

(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. 

Comment 6

[Zack Rusin]

Attachment: embedding/browser/qt dir

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

>>     // return FALSE to this function to mark this event as not 
>>     // consumed... 

ah sorry. I read "to" as "from" in that sentence.

Comment 8

[Zack Rusin]

Attachment: embedding/browser/qt dir

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.

Comment 9

[Zack Rusin]

Attachment: embedding/browser/qt directory

Sets profile paths correctly to stop all the assertions in the TestQGeckoEmbed.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

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)

Comment 11

[Zack Rusin]

(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

[Christian :Biesinger (don't email me, ping me on IRC)]

> 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.

Comment 13

[Zack Rusin]

Committed the code.