[Webkit-unassigned] [Bug 62669] [Qt] Add storage tracker WK1 API for QtWebKit port

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=62669


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #99087|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #15 from Benjamin Poulain <benjamin at webkit.org>  2011-06-30 07:00:03 PST ---
(From update of attachment 99087)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.