Bug 62669 – [Qt] Add storage tracker WK1 API for QtWebKit port

• Component: New Bugs
• Status: RESOLVED INVALID

[kasthuri]

Add storage tracker WK1 API's for tracking local storage.  As of now looks like its implemented only in MAC and planning to add these local storage tracker API's to QtWebKit port as well.
The QWebSecurityOrigin class already has some methods for tracking database storage.  So planning to add the new local storage tracker API's to QWebSecurityOrigin class.

Also the layouttests for the storage tracker uses the storage tracker client which is used for signaling the updates to storage tracker database.  This tracker client can be added as a new class to webkit API layer or can be just added to make the layout tests pass.

[kasthuri]

Orignial bug is here https://bugs.webkit.org/show_bug.cgi?id=51878

[kasthuri]

Created attachment 97637 [details]
Initial patch

Initial version of the patch with new storage tracker api's added to QtWebKit API layer.

[Yael]

Comment on attachment 97637 [details]
Initial patch

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

I am not a reviewer, but have a few comments.

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:275
> +    Returns a list of all security origins with local storage defined.

I would omit the word "defined" here.

> Source/WebKit/qt/QtWebKit.pro:211
> +        $$PWD/WebCoreSupport/StorageTrackerClientQt.h

This file is missing.

> Source/WebKit/qt/QtWebKit.pro:214
> +        $$PWD/WebCoreSupport/StorageTrackerClientQt.cpp

This file is missing.

> Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:1178
> +       res.append(((((ret[i].scheme()).append(originDelimeter)).append(ret[i].host())).append(originDelimeter)).append(QVariant(ret[i].port()).toString()));

That hurts my eyes :)
I would use operator + since QStringBuilder is public only from Qt 4.8.

> Tools/DumpRenderTree/qt/DumpRenderTree.pro:10
> +INCLUDEPATH += ../../../Source/WebCore/storage

Don't include WebCore files outside of QtWebKit library.

> Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:52
> +    if (m_localStorageTracker)

This check is not needed.

> Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:81
> +    m_localStorageTracker = 0;

This should also delete storage that might have been created by the test.

[kasthuri]

I think I have few style issues as well which I didn't see with the style checker script with my patch.  Patch v2 with updates coming soon.

[kasthuri]

(In reply to comment #3)
> (From update of attachment 97637 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97637&action=review
> 
> I am not a reviewer, but have a few comments.
> 
> > Source/WebKit/qt/Api/qwebsecurityorigin.cpp:275
> > +    Returns a list of all security origins with local storage defined.
> 
> I would omit the word "defined" here.
> 
Changed.
> > Source/WebKit/qt/QtWebKit.pro:211
> > +        $$PWD/WebCoreSupport/StorageTrackerClientQt.h
> 
> This file is missing.
> 
Added the file in the new patch that I'm working on.
> > Source/WebKit/qt/QtWebKit.pro:214
> > +        $$PWD/WebCoreSupport/StorageTrackerClientQt.cpp
> 
> This file is missing.
> 
Added the file in the new patch that I'm working on.
> > Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:1178
> > +       res.append(((((ret[i].scheme()).append(originDelimeter)).append(ret[i].host())).append(originDelimeter)).append(QVariant(ret[i].port()).toString()));
> 
> That hurts my eyes :)
> I would use operator + since QStringBuilder is public only from Qt 4.8.
I tried operator + initially, but then it gave compilation error saying its being deprecated.  So I had to use append(), probably I can split this into multiple lines for better readability.
> 
> > Tools/DumpRenderTree/qt/DumpRenderTree.pro:10
> > +INCLUDEPATH += ../../../Source/WebCore/storage
> 
> Don't include WebCore files outside of QtWebKit library.
> 
I'm not including any WebCore files directly.  However since the stroage tracker client header that I'm using here depends on a webcore interface I had to use this.  To get away from this I need to introduce a new layer which talks to the storage tracker client maybe as new Qt API.  Please let me know what you think about this.
> > Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:52
> > +    if (m_localStorageTracker)
> 
> This check is not needed.
> 
This check is done in destructor to avoid deleting m_localStorageTracker if its not initialized.  Since this is created only if the obserStorageTrackerNotifications() method is called, I did this check.
> > Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:81
> > +    m_localStorageTracker = 0;
> 
> This should also delete storage that might have been created by the test.
I just initialized it here in reset() method since its only called from LayoutTestController constructor and deleted it in the destructor.

[kasthuri]

Created attachment 97874 [details]
Changes version 2

Patch with few review comments incorporated.  Just uploading to see whether the style bots are accepting the patch.  Working on patch which satisfies all review comments.

[kasthuri]

Created attachment 98428 [details]
patch v3

Patch with review comments incorporated.

[kasthuri]

Attaching patch with review comments incorporated.  The storageTrackerClient is now called through a public method from QWebPage and the corresponding notification is raised through signal in QWebPage.  The access level can be changed from public to private if we don't want to expose this to clients.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 98428 [details]
patch v3

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

> Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:1169
> +        res.append(ret[i].scheme() + originDelimeter + ret[i].host() + originDelimeter + QString(ret[i].port()));

Maybe use a formating QString and the QString::arg() method can help the construction of this string.

Somethin like: res.append(QString::fromLatin1("%1_%2_%3").arg(ret[i].scheme(), ret[i].host(), ret[i].port())); // untested

I think you could even create one QString with the format and reuse it inside the loop.

[Keith Kyzivat]

NOTE: I am not a reviewer, but had some comments.

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

Wrap in #if ENABLE(DOM_STORAGE) or make sure that m_localStorageTracker is NULL if !DOM_STORAGE.

> Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:1163
> +    QVariantList res;

This needs a more descriptive name than 'res'.

> Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:1164
> +    QString originDelimeter = QString(QLatin1Char('_'));

Does this really need to be in a var?

> Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:1165
> +    QList<QWebSecurityOrigin> ret;

Needs a more descriptive name - you used webOrigins in an above function - I'd use that.

> Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:1167
> +    ret = QWebSecurityOrigin::allLocalStorageOrigins();

As Caio mentioned - you could define format here something like this:
    QString localStorageOriginFormat("%1_%2_%3");

>> Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:1169
>> +        res.append(ret[i].scheme() + originDelimeter + ret[i].host() + originDelimeter + QString(ret[i].port()));
> 
> Maybe use a formating QString and the QString::arg() method can help the construction of this string.
> 
> Somethin like: res.append(QString::fromLatin1("%1_%2_%3").arg(ret[i].scheme(), ret[i].host(), ret[i].port())); // untested
> 
> I think you could even create one QString with the format and reuse it inside the loop.

I would agree.  Formatting string as shown above would be a more clean way to do this, and would be more efficient.

> LayoutTests/platform/qt/Skipped:147
>  # StorageTracker is not enabled.

Update comment

[Alexis Menard (darktears)]

Comment on attachment 98428 [details]
patch v3

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.

[kasthuri]

Created attachment 99087 [details]
patch v4

Patch with review comments incorporated.

[kasthuri]

(In reply to comment #11)
> (From update of attachment 98428 [details])
> 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.

Changed.
> 
> > Source/WebKit/qt/Api/qwebpage.cpp:3955
> >  
> 
> Where is doc?
Added the doc.  Didn't add doc previously as I was not sure whether to expose this as a public API or do it internally in qwebpage and let the use just look for signal if interested. 
> 
> > 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?

My thinking is if the user no longer is interested, then he can disconnect from the signal.  Again as quoted above, if the observeStorageTrackerNotifications() call is handled internally and not exposed as a API, then all the user has to do is just connect to localStorageModified() signal.
> 
> > 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?
I used this to prevent duplicate connections from happening, if in case the user calls the api twice.
> 
> > 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.
> 
Done.
> > Source/WebKit/qt/Api/qwebsecurityorigin.cpp:315
> > +    Returns the disk usage for a particular security origin.
> 
> Ditto.
>
Changed. 
> > Source/WebKit/qt/Api/qwebsecurityorigin.cpp:327
> > +    Syncs local storage dbs.
> 
> You have to use full english words in the public documentation.
> 
Changed.
> > Source/WebKit/qt/WebCoreSupport/StorageTrackerClientQt.h:32
> > +#include <QtCore/qstring.h>
> 
> Why QObject and the next line qstring.h. Make it consistent.
> 
Changed.
> > 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.
There is a hardcoded size in the testcase which I'm checking further.  Will update the skipped list or create a new bug accordingly.

[kasthuri]

sorry..forgot to update that the patch takes care of Cmarcelo and Keith's comments as well.

[Benjamin Poulain]

Comment on attachment 99087 [details]
patch v4

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.