85139 2012-04-28 10:08:32 -0700 MessagePort must set m_closed to be true at the end of MessagePort::close function 2012-04-28 18:13:26 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 0 li.yin webkit-unassigned abarth ap dimich haraken levin webkit.review.bot oldest_to_newest 612285 0 li.yin 2012-04-28 10:08:32 -0700 In the function MessagePort::close, the "m_closed = true" must be executed at the end of function, not at the beginning. Or, the m_entangledChannel->close() will not be executed. And it resulted in the failure of MS bench mark messagechannel_close.htm. You can try it from http://samples.msdn.microsoft.com/ietestcenter/WebWorkers/messagechannel_close.htm 612286 1 139363 li.yin 2012-04-28 10:22:31 -0700 Created attachment 139363 Patch 612295 2 139363 haraken 2012-04-28 11:15:38 -0700 Comment on attachment 139363 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139363&action=review The change looks reasonable. r- due to missing a test. I think this change is testable. > Source/WebCore/ChangeLog:12 > + No new tests. The test can be found from http://samples.msdn.microsoft.com/ietestcenter/WebWorkers/messagechannel_close.htm Would you add the test to LayoutTests? 612303 3 139368 li.yin 2012-04-28 12:03:17 -0700 Created attachment 139368 Patch 612305 4 li.yin 2012-04-28 12:07:42 -0700 (In reply to comment #2) > (From update of attachment 139363 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139363&action=review > > The change looks reasonable. r- due to missing a test. I think this change is testable. > > > Source/WebCore/ChangeLog:12 > > + No new tests. The test can be found from http://samples.msdn.microsoft.com/ietestcenter/WebWorkers/messagechannel_close.htm > > Would you add the test to LayoutTests? Add the test case, please have a look, thanks. 612310 5 139368 haraken 2012-04-28 12:12:51 -0700 Comment on attachment 139368 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139368&action=review > LayoutTests/fast/events/message-port-close.html:1 > +<body> Nit: Shall we use a standard HTML format? <!DOCTYPE html><html><head></head><body>...</body></html> > LayoutTests/fast/events/message-port-close.html:4 > +Test Closed MessagePort Whether Receive Message Or Not. > +Should be a START message, followed with DONE. > +<pre id=log></pre> It would be better to use js-test-pre.js and js-test-post.js. You can use a series of methods for tests (e.g. debug("..."), shouldBe("...") etc). Please look at other layout tests using them. 612312 6 li.yin 2012-04-28 12:18:31 -0700 (In reply to comment #5) > (From update of attachment 139368 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139368&action=review > > > LayoutTests/fast/events/message-port-close.html:1 > > +<body> > > Nit: Shall we use a standard HTML format? > > <!DOCTYPE html><html><head></head><body>...</body></html> > > > LayoutTests/fast/events/message-port-close.html:4 > > +Test Closed MessagePort Whether Receive Message Or Not. > > +Should be a START message, followed with DONE. > > +<pre id=log></pre> > > It would be better to use js-test-pre.js and js-test-post.js. You can use a series of methods for tests (e.g. debug("..."), shouldBe("...") etc). Please look at other layout tests using them. I am following the format of LayoutTests/fast/events/message-port-clone.html, it seems that all of MessagePort related test cases used this format. 612320 7 139368 haraken 2012-04-28 13:26:55 -0700 Comment on attachment 139368 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139368&action=review >>> LayoutTests/fast/events/message-port-close.html:1 >>> +<body> >> >> Nit: Shall we use a standard HTML format? >> >> <!DOCTYPE html><html><head></head><body>...</body></html> > > I am following the format of LayoutTests/fast/events/message-port-clone.html, it seems that all of MessagePort related test cases used this format. Makes sense. 612365 8 139368 webkit.review.bot 2012-04-28 18:13:21 -0700 Comment on attachment 139368 Patch Clearing flags on attachment: 139368 Committed r115588: <http://trac.webkit.org/changeset/115588> 612366 9 webkit.review.bot 2012-04-28 18:13:26 -0700 All reviewed patches have been landed. Closing bug. 139363 2012-04-28 10:22:31 -0700 2012-04-28 12:03:07 -0700 Patch bug-85139-20120430012535.patch text/plain 1650 li.yin U3VidmVyc2lvbiBSZXZpc2lvbjogMTE1NTcyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNzAzYmYxZDNkY2Y0ZmMy MGNiMzNmNGQzYzI0YTQwZjA5NzZjMzc1Zi4uOWM0NTY4NTliZGM2MTZkNThiNGEwNjhlMDM4OTQz ODdiZDQwNzNiNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDEyLTA0LTI5ICBMaSBZ aW4gIDxsaS55aW5AaW50ZWwuY29tPgorCisgICAgICAgIE1lc3NhZ2VQb3J0IG11c3Qgc2V0IG1f Y2xvc2VkIHRvIGJlIHRydWUgYXQgdGhlIGVuZCBvZiBNZXNzYWdlUG9ydDo6Y2xvc2UgZnVuY3Rp b24KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTg1MTM5 CisKKyAgICAgICAgSW4gdGhlIGZ1bmN0aW9uIE1lc3NhZ2VQb3J0OjpjbG9zZSwgdGhlICJtX2Ns b3NlZCA9IHRydWUiIG11c3QgYmUgZXhlY3V0ZWQgYXQgdGhlIGVuZCwgbm90IGF0IHRoZSBiZWdp bm5pbmcuCisgICAgICAgIE9yLCB0aGUgbV9lbnRhbmdsZWRDaGFubmVsLT5jbG9zZSgpIHdpbGwg bm90IGJlIGV4ZWN1dGVkLgorICAgICAgICBBbmQgaXQgcmVzdWx0ZWQgaW4gdGhlIGZhaWx1cmUg b2YgTVMgYmVuY2ggbWFyayBtZXNzYWdlY2hhbm5lbF9jbG9zZS5odG0uCisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLiBUaGUgdGVz dCBjYW4gYmUgZm91bmQgZnJvbSBodHRwOi8vc2FtcGxlcy5tc2RuLm1pY3Jvc29mdC5jb20vaWV0 ZXN0Y2VudGVyL1dlYldvcmtlcnMvbWVzc2FnZWNoYW5uZWxfY2xvc2UuaHRtCisKKyAgICAgICAg KiBkb20vTWVzc2FnZVBvcnQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6TWVzc2FnZVBvcnQ6OmNs b3NlKToKKwogMjAxMi0wNC0yOCAgQWxleGFuZGVyIFBhdmxvdiAgPGFwYXZsb3ZAY2hyb21pdW0u b3JnPgogCiAgICAgICAgIFdlYiBJbnNwZWN0b3I6IEVuYWJsZSB0b3VjaCBldmVudHMgZmVhdHVy ZSBmYWlscyB0b3VjaCBmZWF0dXJlIGRldGVjdGlvbgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNv cmUvZG9tL01lc3NhZ2VQb3J0LmNwcCBiL1NvdXJjZS9XZWJDb3JlL2RvbS9NZXNzYWdlUG9ydC5j cHAKaW5kZXggYWRlMjQ5YzVlNTI5YWMxZjQ3MTVlYjRiYWEzZmUxYThhMzE3ODg1MS4uYWI0ZGNk YzBkNWQ4NWIzM2ViZDUzZTViZTg3Y2IwY2I2NGM3ZTQzOSAxMDA2NDQKLS0tIGEvU291cmNlL1dl YkNvcmUvZG9tL01lc3NhZ2VQb3J0LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9kb20vTWVzc2Fn ZVBvcnQuY3BwCkBAIC0xMzMsMTAgKzEzMywxMCBAQCB2b2lkIE1lc3NhZ2VQb3J0OjpzdGFydCgp CiAKIHZvaWQgTWVzc2FnZVBvcnQ6OmNsb3NlKCkKIHsKLSAgICBtX2Nsb3NlZCA9IHRydWU7CiAg ICAgaWYgKCFpc0VudGFuZ2xlZCgpKQogICAgICAgICByZXR1cm47CiAgICAgbV9lbnRhbmdsZWRD aGFubmVsLT5jbG9zZSgpOworICAgIG1fY2xvc2VkID0gdHJ1ZTsKIH0KIAogdm9pZCBNZXNzYWdl UG9ydDo6ZW50YW5nbGUoUGFzc093blB0cjxNZXNzYWdlUG9ydENoYW5uZWw+IHJlbW90ZSkK 139368 2012-04-28 12:03:17 -0700 2012-04-28 18:13:21 -0700 Patch bug-85139-20120430030621.patch text/plain 4145 li.yin U3VidmVyc2lvbiBSZXZpc2lvbjogMTE1NTcyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNzAzYmYxZDNkY2Y0ZmMy MGNiMzNmNGQzYzI0YTQwZjA5NzZjMzc1Zi4uZWE0OGIwODAzNWUxMDMzZGU1ZWQzZjgyYjJlZGI2 NjgyOWYzY2ZkNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDEyLTA0LTI5ICBMaSBZ aW4gIDxsaS55aW5AaW50ZWwuY29tPgorCisgICAgICAgIE1lc3NhZ2VQb3J0IG11c3Qgc2V0IG1f Y2xvc2VkIHRvIGJlIHRydWUgYXQgdGhlIGVuZCBvZiBNZXNzYWdlUG9ydDo6Y2xvc2UgZnVuY3Rp b24KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTg1MTM5 CisKKyAgICAgICAgSW4gdGhlIGZ1bmN0aW9uIE1lc3NhZ2VQb3J0OjpjbG9zZSwgdGhlICJtX2Ns b3NlZCA9IHRydWUiIG11c3QgYmUgZXhlY3V0ZWQgYXQgdGhlIGVuZCwgbm90IGF0IHRoZSBiZWdp bm5pbmcuCisgICAgICAgIE9yLCB0aGUgbV9lbnRhbmdsZWRDaGFubmVsLT5jbG9zZSgpIHdpbGwg bm90IGJlIGV4ZWN1dGVkLgorICAgICAgICBBbmQgaXQgcmVzdWx0ZWQgaW4gdGhlIGZhaWx1cmUg b2YgTVMgYmVuY2ggbWFyayBtZXNzYWdlY2hhbm5lbF9jbG9zZS5odG0uCisgICAgICAgIGh0dHA6 Ly9zYW1wbGVzLm1zZG4ubWljcm9zb2Z0LmNvbS9pZXRlc3RjZW50ZXIvV2ViV29ya2Vycy9tZXNz YWdlY2hhbm5lbF9jbG9zZS5odG0KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICBUZXN0OiBmYXN0L2V2ZW50cy9tZXNzYWdlLXBvcnQtY2xvc2UuaHRtbAor CisgICAgICAgICogZG9tL01lc3NhZ2VQb3J0LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Ok1lc3Nh Z2VQb3J0OjpjbG9zZSk6CisKIDIwMTItMDQtMjggIEFsZXhhbmRlciBQYXZsb3YgIDxhcGF2bG92 QGNocm9taXVtLm9yZz4KIAogICAgICAgICBXZWIgSW5zcGVjdG9yOiBFbmFibGUgdG91Y2ggZXZl bnRzIGZlYXR1cmUgZmFpbHMgdG91Y2ggZmVhdHVyZSBkZXRlY3Rpb24KZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL2RvbS9NZXNzYWdlUG9ydC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9kb20vTWVz c2FnZVBvcnQuY3BwCmluZGV4IGFkZTI0OWM1ZTUyOWFjMWY0NzE1ZWI0YmFhM2ZlMWE4YTMxNzg4 NTEuLmFiNGRjZGMwZDVkODViMzNlYmQ1M2U1YmU4N2NiMGNiNjRjN2U0MzkgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJDb3JlL2RvbS9NZXNzYWdlUG9ydC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUv ZG9tL01lc3NhZ2VQb3J0LmNwcApAQCAtMTMzLDEwICsxMzMsMTAgQEAgdm9pZCBNZXNzYWdlUG9y dDo6c3RhcnQoKQogCiB2b2lkIE1lc3NhZ2VQb3J0OjpjbG9zZSgpCiB7Ci0gICAgbV9jbG9zZWQg PSB0cnVlOwogICAgIGlmICghaXNFbnRhbmdsZWQoKSkKICAgICAgICAgcmV0dXJuOwogICAgIG1f ZW50YW5nbGVkQ2hhbm5lbC0+Y2xvc2UoKTsKKyAgICBtX2Nsb3NlZCA9IHRydWU7CiB9CiAKIHZv aWQgTWVzc2FnZVBvcnQ6OmVudGFuZ2xlKFBhc3NPd25QdHI8TWVzc2FnZVBvcnRDaGFubmVsPiBy ZW1vdGUpCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cgYi9MYXlvdXRUZXN0cy9D aGFuZ2VMb2cKaW5kZXggZDI4N2I2YjVjNzA0MmQ5ZmYwOGMzMDAyOWQ5MTZlZWFmYWEzM2M5OS4u M2JhNGU4ZDY3NTc0NWQyYTJhYzJmYmZlN2I4ZTJmYTQwNTg1YzM0ZCAxMDA2NDQKLS0tIGEvTGF5 b3V0VGVzdHMvQ2hhbmdlTG9nCisrKyBiL0xheW91dFRlc3RzL0NoYW5nZUxvZwpAQCAtMSwzICsx LDE1IEBACisyMDEyLTA0LTI5ICBMaSBZaW4gIDxsaS55aW5AaW50ZWwuY29tPgorCisgICAgICAg IE1lc3NhZ2VQb3J0IG11c3Qgc2V0IG1fY2xvc2VkIHRvIGJlIHRydWUgYXQgdGhlIGVuZCBvZiBN ZXNzYWdlUG9ydDo6Y2xvc2UgZnVuY3Rpb24KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTg1MTM5CisKKyAgICAgICAgVGVzdCBNZXNzYWdlQ2hhbm5lbC5w b3J0IHdoZXRoZXIgY2FuIHJlY2VpdmUgbWVzc2FnZSBhZnRlciBpdCBpcyBjbG9zZWQuCisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBmYXN0L2V2ZW50 cy9tZXNzYWdlLXBvcnQtY2xvc2UtZXhwZWN0ZWQudHh0OiBBZGRlZC4KKyAgICAgICAgKiBmYXN0 L2V2ZW50cy9tZXNzYWdlLXBvcnQtY2xvc2UuaHRtbDogQWRkZWQuCisKIDIwMTItMDQtMjggIFph biBEb2JlcnNlayAgPHphbmRvYmVyc2VrQGdtYWlsLmNvbT4KIAogICAgICAgICBbV0syXSBodHRw L3Rlc3RzL25hdmlnYXRpb24vYW5jaG9yLWZyYW1lcy1nYmsuaHRtbCBmYWlscwpkaWZmIC0tZ2l0 IGEvTGF5b3V0VGVzdHMvZmFzdC9ldmVudHMvbWVzc2FnZS1wb3J0LWNsb3NlLWV4cGVjdGVkLnR4 dCBiL0xheW91dFRlc3RzL2Zhc3QvZXZlbnRzL21lc3NhZ2UtcG9ydC1jbG9zZS1leHBlY3RlZC50 eHQKbmV3IGZpbGUgbW9kZSAxMDA2NDQKaW5kZXggMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMC4uOTJjZmVkY2ViNDc1NTE0N2JhYTQxMGI0NDU0YTQ0ZWEwMDQ3ZWI2MQot LS0gL2Rldi9udWxsCisrKyBiL0xheW91dFRlc3RzL2Zhc3QvZXZlbnRzL21lc3NhZ2UtcG9ydC1j bG9zZS1leHBlY3RlZC50eHQKQEAgLTAsMCArMSw3IEBACitUZXN0IENsb3NlZCBNZXNzYWdlUG9y dCBXaGV0aGVyIFJlY2VpdmUgTWVzc2FnZSBPciBOb3QuCisKK1Nob3VsZCBiZSBhIFNUQVJUIG1l c3NhZ2UsIGZvbGxvd2VkIHdpdGggRE9ORS4KKworU1RBUlQKK0RPTkUKKwpkaWZmIC0tZ2l0IGEv TGF5b3V0VGVzdHMvZmFzdC9ldmVudHMvbWVzc2FnZS1wb3J0LWNsb3NlLmh0bWwgYi9MYXlvdXRU ZXN0cy9mYXN0L2V2ZW50cy9tZXNzYWdlLXBvcnQtY2xvc2UuaHRtbApuZXcgZmlsZSBtb2RlIDEw MDY0NAppbmRleCAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwLi5jYTFj ODg2MGI0YjUzODk3MmExMjE3ZDQyZWNhY2NhZWMwNjBhNjVjCi0tLSAvZGV2L251bGwKKysrIGIv TGF5b3V0VGVzdHMvZmFzdC9ldmVudHMvbWVzc2FnZS1wb3J0LWNsb3NlLmh0bWwKQEAgLTAsMCAr MSwzNiBAQAorPGJvZHk+Cis8cD5UZXN0IENsb3NlZCBNZXNzYWdlUG9ydCBXaGV0aGVyIFJlY2Vp dmUgTWVzc2FnZSBPciBOb3QuPC9wPgorPHA+U2hvdWxkIGJlIGEgU1RBUlQgbWVzc2FnZSwgZm9s bG93ZWQgd2l0aCBET05FLjwvcD4KKzxwcmUgaWQ9bG9nPjwvcHJlPgorPHNjcmlwdD4KK2Z1bmN0 aW9uIGxvZyhtZXNzYWdlKQoreworICAgIGRvY3VtZW50LmdldEVsZW1lbnRCeUlkKCJsb2ciKS5p bm5lckhUTUwgKz0gbWVzc2FnZSArICI8YnI+IjsKK30KKworaWYgKHdpbmRvdy5sYXlvdXRUZXN0 Q29udHJvbGxlcikgeworICAgIGxheW91dFRlc3RDb250cm9sbGVyLmR1bXBBc1RleHQoKTsKKyAg ICBsYXlvdXRUZXN0Q29udHJvbGxlci53YWl0VW50aWxEb25lKCk7Cit9CisKK2xvZygiU1RBUlQi KTsKKwordmFyIGNoYW5uZWwgPSBuZXcgTWVzc2FnZUNoYW5uZWw7CitjaGFubmVsLnBvcnQxLm9u bWVzc2FnZSA9IGZ1bmN0aW9uKGV2dCkgeworICAgIGxvZygiQ2xvc2VkIHBvcnQgcmVjZWl2aW5n OiBGQUlMLiBHb3QgTWVzc2FnZTogIiArICBldnQuZGF0YSArICIgYWZ0ZXIgY2xvc2UiKTsKK30K K2NoYW5uZWwucG9ydDEuY2xvc2UoKTsKK2NoYW5uZWwucG9ydDIucG9zdE1lc3NhZ2UoIlRoaXMg bWVzc2FnZSBjYW4ndCBiZSByZWNlaXZlZCIpOworZG9uZSgpOworCitmdW5jdGlvbiBkb25lKCkg eworICAgIC8vIFdhaXQgYSBzaG9ydCBwZXJpb2Qgb2YgdGltZSB0byBlbnN1cmUgbm8gbWVzc2Fn ZXMgY29tZSBpbiBmcm9tIHByZXZpb3VzIHRlc3RzLgorICAgIHNldFRpbWVvdXQoZnVuY3Rpb24o KSB7CisgICAgICAgIGxvZygiRE9ORSIpOworICAgICAgICBpZiAod2luZG93LmxheW91dFRlc3RD b250cm9sbGVyKQorICAgICAgICAgICAgbGF5b3V0VGVzdENvbnRyb2xsZXIubm90aWZ5RG9uZSgp OworICAgIH0sIDEwMCk7Cit9CisKKzwvc2NyaXB0PgorPC9ib2R5Pgo=