Bug 49063

Summary: [Qt] WebKit2, QWKHistory needs more functionality and QWKHistoryItemPrivate should be shared object.
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
More functionality to history and QWKHistoryItemPrivate has changed to shared object
kling: review-
fixed patch none

Description Juha Savolainen 2010-11-05 03:51:05 PDT
History needs at least these methods:
- QWKHistoryItem currentItem() const
- QWKHistoryItem backItem() const
- QWKHistoryItem forwardItem() const
- QWKHistoryItem itemAtIndex(int index) const
- QList<QWKHistoryItem> backItems(int maxItems) const
- QList<QWKHistoryItem> forwardItems(int maxItems) const

Also QWKHistoryItemPrivate should be shared object, because
we cannot delete the QWKHistoryItemPrivate pointer in destructor of QWKHistoryItem,
there may have other history instances which share the same private implementation.
Comment 1 Juha Savolainen 2010-11-05 03:57:21 PDT
Created attachment 73051 [details]
More functionality to history and QWKHistoryItemPrivate has changed to shared object
Comment 2 Andreas Kling 2010-11-07 19:06:16 PST
Comment on attachment 73051 [details]
More functionality to history and QWKHistoryItemPrivate has changed to shared object

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

> WebKit2/UIProcess/API/qt/qwkhistory.cpp:39
>  #include <QUrl>
> +#include <QSharedData>
>  #include <WebKit2/WKRetainPtr.h>
> +#include <WebKit2/WKArray.h>

Include files should be in alphabetical order.

> WebKit2/UIProcess/API/qt/qwkhistory.cpp:52
> +QWKHistoryItem::QWKHistoryItem(const QWKHistoryItem &other)

Ampersand (&) placement, should be immediately after QWKHistoryItem.

> WebKit2/UIProcess/API/qt/qwkhistory.cpp:57
> +QWKHistoryItem& QWKHistoryItem::QWKHistoryItem::operator=(const QWKHistoryItem &other) 

Ditto.

> WebKit2/UIProcess/API/qt/qwkhistory.cpp:139
> +    QWKHistoryItem item;
> +    item.d = new QWKHistoryItemPrivate(itemRef.get());

This pattern is ugly, we should have the QWKHistoryItem constructor initialize its d-pointer instead.

> WebKit2/UIProcess/API/qt/qwkhistory.cpp:172
> +    for (int i = 0; i < size; i++) {

Use prefix increment (++i)

> WebKit2/UIProcess/API/qt/qwkhistory.cpp:187
> +    for (int i = 0; i < size; i++) {

Use prefix increment (++i)

> WebKit2/UIProcess/API/qt/qwkhistory.h:44
> +    QWKHistoryItem(const QWKHistoryItem &other);
> +    QWKHistoryItem &operator=(const QWKHistoryItem &other);

Ampersand (&) placement, should be immediately after QWKHistoryItem.

> WebKit2/UIProcess/API/qt/qwkhistory.h:69
> +    QWKHistoryItem itemAtIndex(int index) const;

Should be called "itemAt" to match the QWebHistory API.

> WebKit2/UIProcess/API/qt/qwkhistory_p.h:32
> +#include <QSharedData>

Include files should be in alphabetical order.

> WebKit2/UIProcess/API/qt/qwkhistory_p.h:45
> +    

Adding unnecessary whitespace.
Comment 3 Juha Savolainen 2010-11-08 02:29:53 PST
Created attachment 73228 [details]
fixed patch
Comment 4 Andreas Kling 2010-11-08 03:23:48 PST
Comment on attachment 73228 [details]
fixed patch

r=me
Comment 5 WebKit Commit Bot 2010-11-08 18:48:39 PST
Comment on attachment 73228 [details]
fixed patch

Clearing flags on attachment: 73228

Committed r71596: <http://trac.webkit.org/changeset/71596>
Comment 6 WebKit Commit Bot 2010-11-08 18:48:44 PST
All reviewed patches have been landed.  Closing bug.