Bug 89666

Created attachment 148807 [details] patch

Attachment 148807 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Tools/WebKitTestRunner/qt/TestControllerQt.cpp:41: Alphabetical sorting problem. [build/include_order] [4] Tools/WebKitTestRunner/qt/TestControllerQt.cpp:41: Alphabetical sorting problem. [build/include_order] [4] Tools/WebKitTestRunner/qt/TestControllerQt.cpp:56: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/WebKitTestRunner/qt/TestControllerQt.cpp:56: Use 0 instead of NULL. [readability/null] [5] Tools/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Source/WebKit2/Shared/qt/QtDefaultDataLocation.h:38: The parameter name "dataLocation" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/Shared/qt/QtDefaultDataLocation.cpp:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 8 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 148807 [details] patch 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.

> > > Source/WebKit2/Shared/qt/QtDefaultDataLocation.h:38 > > +QWEBKIT_EXPORT void setDataLocation(QString dataLocation); > And this needs a comment saying it is private api for WebKitTestRunner.

Created attachment 148997 [details] patch Thanks your advice. I fixed the patch.

Comment on attachment 148997 [details] patch 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.

Created attachment 149754 [details] patch

Comment on attachment 149754 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=149754&action=review Looks good to me. > Source/WebKit2/Shared/qt/QtDefaultDataLocation.cpp:65 > +// It is private api for WebKitTestRunner API > Tools/WebKitTestRunner/qt/TestControllerQt.cpp:61 > + QString dumprendertreetemp = qgetenv("DUMPRENDERTREE_TEMP"); > + if (!dumprendertreetemp.isNull()) > + setPersistentDataLocation(dumprendertreetemp); dumpRenderTreeTemp

Comment on attachment 149754 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=149754&action=review I do think that the idea is good, but I don't think the implementation is correct. This is not an issue that is specific to the Qt port, so support for this should be present for all ports using WTR. And that appears to be the case, given that WTR's TestController.cpp already has code for reading the DUMPRENDERTREE_TEMP variable and then uses WKContextSetDatabaseDirectory to direct the databases to the location pointed to by the environment variable. I suspect that what's happening in our case is that we end up running our initialization code afterwards and over-write the previously set database directory - for example the PlatformWebView is created after the call to the C API. Consequently I think the cleanest fix would involve us correctly taking the existing cross-platform code path. > Source/WebKit2/ChangeLog:4 > + [Qt] Separate WebKit2 instances use the same local storage > + https://bugs.webkit.org/show_bug.cgi?id=89666 Can you elaborate a bit more (either in the changelog or bugzilla) what the _symptom_ is that you're trying to fix? Is this an issue when using NWRT with multiple WTR instances writing to the same directory? > Tools/ChangeLog:9 > + directory.If ther is the DUMPRENDERTREE_TEMP environment Space between punctuation and the If. And "ther" -> "there" :)

Created attachment 151914 [details] patch for ews

Comment on attachment 151914 [details] patch for ews Attachment 151914 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13198745

Comment on attachment 151914 [details] patch for ews Attachment 151914 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13208668

Created attachment 154367 [details] patch for ews

Comment on attachment 154367 [details] patch for ews View in context: https://bugs.webkit.org/attachment.cgi?id=154367&action=review LGTM with the need of some fix. > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:52 > + notImplemented(); > + return ""; return String() > Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:51 > + // FIXME: Implement > + return WTF::String(); I don't think you need WTF:: here. > Source/WebKit2/UIProcess/mac/WebContextMac.mm:-91 > + parameters.parentProcessName = [[NSProcessInfo processInfo] processName]; > > NSURLCache *urlCache = [NSURLCache sharedURLCache]; > - > - parameters.parentProcessName = [[NSProcessInfo processInfo] processName]; > - parameters.nsURLCachePath = [(NSString *)cachePath.get() stringByStandardizingPath]; You don't need to move the assigment of parentProcessName, it just adds noise to the patch. > Source/WebKit2/WebProcess/mac/WebProcessMac.mm:216 > - appendReadwriteSandboxDirectory(sandboxParameters, "NSURL_CACHE_DIR", parameters.nsURLCachePath); > + appendReadwriteSandboxDirectory(sandboxParameters, "NSURL_CACHE_DIR", static_cast<NSString*>(parameters.diskCacheDirectory)); Do you need to cast to NSString* here? Isn't appendRead... a webkit function that get's a String? > Source/WebKit2/WebProcess/mac/WebProcessMac.mm:264 > - if (!parameters.nsURLCachePath.isNull()) { > + if (!static_cast<NSString*>(parameters.diskCacheDirectory).isNull()) { > NSUInteger cacheMemoryCapacity = parameters.nsURLCacheMemoryCapacity; > NSUInteger cacheDiskCapacity = parameters.nsURLCacheDiskCapacity; You don't need the cast here. > Tools/WebKitTestRunner/TestController.cpp:315 > + // Can we remove it? > +// const char* path = libraryPathForTesting(); > +// if (path) { > +// Vector<char> databaseDirectory(strlen(path) + strlen("/Databases") + 1); > +// sprintf(databaseDirectory.data(), "%s%s", path, "/Databases"); > +// WKRetainPtr<WKStringRef> databaseDirectoryWK(AdoptWK, WKStringCreateWithUTF8CString(databaseDirectory.data())); > +// WKContextSetDatabaseDirectory(m_context.get(), databaseDirectoryWK.get()); > +// } I think we can. :)

Adding API maintainers because this effects WebKit2 C API.

Created attachment 154618 [details] patch for ews Thanks for your advice!

Comment on attachment 154618 [details] patch for ews View in context: https://bugs.webkit.org/attachment.cgi?id=154618&action=review > Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:51 > + // FIXME: Implement > + return String(); Nit: missing a notImplemented() call.

Comment on attachment 154618 [details] patch for ews Attachment 154618 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13368050

Created attachment 157977 [details] patch for ews

Comment on attachment 157977 [details] patch for ews Attachment 157977 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13493071

Created attachment 157992 [details] patch for ews

Comment on attachment 149754 [details] patch Marking obviously obsolete attachmen to be obsolete :)

Comment on attachment 157992 [details] patch for ews View in context: https://bugs.webkit.org/attachment.cgi?id=157992&action=review This patch is missing a ChangeLog, which I suppose would provide an explanation as to why these changes are submitted? :) I'm particularly curious why the disk cache directory needs to be per DRT/WTR instance. > Source/WebKit2/UIProcess/mac/WebContextMac.mm:-95 > - ASSERT(!parameters.nsURLCachePath.isEmpty()); Technically this ASSERT should move somewhere else, shouldn't it? > Tools/WebKitTestRunner/TestController.cpp:320 > + WKContextSetDatabaseDirectory(m_context.get(), WKStringCreateWithUTF8CString(dumpRenderTreeTemp)); > + WKContextSetLocalStorageDirectory(m_context.get(), WKStringCreateWithUTF8CString(dumpRenderTreeTemp)); > + WKContextSetDiskCacheDirectory(m_context.get(), WKStringCreateWithUTF8CString(dumpRenderTreeTemp)); I think you need to store the strings created by WKStringCreate* in a RetainPtr to avoid them leaking.

(In reply to comment #23) > (From update of attachment 157992 [details]) > I'm particularly curious why the disk cache directory needs to be per DRT/WTR instance. Because we would like to go forward and run layout tests parallel (https://bugs.webkit.org/show_bug.cgi?id=77730). It isn't a good thing if a WTR modifies another ones disk cache. For example purges it between tests, but the other one still uses it. It was implemented for WK1 long time ago and works fine. Additionally we run more bots on same machine (only one WK2 tester now because of this bug), and don't want to let a slave to break an other slaves disk cache.

(In reply to comment #23) > (From update of attachment 157992 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157992&action=review > > This patch is missing a ChangeLog, which I suppose would provide an explanation as to why these changes are submitted? :) Sorry for the late response! These patches were only made for testing EWS, and I had the wrong intention that these patches need to be r=? and cq=? to test the EWS. I'm going to apply your recommendations and hints in the final patch (that will also contain ChangeLogs of course :) ). > > Source/WebKit2/UIProcess/mac/WebContextMac.mm:-95 > > - ASSERT(!parameters.nsURLCachePath.isEmpty()); > > Technically this ASSERT should move somewhere else, shouldn't it? From the final patch nsURLCachePath will be obsoleted. > > Tools/WebKitTestRunner/TestController.cpp:320 > > + WKContextSetDatabaseDirectory(m_context.get(), WKStringCreateWithUTF8CString(dumpRenderTreeTemp)); > > + WKContextSetLocalStorageDirectory(m_context.get(), WKStringCreateWithUTF8CString(dumpRenderTreeTemp)); > > + WKContextSetDiskCacheDirectory(m_context.get(), WKStringCreateWithUTF8CString(dumpRenderTreeTemp)); > > I think you need to store the strings created by WKStringCreate* in a RetainPtr to avoid them leaking. I'm going to do it.

Removing [Qt] as this touches platform independent code.

Created attachment 161824 [details] patch

Created attachment 163054 [details] patch v2

Comment on attachment 163054 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=163054&action=review I think this patch is an improvement, though I still find the ChangeLog a bit too brief about the issue that is being fixed. > Source/WebKit2/ChangeLog:8 > + WebContext has been extended for the paralelly started WTRs to use *parallelly* > Source/WebKit2/ChangeLog:12 > + separate directories for their local storage. > + platformDiskCacheDirectory API has been added to the existing > + platformLocalStorageDirectory and platformDataBaseDirectory APIs. > + I think it would be good to explain a bit more why this change is needed and what the symptoms of the bug are. Probably also refer to https://bugs.webkit.org/show_bug.cgi?id=77730 and the special for Qt to run multiple run-webkit-test instances in parallel. > Source/WebKit2/Shared/WebProcessCreationParameters.h:-104 > // FIXME: These should be merged with CFURLCache counterparts below. > - String nsURLCachePath; > - SandboxExtension::Handle nsURLCachePathExtensionHandle; Following the FIXME comment to me it looks like the cfURLCachePath below is used for the same purpose on Windows as nsURLCachePath on Mac. Doesn't this patch also affect the need for cfURLCachePath on Windows?

Created attachment 167054 [details] patch for ews

Comment on attachment 167054 [details] patch for ews Attachment 167054 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14175021

Created attachment 167094 [details] patch

Comment on attachment 167094 [details] patch Attachment 167094 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14171223

Created attachment 167530 [details] patch

Comment on attachment 167530 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=167530&action=review I think the patch looks good. The only two issues I can find are the missing notImplemented() and the static_cast that appears to be unnecessary. Please fix those before landing :) (And I suppose this needs very careful perhaps manual landing, since it affects the quite a few ports) > Source/WebKit2/UIProcess/win/WebContextWin.cpp:107 > + // FIXME: not implemented! Use notImplemented(); here? > Source/WebKit2/WebProcess/mac/WebProcessMac.mm:271 > - RetainPtr<NSURLCache> parentProcessURLCache(AdoptNS, [[NSURLCache alloc] initWithMemoryCapacity:cacheMemoryCapacity diskCapacity:cacheDiskCapacity diskPath:parameters.nsURLCachePath]); > + RetainPtr<NSURLCache> parentProcessURLCache(AdoptNS, [[NSURLCache alloc] initWithMemoryCapacity:cacheMemoryCapacity diskCapacity:cacheDiskCapacity diskPath:static_cast<NSString*>(parameters.diskCacheDirectory)]); Why is the static_cast needed here? It seems that with diskPath:parameters.nsURLCachePath the nsURLCachePath as a String. With diskPath:static_cast<NSString*>(parameters.diskCacheDirectory) the diskCacheDirectory appears to be a String, too. So I don't understand why the cast is needed :)

I tried the patch, but it seems WTR still uses this file: ~/.local/share/WebKitTestRunner/.QtWebKit/WebpageIcons.db Could you check it?

(In reply to comment #36) > I tried the patch, but it seems WTR still uses this file: > ~/.local/share/WebKitTestRunner/.QtWebKit/WebpageIcons.db > > Could you check it? I guess we should override the icondatabase path as well. But it should not stop this patch from landing, we can do that in a follow-up.

(In reply to comment #37) > (In reply to comment #36) > > I tried the patch, but it seems WTR still uses this file: > > ~/.local/share/WebKitTestRunner/.QtWebKit/WebpageIcons.db > > > > Could you check it? > > I guess we should override the icondatabase path as well. But it should not stop this patch from landing, we can do that in a follow-up. Sure, we can do it later.

Created attachment 168660 [details] patch for commit Thanks for your advice!

Comment on attachment 168660 [details] patch for commit Clearing flags on attachment: 168660 Committed r131428: <http://trac.webkit.org/changeset/131428>

All reviewed patches have been landed. Closing bug.