WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Use gc() in js-test-pre.js instead.
(13.26 KB, patch)
2013-03-06 17:40 PST
,
Aaron Colwell
no flags
Details
Formatted Diff
Diff
Update includes to /js-test-resources/js-test-pre.js
(13.41 KB, patch)
2013-03-07 09:12 PST
,
Aaron Colwell
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Aaron Colwell
Comment 1
2013-03-06 09:41:03 PST
Created
attachment 191772
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug