46544 2010-09-24 17:59:32 -0700 Fix event sequencing in FileWriter 2010-09-27 21:28:14 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) All All RESOLVED FIXED P2 Enhancement --- 44358 1 ericu ericu commit-queue kinuko levin michaeln oldest_to_newest 284881 0 ericu 2010-09-24 17:59:32 -0700 There are a couple of problems there. Some of the progress events are synchronous, and should really be asynchronous. The variable readyState is set too early [see info at http://lists.w3.org/Archives/Public/public-webapps/2010JulSep/1045.html]. 284883 1 68796 ericu 2010-09-24 18:08:23 -0700 Created attachment 68796 Patch 284969 2 68796 kinuko 2010-09-25 01:16:49 -0700 Comment on attachment 68796 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68796&action=review LGTM. > WebCore/fileapi/FileWriter.cpp:172 > + fireEvent(eventNames().writestartEvent); hmm... so we fire all of three events when it's finished. ok (seems like it conforms to the spec). 285288 3 68796 abarth 2010-09-26 21:25:56 -0700 Comment on attachment 68796 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68796&action=review We really need testing for this code / these patches. > WebCore/ChangeLog:13 > + No new tests, as none of this is fully implemented yet. What does that mean? It's too bad we can't test these patches. 285295 4 ericu 2010-09-26 21:35:56 -0700 (In reply to comment #3) > (From update of attachment 68796 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68796&action=review > > We really need testing for this code / these patches. > > > WebCore/ChangeLog:13 > > + No new tests, as none of this is fully implemented yet. > > What does that mean? It's too bad we can't test these patches. It means that I'm tweaking code that makes up only part of a feature, and we can't really test it until I put in the rest of the feature. None of this is exposed yet, and it needs an implementation of AsyncFileWriter to do most of its work. It could theoretically be unit-tested by making a mock AsyncFileWriter backend, but I was planning just to use layout tests once I got the real thing in [which should be quite soon]. That's the webkit way of doing the testing, right? 285865 5 levin 2010-09-27 16:28:47 -0700 (In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 68796 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=68796&action=review > > > > We really need testing for this code / these patches. > > > > > WebCore/ChangeLog:13 > > > + No new tests, as none of this is fully implemented yet. > > > > What does that mean? It's too bad we can't test these patches. > > It means that I'm tweaking code that makes up only part of a feature, and we can't really test it until I put in the rest of the feature. None of this is exposed yet, and it needs an implementation of AsyncFileWriter to do most of its work. > > It could theoretically be unit-tested by making a mock AsyncFileWriter backend, but I was planning just to use layout tests once I got the real thing in [which should be quite soon]. That's the webkit way of doing the testing, right? It is sort of. However with complicated features it is easy to do some minimal testing at the end (but it would have been much greater if the testing had been added as the code went along -- I'm guilty here as well). Is it possible to add a disabled test that would verify this if the functionality were working? 285876 6 ericu 2010-09-27 16:40:45 -0700 (In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (From update of attachment 68796 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=68796&action=review > > > > > > We really need testing for this code / these patches. > > > > > > > WebCore/ChangeLog:13 > > > > + No new tests, as none of this is fully implemented yet. > > > > > > What does that mean? It's too bad we can't test these patches. > > > > It means that I'm tweaking code that makes up only part of a feature, and we can't really test it until I put in the rest of the feature. None of this is exposed yet, and it needs an implementation of AsyncFileWriter to do most of its work. > > > > It could theoretically be unit-tested by making a mock AsyncFileWriter backend, but I was planning just to use layout tests once I got the real thing in [which should be quite soon]. That's the webkit way of doing the testing, right? > > It is sort of. However with complicated features it is easy to do some minimal testing at the end (but it would have been much greater if the testing had been added as the code went along -- I'm guilty here as well). > > Is it possible to add a disabled test that would verify this if the functionality were working? Well, of course it's possible ;'>. The API is defined in the spec, so I could write any number of FileWriter and FileSystem layout tests based on that and disable them all. But then we'd have a bunch of code that wasn't getting run, and could rot by the time it was ready to be turned on. These APIs are unfortunately changing as we implement them and run into bits that don't quite work in the spec. I think it may be premature to write the tests before we have even a single port implementing them. I expect to have both the Chromium and WebCore ports implemented within the next couple of months, though, and the Chromium one will have at least part of its functionality within a couple of weeks [for M8]. How would you feel about me adding the first test once I've got one port that could run it? 285883 7 68796 ericu 2010-09-27 16:45:17 -0700 Comment on attachment 68796 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68796&action=review >> WebCore/fileapi/FileWriter.cpp:172 >> + fireEvent(eventNames().writestartEvent); > > hmm... so we fire all of three events when it's finished. ok (seems like it conforms to the spec). Yeah, a truncate is sort of an all-at-once operation, so as soon as we know it's started, it's done. 286011 8 68796 commit-queue 2010-09-27 19:21:20 -0700 Comment on attachment 68796 Patch Rejecting patch 68796 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2 Last 500 characters of output: th/git/webkit-queue/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py", line 501, in start_websocket_server self._websocket_server.start() File "/Users/abarth/git/webkit-queue/WebKitTools/Scripts/webkitpy/layout_tests/port/websocket_server.py", line 219, in start (self._server_name, self._port)) PyWebSocketNotStarted: Failed to start PyWebSocket server on port 8880. ---------------------------------------------------------------------- Ran 607 tests in 25.087s FAILED (errors=1) Full output: http://queues.webkit.org/results/4042144 286048 9 68796 commit-queue 2010-09-27 21:28:09 -0700 Comment on attachment 68796 Patch Clearing flags on attachment: 68796 Committed r68483: <http://trac.webkit.org/changeset/68483> 286049 10 commit-queue 2010-09-27 21:28:14 -0700 All reviewed patches have been landed. Closing bug. 68796 2010-09-24 18:08:23 -0700 2010-09-27 21:28:09 -0700 Patch bug-46544-20100924180822.patch text/plain 3524 ericu SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2ODMxNikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjUgQEAKKzIwMTAtMDktMjQgIEVyaWMgVWhyaGFuZSAgPGVyaWN1QGNocm9taXVt Lm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBG aXggZXZlbnQgc2VxdWVuY2luZyBpbiBGaWxlV3JpdGVyCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00NjU0NAorCisgICAgICAgIE9ubHkgc2V0IHJlYWR5 U3RhdGUgdG8gRE9ORSB3aGVuIHdlJ3JlIGFib3V0IHRvIHNlbmQgdGhlIGxhc3QgcHJvZ3Jlc3MK KyAgICAgICAgZXZlbnQgYXNzb2NpYXRlZCB3aXRoIGFuIG9wZXJhdGlvbi4gIE1ha2Ugc3VyZSBh bGwgcHJvZ3Jlc3MgZXZlbnRzIGNvbWUKKyAgICAgICAgZnJvbSBiYWNrZW5kIGNhbGxzLCBhbmQg YXJlbid0IGV2ZXIgZmlyZWQgc3luY2hyb25vdXNseSBpbiByZXNwb25zZSB0bworICAgICAgICB1 c2VyIEpTIG1ldGhvZCBjYWxscy4KKworICAgICAgICBObyBuZXcgdGVzdHMsIGFzIG5vbmUgb2Yg dGhpcyBpcyBmdWxseSBpbXBsZW1lbnRlZCB5ZXQuCisKKyAgICAgICAgKiBmaWxlYXBpL0ZpbGVX cml0ZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RmlsZVdyaXRlcjo6d3JpdGUpOgorICAgICAg ICAoV2ViQ29yZTo6RmlsZVdyaXRlcjo6dHJ1bmNhdGUpOgorICAgICAgICAoV2ViQ29yZTo6Rmls ZVdyaXRlcjo6YWJvcnQpOgorICAgICAgICAoV2ViQ29yZTo6RmlsZVdyaXRlcjo6ZGlkV3JpdGUp OgorICAgICAgICAoV2ViQ29yZTo6RmlsZVdyaXRlcjo6ZGlkVHJ1bmNhdGUpOgorICAgICAgICAo V2ViQ29yZTo6RmlsZVdyaXRlcjo6ZGlkRmFpbCk6CisKIDIwMTAtMDktMjQgIE1hcnRpbiBSb2Jp bnNvbiAgPG1yb2JpbnNvbkBpZ2FsaWEuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEd1c3Rh dm8gTm9yb25oYSBTaWx2YS4KSW5kZXg6IFdlYkNvcmUvZmlsZWFwaS9GaWxlV3JpdGVyLmNwcAo9 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09Ci0tLSBXZWJDb3JlL2ZpbGVhcGkvRmlsZVdyaXRlci5jcHAJKHJldmlzaW9uIDY4 MjY2KQorKysgV2ViQ29yZS9maWxlYXBpL0ZpbGVXcml0ZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBA IC05Niw3ICs5Niw2IEBAIHZvaWQgRmlsZVdyaXRlcjo6d3JpdGUoQmxvYiogZGF0YSwgRXhjZXAK ICAgICBtX3JlYWR5U3RhdGUgPSBXUklUSU5HOwogICAgIG1fYnl0ZXNXcml0dGVuID0gMDsKICAg ICBtX2J5dGVzVG9Xcml0ZSA9IGRhdGEtPnNpemUoKTsKLSAgICBmaXJlRXZlbnQoZXZlbnROYW1l cygpLndyaXRlc3RhcnRFdmVudCk7CiAgICAgbV93cml0ZXItPndyaXRlKG1fcG9zaXRpb24sIGRh dGEpOwogfQogCkBAIC0xMzIsNyArMTMxLDYgQEAgdm9pZCBGaWxlV3JpdGVyOjp0cnVuY2F0ZShs b25nIGxvbmcgcG9zaQogICAgIG1fYnl0ZXNXcml0dGVuID0gMDsKICAgICBtX2J5dGVzVG9Xcml0 ZSA9IDA7CiAgICAgbV90cnVuY2F0ZUxlbmd0aCA9IHBvc2l0aW9uOwotICAgIGZpcmVFdmVudChl dmVudE5hbWVzKCkud3JpdGVzdGFydEV2ZW50KTsKICAgICBtX3dyaXRlci0+dHJ1bmNhdGUocG9z aXRpb24pOwogfQogCkBAIC0xNDQsMTEgKzE0Miw4IEBAIHZvaWQgRmlsZVdyaXRlcjo6YWJvcnQo RXhjZXB0aW9uQ29kZSYgZWMKICAgICAgICAgbV9lcnJvciA9IEZpbGVFcnJvcjo6Y3JlYXRlKGVj KTsKICAgICAgICAgcmV0dXJuOwogICAgIH0KKyAgICAKICAgICBtX2Vycm9yID0gRmlsZUVycm9y OjpjcmVhdGUoQUJPUlRfRVJSKTsKLSAgICBtX3JlYWR5U3RhdGUgPSBET05FOwotICAgIGZpcmVF dmVudChldmVudE5hbWVzKCkuZXJyb3JFdmVudCk7Ci0gICAgZmlyZUV2ZW50KGV2ZW50TmFtZXMo KS5hYm9ydEV2ZW50KTsKLSAgICBmaXJlRXZlbnQoZXZlbnROYW1lcygpLndyaXRlZW5kRXZlbnQp OwogICAgIG1fd3JpdGVyLT5hYm9ydCgpOwogfQogCkBAIC0xNTcsMzUgKzE1Miw0MSBAQCB2b2lk IEZpbGVXcml0ZXI6OmRpZFdyaXRlKGxvbmcgbG9uZyBieXRlCiAgICAgQVNTRVJUKGJ5dGVzID4g MCk7CiAgICAgQVNTRVJUKGJ5dGVzICsgbV9ieXRlc1dyaXR0ZW4gPiAwKTsKICAgICBBU1NFUlQo Ynl0ZXMgKyBtX2J5dGVzV3JpdHRlbiA8PSBtX2J5dGVzVG9Xcml0ZSk7CisgICAgaWYgKCFtX2J5 dGVzV3JpdHRlbikKKyAgICAgICAgZmlyZUV2ZW50KGV2ZW50TmFtZXMoKS53cml0ZXN0YXJ0RXZl bnQpOwogICAgIG1fYnl0ZXNXcml0dGVuICs9IGJ5dGVzOwogICAgIEFTU0VSVCgobV9ieXRlc1dy aXR0ZW4gPT0gbV9ieXRlc1RvV3JpdGUpID09IGNvbXBsZXRlKTsKICAgICBtX3Bvc2l0aW9uICs9 IGJ5dGVzOwogICAgIGlmIChtX3Bvc2l0aW9uID4gbV9sZW5ndGgpCiAgICAgICAgIG1fbGVuZ3Ro ID0gbV9wb3NpdGlvbjsKLSAgICBpZiAoY29tcGxldGUpCi0gICAgICAgIG1fcmVhZHlTdGF0ZSA9 IERPTkU7CiAgICAgZmlyZUV2ZW50KGV2ZW50TmFtZXMoKS53cml0ZUV2ZW50KTsKLSAgICBpZiAo Y29tcGxldGUpCisgICAgaWYgKGNvbXBsZXRlKSB7CisgICAgICAgIG1fcmVhZHlTdGF0ZSA9IERP TkU7CiAgICAgICAgIGZpcmVFdmVudChldmVudE5hbWVzKCkud3JpdGVlbmRFdmVudCk7CisgICAg fQogfQogCiB2b2lkIEZpbGVXcml0ZXI6OmRpZFRydW5jYXRlKCkKIHsKICAgICBBU1NFUlQobV90 cnVuY2F0ZUxlbmd0aCA+PSAwKTsKKyAgICBmaXJlRXZlbnQoZXZlbnROYW1lcygpLndyaXRlc3Rh cnRFdmVudCk7CiAgICAgbV9sZW5ndGggPSBtX3RydW5jYXRlTGVuZ3RoOwogICAgIGlmIChtX3Bv c2l0aW9uID4gbV9sZW5ndGgpCiAgICAgICAgIG1fcG9zaXRpb24gPSBtX2xlbmd0aDsKLSAgICBt X3JlYWR5U3RhdGUgPSBET05FOwogICAgIG1fdHJ1bmNhdGVMZW5ndGggPSAtMTsKICAgICBmaXJl RXZlbnQoZXZlbnROYW1lcygpLndyaXRlRXZlbnQpOworICAgIG1fcmVhZHlTdGF0ZSA9IERPTkU7 CiAgICAgZmlyZUV2ZW50KGV2ZW50TmFtZXMoKS53cml0ZWVuZEV2ZW50KTsKIH0KIAogdm9pZCBG aWxlV3JpdGVyOjpkaWRGYWlsKEV4Y2VwdGlvbkNvZGUgZWMpCiB7CiAgICAgbV9lcnJvciA9IEZp bGVFcnJvcjo6Y3JlYXRlKGVjKTsKLSAgICBtX3JlYWR5U3RhdGUgPSBET05FOwogICAgIGZpcmVF dmVudChldmVudE5hbWVzKCkuZXJyb3JFdmVudCk7CisgICAgaWYgKEFCT1JUX0VSUiA9PSBlYykK KyAgICAgICAgZmlyZUV2ZW50KGV2ZW50TmFtZXMoKS5hYm9ydEV2ZW50KTsKKyAgICBmaXJlRXZl bnQoZXZlbnROYW1lcygpLmVycm9yRXZlbnQpOworICAgIG1fcmVhZHlTdGF0ZSA9IERPTkU7CiAg ICAgZmlyZUV2ZW50KGV2ZW50TmFtZXMoKS53cml0ZWVuZEV2ZW50KTsKIH0KIAo=