Bug 96543

Created attachment 163671 [details] Patch

Comment on attachment 163671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163671&action=review > Tools/Scripts/webkitpy/layout_tests/port/gtk.py:-70 > + environment['XDG_CACHE_HOME'] = self._filesystem.join(self.results_directory(), 'appcache') > self._copy_value_from_environ_if_set(environment, 'WEBKITOUTPUTDIR') > - if self.get_option('webkit_test_runner'): > - # FIXME: This is a workaround to ensure that testing with WebKitTestRunner is started with > - # a non-existing cache. This should be removed when (and if) it will be possible to properly > - # set the cache directory path through a WebKitWebContext. > - environment['XDG_CACHE_HOME'] = self._filesystem.join(self.results_directory(), 'appcache') Not sure I understand this change. If the appcache is cleared properly why do we still need to set XDG_CACHE_HOME to point to the results directory?

(In reply to comment #2) > > Not sure I understand this change. If the appcache is cleared properly why do we still need to set XDG_CACHE_HOME to point to the results directory? One reason would be to avoid manipulating the default user's cache (~/.cache/) which is used by Epiphany as well. Another reason is to start every test run (at least on the builders, with each build) with a clean, non-existent cache. I can put a comment above this change, explaining why this is done.

(In reply to comment #3) > (In reply to comment #2) > > > > Not sure I understand this change. If the appcache is cleared properly why do we still need to set XDG_CACHE_HOME to point to the results directory? > > One reason would be to avoid manipulating the default user's cache (~/.cache/) which is used by Epiphany as well. Well Epiphany uses a custom directory I hope? > Another reason is to start every test run (at least on the builders, with each build) with a clean, non-existent cache. > But that's exactly the purpose of the method added in DRT support no?

(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > > > > Not sure I understand this change. If the appcache is cleared properly why do we still need to set XDG_CACHE_HOME to point to the results directory? > > > > One reason would be to avoid manipulating the default user's cache (~/.cache/) which is used by Epiphany as well. > > Well Epiphany uses a custom directory I hope? I can't tell for sure, the cache directory is by default set to $XDG_CACHE_HOME/webkitgtk/applications: http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/webkit/webkitglobals.cpp#L585 > > Another reason is to start every test run (at least on the builders, with each build) with a clean, non-existent cache. > > > > But that's exactly the purpose of the method added in DRT support no? The main use of that method is to clean the cache between each test. Setting XDG_CACHE_HOME env just assures we're not poking around user's actual WebKitGTK+ cache and that the testing is started with an empty cache directory.

(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > Another reason is to start every test run (at least on the builders, with each build) with a clean, non-existent cache. > > > > > > > But that's exactly the purpose of the method added in DRT support no? > > The main use of that method is to clean the cache between each test. Setting XDG_CACHE_HOME env just assures we're not poking around user's actual WebKitGTK+ cache and that the testing is started with an empty cache directory. Another option is to point XDG_CACHE_HOME to a temporary directory (if it's decided overriding XDG_CACHE_HOME is acceptable).

Ok I think it's fine to use the results directory then. Thanks for the clarification!

(In reply to comment #7) > Ok I think it's fine to use the results directory then. Thanks for the clarification! So r+ maybe? :>

Comment on attachment 163671 [details] Patch r=me

(In reply to comment #9) > (From update of attachment 163671 [details]) > r=me Thanks!

Comment on attachment 163671 [details] Patch Clearing flags on attachment: 163671 Committed r128453: <http://trac.webkit.org/changeset/128453>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 96681

Created attachment 164115 [details] Patch

Comment on attachment 164115 [details] Patch Clearing flags on attachment: 164115 Committed r128596: <http://trac.webkit.org/changeset/128596>

All reviewed patches have been landed. Closing bug.