Remove GC code duplication in http/tests LayoutTests.
Created attachment 191772 [details] Patch
Comment on attachment 191772 [details] Patch Can we just use js-test-pre.js, which also defines gc()? I'm not quite sure if this script doesn't have any side effects when included though.
Created attachment 191874 [details] Use gc() in js-test-pre.js instead.
(In reply to comment #2) > (From update of attachment 191772 [details]) > Can we just use js-test-pre.js, which also defines gc()? I'm not quite sure if this script doesn't have any side effects when included though. I changed the code to use js-test-pre.js and reran all the tests locally and they seem to pass. Lets see what the EWS bots say. The mechanism for triggering GC when GCController is not available is different in the js-test-pre.js implementation, but I'm not sure how much that matters.
Comment on attachment 191874 [details] Use gc() in js-test-pre.js instead. Good refactoring. GC behavior is a bit unpredictable (e.g. 10000 and 100000 can result in different behavior) and thus it's important to maintain the code in one place.
Actually I've observed a couple of cases where manually written gc() didn't trigger a major GC of V8 because the number of object instantiation was not enough.
Comment on attachment 191874 [details] Use gc() in js-test-pre.js instead. View in context: https://bugs.webkit.org/attachment.cgi?id=191874&action=review r=me too, but I have comments that need to be addressed. > LayoutTests/ChangeLog:8 > +Replaced duplicated GC logic in various tests with a function call to the implementation in resources/js-test-pre.js Please fix formatting. > LayoutTests/http/tests/appcache/destroyed-frame.html:6 > +<script src="/resources/js-test-pre.js"></script> We should not have these files in http/tests/resources. Someone was confused when adding them. Please use "/js-test-resources/js-test-pre.js". This path is mapped to fast/js/resources, so we avoid code duplication. It would be helpful to remove the copies in http/tests/resources.
Comment on attachment 191874 [details] Use gc() in js-test-pre.js instead. View in context: https://bugs.webkit.org/attachment.cgi?id=191874&action=review >> LayoutTests/ChangeLog:8 >> +Replaced duplicated GC logic in various tests with a function call to the implementation in resources/js-test-pre.js > > Please fix formatting. done. >> LayoutTests/http/tests/appcache/destroyed-frame.html:6 >> +<script src="/resources/js-test-pre.js"></script> > > We should not have these files in http/tests/resources. Someone was confused when adding them. > > Please use "/js-test-resources/js-test-pre.js". This path is mapped to fast/js/resources, so we avoid code duplication. > > It would be helpful to remove the copies in http/tests/resources. I've changed includes to /js-test-resources/js-test-pre.js, but I think removing the duplicate file and updating the other tests should be in a different patch since it is unrelated to the goal of this bug.
Created attachment 192014 [details] Update includes to /js-test-resources/js-test-pre.js
Comment on attachment 192014 [details] Update includes to /js-test-resources/js-test-pre.js Clearing flags on attachment: 192014 Committed r145090: <http://trac.webkit.org/changeset/145090>
All reviewed patches have been landed. Closing bug.
> I think removing the duplicate file and updating the other tests should be in a different patch since it is unrelated to the goal of this bug. The title of this bug is "Remove GC code duplication in http/tests LayoutTests", so updating other tests is part of that task as stated. It's perfectly fine to do it in one patch or in multiple patches, whichever is more practical.