Bug 24704

When I said fixing this issue allows running multiple instances, I mean if the DumpRenderTree instances have different names. For example, one could create a symbol link and runs that instead to ensure that the settings are isolated from other instances.

Created attachment 28765 [details] Proposed fix.

Why is this useful?

One of the problems mentioned in https://bugs.webkit.org/show_bug.cgi?id=10906 is "- sharing of icon database, cookies and disk cache;" After these changes I could do this ln -s DumpRenderTree DumpRenderTree1 ln -s DumpRenderTree DumpRenderTree2 Then I can run DumpRenderTree1 and DumpRenderTree2 at the same time without sharing the above items. (WebKit on OSX already appears to use the app name to separate settings.)

I don't think that this is the appropriate way to address that problem.

> I don't think that this is the appropriate way to address that problem. That's a bit terse. Would you elaborate on what you think is "the appropriate way"? :) The other idea I had was to pass in some parameter to DumpRenderTree, but it seems like I would want the end result to be the same (separate location for these items). In addition, I would need to find and change code like WebKitInitializeApplicationCachePathIfNecessary() in WebView.mm also (while the differing app name worked nicely with this coding pattern).

(In reply to comment #6) > > I don't think that this is the appropriate way to address that problem. > That's a bit terse. Would you elaborate on what you think is "the appropriate > way"? :) Sorry, I replied to this from my iPhone and gave up on elaborating. Some things, such as cookies, are not application-specific. Since changing the application name can't address all sharing issues, I think it is better to take a cleaner approach. > The other idea I had was to pass in some parameter to DumpRenderTree, but it > seems like I would want the end result to be the same (separate location for > these items). In addition, I would need to find and change code like > WebKitInitializeApplicationCachePathIfNecessary() in WebView.mm also (while the > differing app name worked nicely with this coding pattern). I think that having DumpRenderTree generate a unique identifier and using that in paths, etc, to avoid conflicts would be preferable. It may require additional changes, but some of that functionality will be useful to other WebKit clients, and it seems cleaner than relying on how WebKit and other system frameworks happen to generate cache paths, etc.

Created attachment 28781 [details] Proposed fix.

The remaining issue as I understand it with the current patch is that it doesn't help with the CFUrlCache/CFNetwork. After much looking, I can't find anyway to do this in code. However, it does use the app name in forming the location for the cache, so a good way to address that is to use apps with different names (the ln-s technique).

(In reply to comment #9) > The remaining issue as I understand it with the current patch is that it > doesn't help with the CFUrlCache/CFNetwork. > > After much looking, I can't find anyway to do this in code. However, it does > use the app name in forming the location for the cache, so a good way to > address that is to use apps with different names (the ln-s technique). A better approach would be to disable the disk cache.

Created attachment 28863 [details] Proposed fix. Address previous issues. This patch still exports WebKitLocalCacheDefaultsKey as opposed to making it a constant. I was following the pattern of what was done for WebDatabaseDirectoryDefaultsKey in WebKit/mac/Storage/WebDatabaseManagerPrivate.h. If something different should be done in this case, let me know and I can change it.

Comment on attachment 28863 [details] Proposed fix. else 313 cacheDir = nil; isn't needed here. cacheDir is already known to be nil at the start of that (rather short) if block, right? Otherwise the approach looks fine to me. I guess Mark started this review and should comment before landing. But it looks fine IMO.

Comment on attachment 28863 [details] Proposed fix. It seems saving things to: ~/Library/tmp is silly. ~/Library/Application Support/DumpRenderTree or ~/Library/Caches/DumpRenderTree would make more sense. Otherwise this looks fine. Mark should feel encouraged to comment further, but this looks like a big step forward for our testing. Even if it's not perfect final layout, it's much better than what we have today (in terms of that it moves us closer to running tests in parallel).

Created attachment 30055 [details] Addressed eseidel's concerns.

It looks like this will also fix it so that jparent and I can both run layout tests at the same time on the same machine. :( Currently if one of us has run the other can't until we clear /tmp because the perl script tries to write the same files and gets permission errors. At least I think that's what happened. Dave, would you like me to land this, or do you plan to?

(In reply to comment #15) > It looks like this will also fix it so that jparent and I can both run layout > tests at the same time on the same machine. :( Currently if one of us has run > the other can't until we clear /tmp because the perl script tries to write the > same files and gets permission errors. At least I think that's what happened. Assuming that you're running as different users, that doesn't sound related to this change at all. The only path that this patch moves from /tmp is a single PDF output file. What you're more likely hitting is that the default location for test results is inside /tmp, but there's already a command-line flag to run-webkit-tests to redirect that.

I'm willing to land this. I didn't yet because I had heard there were reservations about it and I was trying to give folks a chance to voice up about it.... I'll land it tomorrow unless someone speaks up.