Bug 48620

Summary: [Qt] WebKit2 needs to support historyitem, QWKHistoryItem
Product: WebKit Reporter: Juha Savolainen <juha.savolainen>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, kenneth, kling
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Implementation of QWKHistoryItem
none
Fixed patch none

Description Juha Savolainen 2010-10-29 01:40:00 PDT
The QWKHistoryItem class represents one item in the history of a QWKPage.
Comment 1 Juha Savolainen 2010-10-29 01:45:38 PDT
Created attachment 72299 [details]
Implementation of QWKHistoryItem
Comment 2 Kenneth Rohde Christiansen 2010-10-30 02:11:17 PDT
Comment on attachment 72299 [details]
Implementation of QWKHistoryItem

View in context: https://bugs.webkit.org/attachment.cgi?id=72299&action=review

> WebKit2/UIProcess/API/qt/qwkhistory.cpp:46
> +QWKHistoryItemPrivate::~QWKHistoryItemPrivate()
> +{
> +}

Why not just use the default destructor.

> WebKit2/UIProcess/API/qt/qwkhistory.cpp:59
> +QString QWKHistoryItem::title() const
> +{
> +    return WKStringCopyQString(WKBackForwardListItemCopyTitle(d->m_backForwardListItem));
> +}

I think that you are leaking here. As you are not freeing the WKString.

Do something like:

WKRetainPtr<WKString> string = WKBackForwardListItemCopyTitle(d->m_backForwardListItem);
return WKStringCopyQString(string);

When the WKRetainPtr goes out of scope, it will free the string.

> WebKit2/UIProcess/API/qt/qwkhistory_p.h:43
> +    WKBackForwardListItemRef m_backForwardListItem;

Don't you need to ref this one? ie. change this into a WKRetainPtr<WKBackForwardListItemRef>?
Comment 3 Juha Savolainen 2010-11-01 05:36:52 PDT
(In reply to comment #2)
> (From update of attachment 72299 [details])

> > WebKit2/UIProcess/API/qt/qwkhistory.cpp:59
> > +QString QWKHistoryItem::title() const
> > +{
> > +    return WKStringCopyQString(WKBackForwardListItemCopyTitle(d->m_backForwardListItem));
> > +}
> 
> I think that you are leaking here. As you are not freeing the WKString.

Also I noticed that I forgot to do checkings:
    if (!d->m_backForwardListItem)
        return QString();
Comment 4 Juha Savolainen 2010-11-01 06:16:35 PDT
Created attachment 72501 [details]
Fixed patch

Fixed patch.
Comment 5 WebKit Commit Bot 2010-11-01 18:14:15 PDT
The commit-queue encountered the following flaky tests while processing attachment 72501 [details]:

media/event-attributes.html

Please file bugs against the tests.  These tests were authored by eric.carlson@apple.com.  The commit-queue is continuing to process your patch.
Comment 6 WebKit Commit Bot 2010-11-01 18:15:29 PDT
Comment on attachment 72501 [details]
Fixed patch

Clearing flags on attachment: 72501

Committed r71089: <http://trac.webkit.org/changeset/71089>
Comment 7 WebKit Commit Bot 2010-11-01 18:15:35 PDT
All reviewed patches have been landed.  Closing bug.