RESOLVED FIXED 111589
Remove GC code duplication in http/tests LayoutTests.
https://bugs.webkit.org/show_bug.cgi?id=111589
Summary Remove GC code duplication in http/tests LayoutTests.
Aaron Colwell
Reported 2013-03-06 09:37:09 PST
Remove GC code duplication in http/tests LayoutTests.
Attachments
Patch (13.77 KB, patch)
2013-03-06 09:41 PST, Aaron Colwell
no flags
Use gc() in js-test-pre.js instead. (13.26 KB, patch)
2013-03-06 17:40 PST, Aaron Colwell
no flags
Update includes to /js-test-resources/js-test-pre.js (13.41 KB, patch)
2013-03-07 09:12 PST, Aaron Colwell
no flags
Aaron Colwell
Comment 1 2013-03-06 09:41:03 PST
Alexey Proskuryakov
Comment 2 2013-03-06 16:59:39 PST
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.
Aaron Colwell
Comment 3 2013-03-06 17:40:03 PST
Created attachment 191874 [details] Use gc() in js-test-pre.js instead.
Aaron Colwell
Comment 4 2013-03-06 17:42:04 PST
(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.
Kentaro Hara
Comment 5 2013-03-06 18:00:04 PST
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.
Kentaro Hara
Comment 6 2013-03-06 18:01:56 PST
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.
Alexey Proskuryakov
Comment 7 2013-03-06 19:37:47 PST
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.
Aaron Colwell
Comment 8 2013-03-07 09:11:58 PST
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.
Aaron Colwell
Comment 9 2013-03-07 09:12:07 PST
Created attachment 192014 [details] Update includes to /js-test-resources/js-test-pre.js
WebKit Review Bot
Comment 10 2013-03-07 09:41:51 PST
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>
WebKit Review Bot
Comment 11 2013-03-07 09:41:54 PST
All reviewed patches have been landed. Closing bug.
Alexey Proskuryakov
Comment 12 2013-03-07 10:10:32 PST
> 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.
Note You need to log in before you can comment on or make changes to this bug.