Bug 111589

Summary: Remove GC code duplication in http/tests LayoutTests.
Product: WebKit Reporter: Aaron Colwell <acolwell>
Component: New BugsAssignee: Aaron Colwell <acolwell>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, eric.carlson, feature-media-reviews, haraken, jer.noble, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Use gc() in js-test-pre.js instead.
none
Update includes to /js-test-resources/js-test-pre.js none

Description Aaron Colwell 2013-03-06 09:37:09 PST
Remove GC code duplication in http/tests LayoutTests.
Comment 1 Aaron Colwell 2013-03-06 09:41:03 PST
Created attachment 191772 [details]
Patch
Comment 2 Alexey Proskuryakov 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.
Comment 3 Aaron Colwell 2013-03-06 17:40:03 PST
Created attachment 191874 [details]
Use gc() in js-test-pre.js instead.
Comment 4 Aaron Colwell 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.
Comment 5 Kentaro Hara 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.
Comment 6 Kentaro Hara 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.
Comment 7 Alexey Proskuryakov 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.
Comment 8 Aaron Colwell 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.
Comment 9 Aaron Colwell 2013-03-07 09:12:07 PST
Created attachment 192014 [details]
Update includes to /js-test-resources/js-test-pre.js
Comment 10 WebKit Review Bot 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>
Comment 11 WebKit Review Bot 2013-03-07 09:41:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Alexey Proskuryakov 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.