[webkit-reviews] review denied: [Bug 62669] [Qt] Add storage tracker WK1 API for QtWebKit port : [Attachment 98428] patch v3

[bugzilla-daemon at webkit.org]

Alexis Menard <alexis.menard at openbossa.org> has denied kasthuri
<kasthuri.n-s at nokia.com>'s request for review:
Bug 62669: [Qt] Add storage tracker WK1 API for QtWebKit port
https://bugs.webkit.org/show_bug.cgi?id=62669

Attachment 98428: patch v3
https://bugs.webkit.org/attachment.cgi?id=98428&action=review

------- Additional Comments from Alexis Menard <alexis.menard at openbossa.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=98428&action=review

> Source/WebKit/qt/Api/qwebpage.cpp:402
> +    delete m_localStorageTracker;

Like previous comment make sure it is 0 or wrap the definition variable in #if
ENABLE(DOM_STORAGE) because in the case !ENABLE(DOM_STORAGE) the pointer is not
initialized.

> Source/WebKit/qt/Api/qwebpage.cpp:3955
>  

Where is doc?

> Source/WebKit/qt/Api/qwebpage.cpp:3956
> +void QWebPage::observeStorageTrackerNotifications()

And if I don't want to observe the notification anymore? Isn't it better to
have an API with a bool on/off?

> Source/WebKit/qt/Api/qwebpage.cpp:3959
> +    QObject::connect(d->m_localStorageTracker, SIGNAL(originModified(const
QString&)), this, SIGNAL(localStorageModified(const QString&)),
Qt::UniqueConnection);

Perhaps it is justified but could you explain me why UniqueConnection?

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:304
> +    Removes a particular security origin which has local storage.

Per doc policy you need to make a comment on the parameter.

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:315
> +    Returns the disk usage for a particular security origin.

Ditto.

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:327
> +    Syncs local storage dbs.

You have to use full english words in the public documentation.

> Source/WebKit/qt/WebCoreSupport/StorageTrackerClientQt.h:32
> +#include <QtCore/qstring.h>

Why QObject and the next line qstring.h. Make it consistent.

> LayoutTests/platform/qt/Skipped:148
> +storage/domstorage/localstorage/storagetracker/storage-tracker-7-usage.html

it doesn't pass? Perhaps you should open a bug report about that one.