[webkit-reviews] review denied: [Bug 49063] [Qt] WebKit2, QWKHistory needs more functionality and QWKHistoryItemPrivate should be shared object. : [Attachment 73051] More functionality to history and QWKHistoryItemPrivate has changed to shared object

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Juha Savolainen
<juha.savolainen at weego.fi>'s request for review:
Bug 49063: [Qt] WebKit2, QWKHistory needs more functionality and
QWKHistoryItemPrivate should be shared object.
https://bugs.webkit.org/show_bug.cgi?id=49063

Attachment 73051: More functionality to history and QWKHistoryItemPrivate has
changed to shared object
https://bugs.webkit.org/attachment.cgi?id=73051&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
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.