[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





--- Comment #5 from kasthuri <kasthuri.n-s at nokia.com>  2011-06-20 15:38:24 PST ---
(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.

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