102870 2012-11-20 21:37:43 -0800 [V8] Replace V8Parameter::prepare() with V8Parameter::operator=() 2012-11-21 01:08:46 -0800 1 1 1 Unclassified WebKit WebCore JavaScript 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 haraken haraken abarth japhet webkit.review.bot oldest_to_newest 772449 0 haraken 2012-11-20 21:37:43 -0800 This is an incremental step for V8Parameter refactoring. By replacing V8Parameter::prepare() with V8Parameter::operator=(), we can make EXCEPTION_BLOCK() and STRING_TO_V8PARAMETER_EXCEPTION_BLOCK() equivalent (except for a return value). In the next patch, I will remove STRING_TO_V8PARAMETER_EXCEPTION_BLOCK(). 772450 1 175334 haraken 2012-11-20 21:39:14 -0800 Created attachment 175334 Patch 772537 2 175334 abarth 2012-11-21 00:17:44 -0800 Comment on attachment 175334 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175334&action=review > Source/WebCore/bindings/v8/V8BindingMacros.h:71 > - type var(value); \ > + type var; \ > { \ > v8::TryCatch block; \ > - var.prepare(); \ > + var = (value); \ Does it matter that you're moving the evaluation of (value) inside the TryCatch block? Perhaps it matters if evaluating (value) throws an exception? 772552 3 haraken 2012-11-21 00:28:06 -0800 (In reply to comment #2) > > - type var(value); \ > > + type var; \ > > { \ > > v8::TryCatch block; \ > > - var.prepare(); \ > > + var = (value); \ > > Does it matter that you're moving the evaluation of (value) inside the TryCatch block? Perhaps it matters if evaluating (value) throws an exception? '=' throws an exception. 772559 4 abarth 2012-11-21 00:31:01 -0800 > '=' throws an exception. I understand that. The issue is that evaluating (value) itself might throw and exception. Because this is a macro and not an inline function, you've changed the time at which the expression (value) is evaluated. Now it is evaluated in the context of the try block. 772563 5 haraken 2012-11-21 00:37:03 -0800 (In reply to comment #4) > > '=' throws an exception. > > I understand that. The issue is that evaluating (value) itself might throw and exception. Because this is a macro and not an inline function, you've changed the time at which the expression (value) is evaluated. Now it is evaluated in the context of the try block. Ah, got it... So, to guarantee that this patch won't change any behavior, I should check that there is no STRING_TO_V8PARAMETER_EXCEPTION_BLOCK() that is inside any TryCatch block, right? c.f. EXCEPTION_BLOCK() evaluates (value) inside the try block. 772566 6 abarth 2012-11-21 00:38:31 -0800 > Ah, got it... So, to guarantee that this patch won't change any behavior, I should check that there is no STRING_TO_V8PARAMETER_EXCEPTION_BLOCK() that is inside any TryCatch block, right? > > c.f. EXCEPTION_BLOCK() evaluates (value) inside the try block. I doubt it's a problem, I just wanted to make sure you were aware of the (small) change you were making to the behavior. 772570 7 haraken 2012-11-21 00:42:47 -0800 (In reply to comment #6) > > Ah, got it... So, to guarantee that this patch won't change any behavior, I should check that there is no STRING_TO_V8PARAMETER_EXCEPTION_BLOCK() that is inside any TryCatch block, right? > > > > c.f. EXCEPTION_BLOCK() evaluates (value) inside the try block. > > I doubt it's a problem, I just wanted to make sure you were aware of the (small) change you were making to the behavior. Thanks. Currently there is no (STRING_TO_V8PARAMETER_)EXCEPTION_BLOCK() that is inside a TryCatch block (Note: Due to a V8 bug (http://code.google.com/p/v8/issues/detail?id=2166), currently we cannot nest TryCatch blocks), so this change would be safe. 772588 8 175334 webkit.review.bot 2012-11-21 01:08:43 -0800 Comment on attachment 175334 Patch Clearing flags on attachment: 175334 Committed r135359: <http://trac.webkit.org/changeset/135359> 772589 9 webkit.review.bot 2012-11-21 01:08:46 -0800 All reviewed patches have been landed. Closing bug. 175334 2012-11-20 21:39:14 -0800 2012-11-21 01:08:42 -0800 Patch bug-102870-20121121143702.patch text/plain 5609 haraken U3VidmVyc2lvbiBSZXZpc2lvbjogMTM1MzQ2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZDY0ODcwNmFiMzJkMmM1 MmJhOTFkNjYzM2I4ZDU5YTRhMmNhNDc4Ny4uMWI1YzE5NTEzZmVlYzkxYjFlZjZjMGY5YmIwM2I3 MTg4MmRlZWIzMiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDI4IEBACiAyMDEyLTExLTIwICBLZW50 YXJvIEhhcmEgIDxoYXJha2VuQGNocm9taXVtLm9yZz4KIAorICAgICAgICBbVjhdIFJlcGxhY2Ug VjhQYXJhbWV0ZXI6OnByZXBhcmUoKSB3aXRoIFY4UGFyYW1ldGVyOjpvcGVyYXRvcj0oKQorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAyODcwCisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhpcyBpcyBhbiBp bmNyZW1lbnRhbCBzdGVwIGZvciBWOFBhcmFtZXRlciByZWZhY3RvcmluZy4KKyAgICAgICAgQnkg cmVwbGFjaW5nIFY4UGFyYW1ldGVyOjpwcmVwYXJlKCkgd2l0aCBWOFBhcmFtZXRlcjo6b3BlcmF0 b3I9KCksCisgICAgICAgIHdlIGNhbiBtYWtlIEVYQ0VQVElPTl9CTE9DSygpIGFuZCBTVFJJTkdf VE9fVjhQQVJBTUVURVJfRVhDRVBUSU9OX0JMT0NLKCkKKyAgICAgICAgZXF1aXZhbGVudCAoZXhj ZXB0IGZvciBhIHJldHVybiB2YWx1ZSkuCisKKyAgICAgICAgVGVzdHM6IGZhc3Qvd29ya2Vycy93 b3JrZXItY29uc3RydWN0b3IuaHRtbAorICAgICAgICAgICAgICAgc3RvcmFnZS93ZWJzcWwvc3Fs LWVycm9yLWNvZGVzLmh0bWwKKyAgICAgICAgICAgICAgIGluc3BlY3Rvci9jb25zb2xlL2FsZXJ0 LXRvU3RyaW5nLWV4Y2VwdGlvbi5odG1sCisgICAgICAgICAgICAgICBodHRwL3Rlc3RzL3dlYnNv Y2tldC90ZXN0cy9oeWJpL3NlbmQtdGhyb3cuaHRtbAorCisgICAgICAgICogYmluZGluZ3Mvdjgv VjhCaW5kaW5nTWFjcm9zLmg6CisgICAgICAgICogYmluZGluZ3MvdjgvVjhTdHJpbmdSZXNvdXJj ZS5oOgorICAgICAgICAoV2ViQ29yZTo6VjhQYXJhbWV0ZXI6OlY4UGFyYW1ldGVyKToKKyAgICAg ICAgKFY4UGFyYW1ldGVyKToKKyAgICAgICAgKFdlYkNvcmU6Oj0pOgorCisyMDEyLTExLTIwICBL ZW50YXJvIEhhcmEgIDxoYXJha2VuQGNocm9taXVtLm9yZz4KKwogICAgICAgICBbVjhdIEludHJv ZHVjZSBjb25zdHJ1Y3RvckNhbGxiYWNrQ3VzdG9tKCkKICAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwMjc2MwogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2Vi Q29yZS9iaW5kaW5ncy92OC9WOEJpbmRpbmdNYWNyb3MuaCBiL1NvdXJjZS9XZWJDb3JlL2JpbmRp bmdzL3Y4L1Y4QmluZGluZ01hY3Jvcy5oCmluZGV4IGIyYmNjYTFkMzRiNDdmY2E5N2VmNjRlZjBk ZmQ2MDJiN2M3YmJkMzMuLmYzODA4Y2E1ZjY3ODNhMDk0MjZmMjUyMTA2MzJmOWI0MTdjZDNiYjEg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4QmluZGluZ01hY3Jvcy5o CisrKyBiL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4QmluZGluZ01hY3Jvcy5oCkBAIC01 NCwxMCArNTQsMTAgQEAgZW51bSBQYXJhbWV0ZXJEZWZhdWx0UG9saWN5IHsKICAgICB9CiAKICNk ZWZpbmUgU1RSSU5HX1RPX1Y4UEFSQU1FVEVSX0VYQ0VQVElPTl9CTE9DSyh0eXBlLCB2YXIsIHZh bHVlKSBcCi0gICAgdHlwZSB2YXIodmFsdWUpOyAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgXAorICAgIHR5cGUgdmFyOyAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgIFwKICAgICB7ICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBcCiAgICAgICAgIHY4OjpUcnlDYXRj aCBibG9jazsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgXAotICAgICAgICB2 YXIucHJlcGFyZSgpOyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIFwK KyAgICAgICAgdmFyID0gKHZhbHVlKTsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICBcCiAgICAgICAgIGlmIChibG9jay5IYXNDYXVnaHQoKSkgeyAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgXAogICAgICAgICAgICAgYmxvY2suUmVUaHJvdygpOyAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgIFwKICAgICAgICAgICAgIHJldHVybiB2ODo6VW5k ZWZpbmVkKCk7ICAgICAgICAgICAgICAgICAgICAgICAgICAgICBcCkBAIC02NSwxMCArNjUsMTAg QEAgZW51bSBQYXJhbWV0ZXJEZWZhdWx0UG9saWN5IHsKICAgICB9CiAKICNkZWZpbmUgU1RSSU5H X1RPX1Y4UEFSQU1FVEVSX0VYQ0VQVElPTl9CTE9DS19WT0lEKHR5cGUsIHZhciwgdmFsdWUpIFwK LSAgICB0eXBlIHZhcih2YWx1ZSk7ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgIFwKKyAgICB0eXBlIHZhcjsgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgIFwKICAgICB7ICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIFwKICAgICAgICAgdjg6 OlRyeUNhdGNoIGJsb2NrOyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg IFwKLSAgICAgICAgdmFyLnByZXBhcmUoKTsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgIFwKKyAgICAgICAgdmFyID0gKHZhbHVlKTsgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIFwKICAgICAgICAgaWYgKGJsb2NrLkhhc0Nh dWdodCgpKSB7ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIFwKICAgICAgICAg ICAgIGJsb2NrLlJlVGhyb3coKTsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgIFwKICAgICAgICAgICAgIHJldHVybjsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgIFwKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdz L3Y4L1Y4U3RyaW5nUmVzb3VyY2UuaCBiL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4U3Ry aW5nUmVzb3VyY2UuaAppbmRleCAyMGY4NjdhNWQ1OTFhMzU0YmEwZWE4YmRlYmRhYmJjYWQ2ZTc5 YTI4Li5kMTc5OTViYzZmNTIxODBkZmMzNTYyODEyODQ4YzhiZDI1ODE5MTRiIDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy92OC9WOFN0cmluZ1Jlc291cmNlLmgKKysrIGIvU291 cmNlL1dlYkNvcmUvYmluZGluZ3MvdjgvVjhTdHJpbmdSZXNvdXJjZS5oCkBAIC01MiwxNiArNTIs MTUgQEAgZW51bSBWOFBhcmFtZXRlck1vZGUgewogdGVtcGxhdGUgPFY4UGFyYW1ldGVyTW9kZSBN b2RlID0gRGVmYXVsdE1vZGU+CiBjbGFzcyBWOFBhcmFtZXRlciB7CiBwdWJsaWM6Ci0gICAgVjhQ YXJhbWV0ZXIodjg6OkxvY2FsPHY4OjpWYWx1ZT4gb2JqZWN0KQotICAgICAgICA6IG1fdjhPYmpl Y3Qob2JqZWN0KQotICAgICAgICAsIG1fbW9kZShFeHRlcm5hbGl6ZSkKKyAgICBWOFBhcmFtZXRl cigpCisgICAgICAgIDogbV9tb2RlKEV4dGVybmFsaXplKQogICAgICAgICAsIG1fc3RyaW5nKCkK ICAgICB7CiAgICAgfQogCiAgICAgLy8gVGhpcyBjYW4gdGhyb3cuIFlvdSBtdXN0IHVzZSB0aGlz IHRocm91Z2ggdGhlCiAgICAgLy8gU1RSSU5HX1RPX1Y4UEFSQU1FVEVSX0VYQ0VQVElPTl9CTE9D SygpIG1hY3JvLgotICAgIHZvaWQgcHJlcGFyZSgpOworICAgIFY4UGFyYW1ldGVyJiBvcGVyYXRv cj0odjg6OkxvY2FsPHY4OjpWYWx1ZT4pOwogICAgIG9wZXJhdG9yIFN0cmluZygpIHsgcmV0dXJu IHRvU3RyaW5nPFN0cmluZz4oKTsgfQogICAgIG9wZXJhdG9yIEF0b21pY1N0cmluZygpIHsgcmV0 dXJuIHRvU3RyaW5nPEF0b21pY1N0cmluZz4oKTsgfQogCkBAIC04Myw4ICs4Miw2IEBAIHByaXZh dGU6CiAgICAgICAgIG1fdjhPYmplY3QgPSBtX3Y4T2JqZWN0LT5Ub1N0cmluZygpOwogICAgIH0K IAotICAgIHY4OjpMb2NhbDx2ODo6VmFsdWU+IG9iamVjdCgpIHsgcmV0dXJuIG1fdjhPYmplY3Q7 IH0KLQogICAgIHZvaWQgc2V0U3RyaW5nKGNvbnN0IFN0cmluZyYgc3RyaW5nKQogICAgIHsKICAg ICAgICAgbV9zdHJpbmcgPSBzdHJpbmc7CkBAIC0xMDUsMjcgKzEwMiwzMSBAQCBwcml2YXRlOgog ICAgIFN0cmluZyBtX3N0cmluZzsKIH07CiAKLXRlbXBsYXRlPD4gaW5saW5lIHZvaWQgVjhQYXJh bWV0ZXI8RGVmYXVsdE1vZGU+OjpwcmVwYXJlKCkKK3RlbXBsYXRlPD4gaW5saW5lIFY4UGFyYW1l dGVyPERlZmF1bHRNb2RlPiYgVjhQYXJhbWV0ZXI8RGVmYXVsdE1vZGU+OjpvcGVyYXRvcj0odjg6 OkxvY2FsPHY4OjpWYWx1ZT4gb2JqZWN0KQogeworICAgIG1fdjhPYmplY3QgPSBvYmplY3Q7CiAg ICAgcHJlcGFyZUJhc2UoKTsKKyAgICByZXR1cm4gKnRoaXM7CiB9CiAKLXRlbXBsYXRlPD4gaW5s aW5lIHZvaWQgVjhQYXJhbWV0ZXI8V2l0aE51bGxDaGVjaz46OnByZXBhcmUoKQordGVtcGxhdGU8 PiBpbmxpbmUgVjhQYXJhbWV0ZXI8V2l0aE51bGxDaGVjaz4mIFY4UGFyYW1ldGVyPFdpdGhOdWxs Q2hlY2s+OjpvcGVyYXRvcj0odjg6OkxvY2FsPHY4OjpWYWx1ZT4gb2JqZWN0KQogewotICAgIGlm IChvYmplY3QoKS5Jc0VtcHR5KCkgfHwgb2JqZWN0KCktPklzTnVsbCgpKSB7CisgICAgbV92OE9i amVjdCA9IG9iamVjdDsKKyAgICBpZiAobV92OE9iamVjdC5Jc0VtcHR5KCkgfHwgbV92OE9iamVj dC0+SXNOdWxsKCkpCiAgICAgICAgIHNldFN0cmluZyhTdHJpbmcoKSk7Ci0gICAgICAgIHJldHVy bjsKLSAgICB9Ci0gICAgcHJlcGFyZUJhc2UoKTsKKyAgICBlbHNlCisgICAgICAgIHByZXBhcmVC YXNlKCk7CisgICAgcmV0dXJuICp0aGlzOwogfQogCi10ZW1wbGF0ZTw+IGlubGluZSB2b2lkIFY4 UGFyYW1ldGVyPFdpdGhVbmRlZmluZWRPck51bGxDaGVjaz46OnByZXBhcmUoKQordGVtcGxhdGU8 PiBpbmxpbmUgVjhQYXJhbWV0ZXI8V2l0aFVuZGVmaW5lZE9yTnVsbENoZWNrPiYgVjhQYXJhbWV0 ZXI8V2l0aFVuZGVmaW5lZE9yTnVsbENoZWNrPjo6b3BlcmF0b3I9KHY4OjpMb2NhbDx2ODo6VmFs dWU+IG9iamVjdCkKIHsKLSAgICBpZiAob2JqZWN0KCkuSXNFbXB0eSgpIHx8IG9iamVjdCgpLT5J c051bGwoKSB8fCBvYmplY3QoKS0+SXNVbmRlZmluZWQoKSkgeworICAgIG1fdjhPYmplY3QgPSBv YmplY3Q7CisgICAgaWYgKG1fdjhPYmplY3QuSXNFbXB0eSgpIHx8IG1fdjhPYmplY3QtPklzTnVs bCgpIHx8IG1fdjhPYmplY3QtPklzVW5kZWZpbmVkKCkpCiAgICAgICAgIHNldFN0cmluZyhTdHJp bmcoKSk7Ci0gICAgICAgIHJldHVybjsKLSAgICB9Ci0gICAgcHJlcGFyZUJhc2UoKTsKKyAgICBl bHNlCisgICAgICAgIHByZXBhcmVCYXNlKCk7CisgICAgcmV0dXJuICp0aGlzOwogfQogICAgIAog fSAvLyBuYW1lc3BhY2UgV2ViQ29yZQo=