[webkit-reviews] review denied: [Bug 89666] [Qt] Separate WebKit2 instances use the same local storage : [Attachment 148807] patch

[bugzilla-daemon at webkit.org]

Balazs Kelemen <kbalazs at webkit.org> has denied	review:
Bug 89666: [Qt] Separate WebKit2 instances use the same local storage
https://bugs.webkit.org/show_bug.cgi?id=89666

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

------- Additional Comments from Balazs Kelemen <kbalazs at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=148807&action=review


The idea is good but you need to polish the patch. Please listen to the style
bot, and use check-webkit-style next time. (I did not mentioned style issues in
the review.)

> Source/WebKit2/ChangeLog:7
> +	   Reviewed by NOBODY (OOPS!).
> +

Normally we summarize the change in a few sentences here.

> Source/WebKit2/Shared/qt/QtDefaultDataLocation.cpp:49
> +    QString& s_dataLocation = globalDataLocation();

It's no more a static, so please remove the misleading s_ prefix.

> Source/WebKit2/Shared/qt/QtDefaultDataLocation.cpp:65
> +    QString& s_dataLocation = globalDataLocation();

ditto

> Source/WebKit2/Shared/qt/QtDefaultDataLocation.h:36
> +QString& s_dataLocation();

What is this??? You don't use it anywhere.

> Source/WebKit2/Shared/qt/QtDefaultDataLocation.h:38
> +QWEBKIT_EXPORT void setDataLocation(QString dataLocation);

I would put this into global namespace. Maybe we could find a more expressing
name, I'm not sure what could it be. setPersistentDataLocation maybe. (And than
rename defaultDataLocation to persistentDataLocation and the files as well, in
a following cleanup patch.)

> Tools/WebKitTestRunner/qt/TestControllerQt.cpp:57
> +    if (getenv("DUMPRENDERTREE_TEMP") != NULL)
> +	   WebKit::setDataLocation(getenv("DUMPRENDERTREE_TEMP"));

remove != NULL, and avoid calling qgetenv twice, please.