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

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.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 99087: patch v4
https://bugs.webkit.org/attachment.cgi?id=99087&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=99087&action=review

Quick review

r- Because
-I don't see a strong use case for the new APIs of QWebPage.
-There is no API tests
-The API of QWebSecurityOrigin is not future proof at all.

New APIs is always a dangerous/difficult problem, you should make sure it is
done right because there is no coming back once an API is public. Because of
that, you must:
-have valid use cases for every new public function
-have test cases (API tests) for those uses cases
-add as few functions as possible, and ensure you have enough flexibility for
the places where you think the needs will evolve.
-ensure the API is consistent with existing patterns and APIs (in this case,
QWebDatabase).

I think you should remove the APIs for QWebPage unless you have a strong use
case. And you should have a better API for QWebSecurityOrigin, closer to what
exists for QWebDatabase.

> Source/WebKit/qt/Api/qwebpage.cpp:3960
> +    This function is called when a user agent wants to listen for local
storage change notificatins.

"notificatins"

By using passive form, you are implying this function is called for you by the
framework (in which case it would typically be virtual.)

> Source/WebKit/qt/Api/qwebpage.h:410
> +    void localStorageModified(const QString& originIdentifier);
> +

This signal does not have any documentation.

> Source/WebKit/qt/Api/qwebpage_p.h:217
> +    WebCore::StorageTrackerClientQt* m_localStorageTracker;

You should use a smart pointer instead of handling memory manually.

This should also probably be allocated lazily.

> Source/WebKit/qt/Api/qwebsecurityorigin.h:46
> +    static long long localStorageDiskUsageForOrigin(const QString&
originIdentifier);

Using long long is a really bad idea since its definition depends on the
compile, and it is signed here.
You should use quint64.