Bug 47680

Created attachment 70882 [details] Patch

Comment on attachment 70882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70882&action=review > LayoutTests/fast/filesystem/resources/file-writer-truncate-extend.js:16 > + verifyFileLength(fileEntry, contents.length + extension, in WebKit I don't think we need to wrap lines. > LayoutTests/fast/filesystem/resources/file-writer-utils.js:15 > +function assert(s) { Is there a strong reason you don't use shouldBe/shouldBeTrue here? This script already depend on some functions that are defined in js-test-pre.js (and fs-worker-common.js). > LayoutTests/fast/filesystem/resources/file-writer-utils.js:16 > + if (!s) { nit: no need to put { } for one line block > LayoutTests/fast/filesystem/resources/file-writer-utils.js:30 > + } catch (ex) { How about calling finishJSTest() here? > LayoutTests/fast/filesystem/resources/file-writer-utils.js:43 > + onSuccess(); // TODO: Swap for next line before checkin. hmm why do you to do this only before checkin...? > LayoutTests/fast/filesystem/resources/file-writer-utils.js:68 > + for (var i=0; i < length; i++) { nit: s/i=0/i = 0/ nit: no need of { } > LayoutTests/fast/filesystem/resources/fs-worker-test-pre.js:16 > +function testPassed(msg) If we really want full-functional, colorized testPassed/testFailed/description functions, I think we should just include js-test-pre.js. Replicating all the code here doesn't look good to me... > LayoutTests/fast/filesystem/resources/fs-worker-test-pre.js:45 > + testPassed(event.data.substring(5)); Not really sure we need this complication just to colorize the results? > LayoutTests/fast/filesystem/workers/file-writer-truncate-extend.html:4 > + <link rel="stylesheet" href="../../js/resources/js-test-style.css"> If we want to include js/resources in workers test we'll need some more tricks in chromium code and it'll automatically allow us to include js-test-pre.js here too. In that case we won't need fs-worker-test-pre.js any more -- let's forget about it and include js-test-pre.js.

Comment on attachment 70882 [details] Patch Kinuko: I'm addressing your changes, but I can't finish debugging yet due to some blocking bugs. I'll upload a new patch as soon as I've worked through the problems. Thanks for the review.

Created attachment 72117 [details] Patch

(In reply to comment #2) > (From update of attachment 70882 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=70882&action=review > > > LayoutTests/fast/filesystem/resources/file-writer-truncate-extend.js:16 > > + verifyFileLength(fileEntry, contents.length + extension, > > in WebKit I don't think we need to wrap lines. Unwrapped a bunch of lines. > > LayoutTests/fast/filesystem/resources/file-writer-utils.js:15 > > +function assert(s) { > > Is there a strong reason you don't use shouldBe/shouldBeTrue here? This script already depend on some functions that are defined in js-test-pre.js (and fs-worker-common.js). I don't trust it. I've got a boolean value already, which I can just test with assert. I'm not sure why shouldBe turns everything into strings and then calls eval on them, but it looks really complicated to me. I'd be happier getting rid of that stuff entirely where it's not absolutely necessary. > > LayoutTests/fast/filesystem/resources/file-writer-utils.js:16 > > + if (!s) { > > nit: no need to put { } for one line block Fixed. > > LayoutTests/fast/filesystem/resources/file-writer-utils.js:30 > > + } catch (ex) { > > How about calling finishJSTest() here? Done. > > LayoutTests/fast/filesystem/resources/file-writer-utils.js:43 > > + onSuccess(); // TODO: Swap for next line before checkin. > > hmm why do you to do this only before checkin...? That was a note to me to fix up the code before checking it in; there was a bug that prevented that test from being runnable, but it's fixed now. > > LayoutTests/fast/filesystem/resources/file-writer-utils.js:68 > > + for (var i=0; i < length; i++) { > > nit: s/i=0/i = 0/ > nit: no need of { } Fixed. > > LayoutTests/fast/filesystem/resources/fs-worker-test-pre.js:16 > > +function testPassed(msg) > > If we really want full-functional, colorized testPassed/testFailed/description functions, I think we should just include js-test-pre.js. Replicating all the code here doesn't look good to me... Removed a lot of replication, shared code where possible. > > LayoutTests/fast/filesystem/resources/fs-worker-test-pre.js:45 > > + testPassed(event.data.substring(5)); > > Not really sure we need this complication just to colorize the results? > > > LayoutTests/fast/filesystem/workers/file-writer-truncate-extend.html:4 > > + <link rel="stylesheet" href="../../js/resources/js-test-style.css"> > > If we want to include js/resources in workers test we'll need some more tricks in chromium code and it'll automatically allow us to include js-test-pre.js here too. In that case we won't need fs-worker-test-pre.js any more -- let's forget about it and include js-test-pre.js. What tricks are needed for Chromium?

Comment on attachment 72117 [details] Patch Thanks for cleaning this up! View in context: https://bugs.webkit.org/attachment.cgi?id=72117&action=review > LayoutTests/ChangeLog:8 > + Cleaning up shared test utility files caused the output of a bunch of existing tests change slightly, so I've rebuilt the expectation files. While fixing this, I found that some filesystem worker .html files were including javascript only appropriate for the workers themselves, so I removed the extra includes. Just to clarify... fs-worker-common.js was intentionally included in .html files for shouldBe methods (since we hadn't included js-test-pre.js). > LayoutTests/fast/filesystem/file-writer-truncate-extend.html:15 > +</html> Fyi, this does not apply to this test (and others that have .js files under resources/ but not under script-tests/) but might be worth mentioning... Many .html files that include js-test-pre.js are generated by a script (make-script-test-wrappers) from script-tests/TEMPLATE.html and corresponding .js files under script-tests. And most of them are not indented (since same/copy-past'ed TEMPLATE.html is often used). http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#WritingJavaScript-basedDOM-onlyTestCases > LayoutTests/fast/filesystem/resources/file-writer-truncate-extend.js:8 > +function onTestSucceeded() { nit: { on the next line in webkit-style. (here and everywhere) > LayoutTests/fast/filesystem/resources/file-writer-utils.js:5 > + s += index + ": " + o[index] + "\n"; Not sure if it was intentional, but "\n" wouldn't work as a line break in testFailed or in other span's. > LayoutTests/fast/filesystem/resources/file-writer-utils.js:13 > +}; nit: no need of ; at the end of line > LayoutTests/fast/filesystem/resources/file-writer-utils.js:27 > + finishJSTest(); nit: indent (both try and catch blocks) Btw do we need try-catch here? Async methods usually do not throw exceptions and we don't catch exceptions at all in most other places. > LayoutTests/fast/filesystem/resources/fs-worker-test-post.js:2 > +successfullyParsed = true; Now that we include js/resources/js-test-pre.js we'd better replace this with js/resources/js-test-post.js as well. (And we'll be able to remove this entire file.) Also could you put this "successfullyParsed = true;" line between <script></script> and after startWorker(...) in the .html files, as you did in file-writer-truncate-extend.html? I think it should be placed at the end of the main script body. > LayoutTests/fast/filesystem/resources/fs-worker-test-pre.js:9 > + if (event.data.substring(0, 5) == "PASS:") { No need of {} for one line blocks (here and below). If we want to handle worker messages differently how about using fixed-length prefixes for all messages? > LayoutTests/fast/filesystem/resources/fs-worker-test-pre.js:14 > + description(event.data.substring(13)); For this one do we care whether it is printed out by description() or by debug()? If we have description in the document's js file it gets replaced by the worker's one. > LayoutTests/fast/filesystem/workers/async-operations-expected.txt:15 > +PASS successfullyParsed is true This duplicated 'successfullyParsed is true' lines look confusing, how about prefixing all the worker's output with "Worker output:" or something? > LayoutTests/fast/filesystem/workers/async-operations.html:4 > <script src="../resources/fs-worker-test-pre.js"></script> Sorry for putting up requests, but could we rename this to fs-worker-test-util.js or something? (As fs-worker-test-post will be gone now) > LayoutTests/fast/filesystem/workers/file-from-file-entry-sync.html:3 > +<script src="../../js/resources/js-test-pre.js"></script> I think you'd want to include the css file here since many of your changes are for pretty-printing the results. <link rel="stylesheet" href="../../js/resources/js-test-style.css">

(In reply to comment #5) > > > LayoutTests/fast/filesystem/resources/file-writer-utils.js:15 > > > +function assert(s) { > > > > Is there a strong reason you don't use shouldBe/shouldBeTrue here? This script already depend on some functions that are defined in js-test-pre.js (and fs-worker-common.js). > > I don't trust it. I've got a boolean value already, which I can just test with assert. I'm not sure why shouldBe turns everything into strings and then calls eval on them, but it looks really complicated to me. I'd be happier getting rid of that stuff entirely where it's not absolutely necessary. I think it's ok not to use shouldBe here, and it's true that shouldBe does some (sometime undesired) stringify and evals, but it's also true that more than 1000 js files rely on this method(s). It must be 'trustable' and otherwise we'll need to fix it. > > If we want to include js/resources in workers test we'll need some more tricks in chromium code and it'll automatically allow us to include js-test-pre.js here too. In that case we won't need fs-worker-test-pre.js any more -- let's forget about it and include js-test-pre.js. > > What tricks are needed for Chromium? (oops my previous comment looks hard to understand) Adding these lines around line 167 should do. (It makes the necessary resource files copied while setting up the test environment) + AddResourceForLayoutTest(worker_test_dir.AppendASCII("js"), + resource_dir); Since worker tests for filesystem are not enabled yet you can file a bug and assign it to me if you want. I can fix it when I enable them.

Created attachment 72279 [details] Patch

(In reply to comment #6) > (From update of attachment 72117 [details]) > Thanks for cleaning this up! > > View in context: https://bugs.webkit.org/attachment.cgi?id=72117&action=review > > > LayoutTests/ChangeLog:8 > > + Cleaning up shared test utility files caused the output of a bunch of existing tests change slightly, so I've rebuilt the expectation files. While fixing this, I found that some filesystem worker .html files were including javascript only appropriate for the workers themselves, so I removed the extra includes. > > Just to clarify... fs-worker-common.js was intentionally included in .html files for shouldBe methods (since we hadn't included js-test-pre.js). Ah, I see. I was confused because it also contained functions that called postMessage. Anyway, I've taken your recommendations for cleanup, and so everything's much nicer now. > > LayoutTests/fast/filesystem/file-writer-truncate-extend.html:15 > > +</html> > > Fyi, this does not apply to this test (and others that have .js files under resources/ but not under script-tests/) but might be worth mentioning... > > Many .html files that include js-test-pre.js are generated by a script (make-script-test-wrappers) from script-tests/TEMPLATE.html and corresponding .js files under script-tests. And most of them are not indented (since same/copy-past'ed TEMPLATE.html is often used). > http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#WritingJavaScript-basedDOM-onlyTestCases OK, so I don't need to do anything this time? > > LayoutTests/fast/filesystem/resources/file-writer-truncate-extend.js:8 > > +function onTestSucceeded() { > > nit: { on the next line in webkit-style. (here and everywhere) Fixed. I reflexively go back to the wrong way in javascript. > > LayoutTests/fast/filesystem/resources/file-writer-utils.js:5 > > + s += index + ": " + o[index] + "\n"; > > Not sure if it was intentional, but "\n" wouldn't work as a line break in testFailed or in other span's. Fixed. > > LayoutTests/fast/filesystem/resources/file-writer-utils.js:13 > > +}; > > nit: no need of ; at the end of line Fixed. > > LayoutTests/fast/filesystem/resources/file-writer-utils.js:27 > > + finishJSTest(); > > nit: indent (both try and catch blocks) Fixed. > Btw do we need try-catch here? Async methods usually do not throw exceptions and we don't catch exceptions at all in most other places. True, but if e.g. somebody corrupts fileEntryForCleanup, we could get an exception calling the method, and I'd like to call finishJSTest even if we fail. But if you still think it's better to remove it, I will. > > LayoutTests/fast/filesystem/resources/fs-worker-test-post.js:2 > > +successfullyParsed = true; > > Now that we include js/resources/js-test-pre.js we'd better replace this with js/resources/js-test-post.js as well. (And we'll be able to remove this entire file.) Done. > Also could you put this "successfullyParsed = true;" line between <script></script> and after startWorker(...) in the .html files, as you did in file-writer-truncate-extend.html? I think it should be placed at the end of the main script body. Done. > > LayoutTests/fast/filesystem/resources/fs-worker-test-pre.js:9 > > + if (event.data.substring(0, 5) == "PASS:") { > > No need of {} for one line blocks (here and below). Fixed. > If we want to handle worker messages differently how about using fixed-length prefixes for all messages? Done. > > LayoutTests/fast/filesystem/resources/fs-worker-test-pre.js:14 > > + description(event.data.substring(13)); > > For this one do we care whether it is printed out by description() or by debug()? > If we have description in the document's js file it gets replaced by the worker's one. Removed the descriptions from the .html files, to match the non-worker tests. > > LayoutTests/fast/filesystem/workers/async-operations-expected.txt:15 > > +PASS successfullyParsed is true > > This duplicated 'successfullyParsed is true' lines look confusing, how about prefixing all the worker's output with "Worker output:" or something? Done: "[worker]" > > LayoutTests/fast/filesystem/workers/async-operations.html:4 > > <script src="../resources/fs-worker-test-pre.js"></script> > > Sorry for putting up requests, but could we rename this to fs-worker-test-util.js or something? (As fs-worker-test-post will be gone now) Yeah, that makes sense. Done. > > LayoutTests/fast/filesystem/workers/file-from-file-entry-sync.html:3 > > +<script src="../../js/resources/js-test-pre.js"></script> > > I think you'd want to include the css file here since many of your changes are for pretty-printing the results. > <link rel="stylesheet" href="../../js/resources/js-test-style.css"> Done.

(In reply to comment #7) > (In reply to comment #5) > > > > LayoutTests/fast/filesystem/resources/file-writer-utils.js:15 > > > > +function assert(s) { > > > > > > Is there a strong reason you don't use shouldBe/shouldBeTrue here? This script already depend on some functions that are defined in js-test-pre.js (and fs-worker-common.js). > > > > I don't trust it. I've got a boolean value already, which I can just test with assert. I'm not sure why shouldBe turns everything into strings and then calls eval on them, but it looks really complicated to me. I'd be happier getting rid of that stuff entirely where it's not absolutely necessary. > > I think it's ok not to use shouldBe here, and it's true that shouldBe does some (sometime undesired) stringify and evals, but it's also true that more than 1000 js files rely on this method(s). It must be 'trustable' and otherwise we'll need to fix it. True...but I suppose somewhere in those tests there's a test that actually needed the 'eval' behavior and the different scoping, and we really don't. > > > If we want to include js/resources in workers test we'll need some more tricks in chromium code and it'll automatically allow us to include js-test-pre.js here too. In that case we won't need fs-worker-test-pre.js any more -- let's forget about it and include js-test-pre.js. > > > > What tricks are needed for Chromium? > > (oops my previous comment looks hard to understand) > > Adding these lines around line 167 should do. (It makes the necessary resource files copied while setting up the test environment) > + AddResourceForLayoutTest(worker_test_dir.AppendASCII("js"), > + resource_dir); > > Since worker tests for filesystem are not enabled yet you can file a bug and assign it to me if you want. I can fix it when I enable them. Let's talk about this one; there's more I need to understand. Thanks for the reviews!

Comment on attachment 72279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72279&action=review > LayoutTests/fast/filesystem/resources/fs-worker-test-util.js:1 > +if (window.layoutTestController) Looks like we have duplicated entries for fs-worker-test-util.js in the patch and this one looks obsolete. > LayoutTests/fast/filesystem/resources/fs-worker-test-util.js:-1 > -if (window.layoutTestController) This one looks to be the 'real' fs-worker-test-util.js we want to include. > LayoutTests/fast/filesystem/workers/async-operations-expected.txt:7 > +[Worker] requested FileSystem. [Worker] prefix looks nice.

Hit 'publish' before adding more comments... overall looks good, except that this one seems to lack the ChangeLog and have the duplicated entries of fs-worker-test-util.js. Thanks for cleaning up them all. Also most of comments in .js files are wrapped at 80 chars; I would unwrap them to conform with the coding style. (In reply to comment #11) > (From update of attachment 72279 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72279&action=review > > > LayoutTests/fast/filesystem/resources/fs-worker-test-util.js:1 > > +if (window.layoutTestController) > > Looks like we have duplicated entries for fs-worker-test-util.js in the patch and this one looks obsolete. > > > LayoutTests/fast/filesystem/resources/fs-worker-test-util.js:-1 > > -if (window.layoutTestController) > > This one looks to be the 'real' fs-worker-test-util.js we want to include. > > > LayoutTests/fast/filesystem/workers/async-operations-expected.txt:7 > > +[Worker] requested FileSystem. > > [Worker] prefix looks nice.

(In reply to comment #12) > Hit 'publish' before adding more comments... overall looks good, except that this one seems to lack the ChangeLog and have the duplicated entries of fs-worker-test-util.js. Thanks for cleaning up them all. I'm not sure how the ChangeLog got dropped, but it should be in this patch. The odd duplication of fs-worker-test-util.js is because I both renamed it from its original location and edited it. > Also most of comments in .js files are wrapped at 80 chars; I would unwrap them to conform with the coding style. Fixed. > (In reply to comment #11) > > (From update of attachment 72279 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=72279&action=review > > > > > LayoutTests/fast/filesystem/resources/fs-worker-test-util.js:1 > > > +if (window.layoutTestController) > > > > Looks like we have duplicated entries for fs-worker-test-util.js in the patch and this one looks obsolete. > > > > > LayoutTests/fast/filesystem/resources/fs-worker-test-util.js:-1 > > > -if (window.layoutTestController) > > > > This one looks to be the 'real' fs-worker-test-util.js we want to include. > > > > > LayoutTests/fast/filesystem/workers/async-operations-expected.txt:7 > > > +[Worker] requested FileSystem. > > > > [Worker] prefix looks nice.

Created attachment 73315 [details] Patch

Comment on attachment 73315 [details] Patch lgtm.

Comment on attachment 73315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73315&action=review It looks like the .html files should be generated like what happens normally for script tests. Is this a future change? Please consider addressing the successfully parsed issue (and removing alert :) ), and this will be good to go. > LayoutTests/ChangeLog:8 > + Cleaning up shared test utility files caused the includes and output of a bunch of existing tests change slightly, so I've rebuilt the expectation files. We usually wrap lines in ChangeLog before they get this long. > LayoutTests/fast/filesystem/resources/file-from-file-entry-sync.js:20 > +finishJSTest(); This looks wrong here and in the other places where this happened. Typically var successfullyParsed = true; is last so that it is clear the whole file was parsed correctly (and it would be checked in js-test-post.js). > LayoutTests/fast/filesystem/resources/fs-worker-common.js:8 > + shouldBeTrue("successfullyParsed"); As mentioned before, I think this should be done elsewhere. > LayoutTests/fast/filesystem/resources/fs-worker-test-util.js:22 > + alert(event); alert?

(In reply to comment #16) > (From update of attachment 73315 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73315&action=review > > It looks like the .html files should be generated like what happens normally for script tests. Is this a future change? I'm not sure what you mean by this. I generated the .html files by hand, by running the tests in Chromium. They don't run in DumpRenderTree yet because we don't have support for these APIs in non-Chromium browsers yet. The filewriter-worker stuff is in that other changelist you're reviewing, so that's future stuff too. > Please consider addressing the successfully parsed issue (and removing alert :) ), and this will be good to go. > > > LayoutTests/ChangeLog:8 > > + Cleaning up shared test utility files caused the includes and output of a bunch of existing tests change slightly, so I've rebuilt the expectation files. > > We usually wrap lines in ChangeLog before they get this long. Done. > > LayoutTests/fast/filesystem/resources/file-from-file-entry-sync.js:20 > > +finishJSTest(); > > This looks wrong here and in the other places where this happened. > > Typically > var successfullyParsed = true; > is last so that it is clear the whole file was parsed correctly (and it would be checked in js-test-post.js). This is tricky. This test file is shared by the DOM test [where you can then include js-test-post.js if you want] and the worker test [where it's the main script for the worker, and nothing can follow it]. Given the current setup, I think we have the choice of either not bothering with the successfullyParsed stuff at all, or doing it something like this. An alternative would be to do something like the SQLDatabase tests, in which there's a common test-runner for all worker tests, and you send it commands. Then you can tell it to import the test script THEN the post-test script. I'm not sure that the successfullyParsed thing is really adding much to these worker tests; I'm inclined just to remove it. What do you think? > > LayoutTests/fast/filesystem/resources/fs-worker-common.js:8 > > + shouldBeTrue("successfullyParsed"); > > As mentioned before, I think this should be done elsewhere. > > > LayoutTests/fast/filesystem/resources/fs-worker-test-util.js:22 > > + alert(event); > > alert? Removed. Was old debugging stuff that slipped through.

Created attachment 74070 [details] Patch

Comment on attachment 73315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73315&action=review >>> LayoutTests/fast/filesystem/resources/file-from-file-entry-sync.js:20 >>> +finishJSTest(); >> >> This looks wrong here and in the other places where this happened. >> >> Typically >> var successfullyParsed = true; >> is last so that it is clear the whole file was parsed correctly (and it would be checked in js-test-post.js). > > This is tricky. This test file is shared by the DOM test [where you can then include js-test-post.js if you want] and the worker test [where it's the main script for the worker, and nothing can follow it]. Given the current setup, I think we have the choice of either not bothering with the successfullyParsed stuff at all, or doing it something like this. > > An alternative would be to do something like the SQLDatabase tests, in which there's a common test-runner for all worker tests, and you send it commands. Then you can tell it to import the test script THEN the post-test script. > > I'm not sure that the successfullyParsed thing is really adding much to these worker tests; I'm inclined just to remove it. What do you think? Fwiw, the point was most of the filesystem tests under resources run in async mode (jsTestIsAsync == true) and for those tests successfullyParsed is tested in finishJSTest() method but not in js-test-post.js. That's why we used to have successfullyParsed test in our own finishJSTest() for workers (in fs-worker-common.js). If you don't think we should test this flag we can just drop it. (I thought it was useful while debugging test scripts)

(In reply to comment #17) > (In reply to comment #16) > > (From update of attachment 73315 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=73315&action=review > > > > It looks like the .html files should be generated like what happens normally for script tests. Is this a future change? > > I'm not sure what you mean by this. I generated the .html files by hand, by running the tests in Chromium. They don't run in DumpRenderTree yet because we don't have support for these APIs in non-Chromium browsers yet. The filewriter-worker stuff is in that other changelist you're reviewing, so that's future stuff too. I think he means what I wrote in comment #6 ("Many .html files that include js-test-pre.js are generated by a script (make-script-test-wrappers) from..."). David, our .js files are under resources but not under script-tests (where the script assumes) mainly because I wanted to share as many .js files as possible for worker and non-worker tests. We'll need to modify the script or rearrange the whole test scripts to use the script to generate html files.

Comment on attachment 74070 [details] Patch Will resubmit tomorrow, with the change Kinuko and I discussed.

Created attachment 74712 [details] Patch

(In reply to comment #22) > Created an attachment (id=74712) [details] > Patch OK, now workers no longer bother checking successfullyParsed. I left the variable in the shared scripts, though, so the non-worker tests that use them still benefit from the check.

The commit-queue encountered the following flaky tests while processing attachment 74712 [details]: http/tests/appcache/foreign-fallback.html inspector/styles-source-offsets.html Please file bugs against the tests. These tests were authored by ap@webkit.org, apavlov@chromium.org, and pfeldman@chromium.org. The commit-queue is continuing to process your patch.

The commit-queue encountered the following flaky tests while processing attachment 74712 [details]: fast/workers/storage/use-same-database-in-page-and-workers.html Please file bugs against the tests. These tests were authored by dumi@chromium.org. The commit-queue is continuing to process your patch.

Comment on attachment 74712 [details] Patch Clearing flags on attachment: 74712 Committed r72704: <http://trac.webkit.org/changeset/72704>

All reviewed patches have been landed. Closing bug.