Bug 111589 – Remove GC code duplication in http/tests LayoutTests.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Aaron Colwell]

Remove GC code duplication in http/tests LayoutTests.

[Aaron Colwell]

Created attachment 191772 [details]
Patch

[Alexey Proskuryakov]

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]

Created attachment 191874 [details]
Use gc() in js-test-pre.js instead.

[Aaron Colwell]

(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 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]

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 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 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]

Created attachment 192014 [details]
Update includes to /js-test-resources/js-test-pre.js

[WebKit Review Bot]

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]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

> 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.