[webkit-reviews] review denied: [Bug 89666] [Qt] Separate WebKit2 instances use the same local storage : [Attachment 148997] 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 148997: patch
https://bugs.webkit.org/attachment.cgi?id=148997&action=review

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


I would like to see the change first, later we can do the renaming. So I would
like you to upload a patch that contains only the behavioral change.

> Source/WebKit2/ChangeLog:5
> +	   [Qt] Added a new API in the WebKit namespace
> +	   for the WebKitTestRunner, which sets the DataDirectory.
> +	   https://bugs.webkit.org/show_bug.cgi?id=89666

There should be the title of the bug, as prepare-changelog do it for you. You
should not edit that part manually :)

> Source/WebKit2/ChangeLog:13
> +	   I have renamed defaultDataLocation to persistentDataLocation.
> +	   From now, with setPersistentDataLocation() we can set which
> +	   directory should be used by WebKitTestRunner instead of the
> +	   default one. Hence, parallelly run WKTRs don't use the same
> +	   DataLocation.

I would like to first do the behavior change, than the renamings in a following
cleanup patch. It's hard to see from the history what has changed if you do it
together.

> Source/WebKit2/Shared/qt/QtPersistentDataLocation.cpp:63
> +QWEBKIT_EXPORT void setPersistentDataLocation(QString dataLocation)

Please move it to global namespace, we don't use internal namespaces for API
(even if it's private).

> Tools/ChangeLog:6
> +	   [Qt] The WebKitTestRunner uses the default LocalStorage
> +	   directory.If ther is the DUMPRENDERTREE_TEMP environment
> +	   variables, use that, instead of the default.
> +	   https://bugs.webkit.org/show_bug.cgi?id=89666

Changelog error again. Put your description below the default prologue
generated by prepare-changelog.

> Tools/WebKitTestRunner/qt/TestControllerQt.cpp:57
> +    if (!qgetenv("DUMPRENDERTREE_TEMP").isNull())
> +	   WebKit::setPersistentDataLocation(qgetenv("DUMPRENDERTREE_TEMP"));

Don't call qgetenv twice.