156676 2016-04-17 08:54:14 -0700 [ES6] Use @isObject to check Object Type instead of using instanceof 2016-04-17 14:53:14 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 ysuzuki ysuzuki benjamin commit-queue fpizlo ggaren keith_miller mark.lam msaboff saam oldest_to_newest 1184472 0 ysuzuki 2016-04-17 08:54:14 -0700 [ES6] Use @isObject to check Object Type instead of using instanceof 1184473 1 276591 ysuzuki 2016-04-17 09:07:09 -0700 Created attachment 276591 Patch 1184474 2 ysuzuki 2016-04-17 09:10:29 -0700 BTW, this is why Array.isArray is introduced despite there is `instanceof Array`. 1184478 3 276591 darin 2016-04-17 09:33:25 -0700 Comment on attachment 276591 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276591&action=review I noticed other similar usage of instanceof elsewhere. For example, "instanceof @Number" in toLocaleString. Does that have a similar problem. In WebCore I see "instanceof @Array" in one part, "instanceof Array" in another part, and "instanceof Function" . Do any of those have similar problems? When is it appropriate to use "Array" instead of "@Array"? I would like to understand more about correct ways to write JavaScript implementations both in the JavaScript engine and the DOM implementation. > Source/JavaScriptCore/jsc.cpp:1783 > + return JSValue::encode(GlobalObject::create(exec->vm(), GlobalObject::createStructure(exec->vm(), jsNull()), Vector<String>())); Since exec->vm() does a number of memory accesses, I often wonder whether we should put it in a local variable in cases like this. Not important in the JSC tool I suppose, but important as an idiom elsewhere in WebKit. 1184513 4 ysuzuki 2016-04-17 14:47:24 -0700 (In reply to comment #3) > Comment on attachment 276591 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=276591&action=review > > I noticed other similar usage of instanceof elsewhere. For example, > "instanceof @Number" in toLocaleString. Does that have a similar problem. Yup. That's wrong and they also should be fixed. For `instanceof @Number` case, we should add @thisNumberValue abstract operation, similar to @thisTimeValue for Date. I've opened the issue for that. https://bugs.webkit.org/show_bug.cgi?id=156680 > In WebCore I see "instanceof @Array" in one part, "instanceof Array" in > another part, and "instanceof Function" . Do any of those have similar > problems? When is it appropriate to use "Array" instead of "@Array"? I would > like to understand more about correct ways to write JavaScript > implementations both in the JavaScript engine and the DOM implementation. I think `instanceof @Array` in fetch/FetchHeader.js is a problem. Since it is internal builtin JS, we should take care such edge cases. But I'm not sure mediacontrols should consider these cases. Mediacontrols are used to construct media UI, and it seems that the code is written as usual user-level JS code; it does not use builtins and it directly uses usual APIs (It means that user can replace the called APIs). And for the WebRTC's addIceCandidate() case, I think `candidate instanceof @RTCIceCandidate` is ok; this is because addIceCandidate() does not have any step-by-step spec. The rough implementation for now seems acceptable. > > > Source/JavaScriptCore/jsc.cpp:1783 > > + return JSValue::encode(GlobalObject::create(exec->vm(), GlobalObject::createStructure(exec->vm(), jsNull()), Vector<String>())); > > Since exec->vm() does a number of memory accesses, I often wonder whether we > should put it in a local variable in cases like this. Not important in the > JSC tool I suppose, but important as an idiom elsewhere in WebKit. Thanks, fixed. 1184515 5 ysuzuki 2016-04-17 14:53:14 -0700 Committed r199647: <http://trac.webkit.org/changeset/199647> 276591 2016-04-17 09:07:09 -0700 2016-04-17 09:33:25 -0700 Patch bug-156676-20160417170725.patch text/plain 5442 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMTk5NjM5CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBm OGMwNjM1MjdiNDA3YjQxMTc3ZDM3ZWIxZmRjNGZkNGJiMzkwNGIwLi4zZmQ5ZTdhMjgwZjFmYWUx ZmVmOGE4ZThiOWZiM2Q5MDE0NjU2MGFkIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyNyBAQAorMjAxNi0wNC0xNyAgWXVzdWtlIFN1enVraSAgPHV0YXRhbmUudGVhQGdtYWls LmNvbT4KKworICAgICAgICBbRVM2XSBVc2UgQGlzT2JqZWN0IHRvIGNoZWNrIE9iamVjdCBUeXBl IGluc3RlYWQgb2YgdXNpbmcgaW5zdGFuY2VvZgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU2Njc2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgVXNlIEBpc09iamVjdCBpbnN0ZWFkIG9mIGBpbnN0YW5jZW9m IEBPYmplY3RgLgorICAgICAgICBUaGUgYGluc3RhbmNlb2ZgIGNoZWNrIGlzIG5vdCBlbm91Z2gg dG8gY2hlY2sgT2JqZWN0IFR5cGUuCisgICAgICAgIEZvciBleGFtcGxlLCBnaXZlbiAyIHJlYWxt cywgdGhlIG9iamVjdCBjcmVhdGVkIGluIG9uZSByZWFsbSBkb2VzIG5vdCBpbmhlcml0IHRoZSBP YmplY3Qgb2YgYW5vdGhlciByZWFsbS4KKyAgICAgICAgQW5vdGhlciBleGFtcGxlIGlzIHRoYXQg dGhlIG9iamVjdCB3aGljaCBkb2VzIG5vdCBpbmhlcml0IE9iamVjdC4KKyAgICAgICAgVGhpcyBv YmplY3QgY2FuIGJlIGVhc2lseSBjcmVhdGVkIGJ5IGNhbGxpbmcgYE9iamVjdC5jcmVhdGUobnVs bClgLgorCisgICAgICAgICogYnVpbHRpbnMvUmVnRXhwUHJvdG90eXBlLmpzOgorICAgICAgICAo bWF0Y2gpOgorICAgICAgICAqIGpzYy5jcHA6CisgICAgICAgIChHbG9iYWxPYmplY3Q6OmZpbmlz aENyZWF0aW9uKToKKyAgICAgICAgKGZ1bmN0aW9uQ3JlYXRlR2xvYmFsT2JqZWN0KToKKyAgICAg ICAgKiB0ZXN0cy9zdHJlc3MvcmVnZXhwLW1hdGNoLWluLW90aGVyLXJlYWxtLXNob3VsZC13b3Jr LmpzOiBBZGRlZC4KKyAgICAgICAgKHNob3VsZEJlKToKKyAgICAgICAgKiB0ZXN0cy9zdHJlc3Mv cmVnZXhwLW1hdGNoLXNob3VsZC13b3JrLXdpdGgtb2JqZWN0cy1ub3QtaW5oZXJpdGluZy1vYmpl Y3QtcHJvdG90eXBlLmpzOiBBZGRlZC4KKyAgICAgICAgKHNob3VsZEJlKToKKyAgICAgICAgKHJl Z2V4cC5leGVjKToKKwogMjAxNi0wNC0xNiAgQmVuamFtaW4gUG91bGFpbiAgPGJwb3VsYWluQHdl YmtpdC5vcmc+CiAKICAgICAgICAgW0pTQ10gREZHIHNob3VsZCBzdXBwb3J0IHJlbGF0aW9uYWwg Y29tcGFyaXNvbnMgb2YgTnVtYmVyIGFuZCBPdGhlcgpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFT Y3JpcHRDb3JlL2J1aWx0aW5zL1JlZ0V4cFByb3RvdHlwZS5qcyBiL1NvdXJjZS9KYXZhU2NyaXB0 Q29yZS9idWlsdGlucy9SZWdFeHBQcm90b3R5cGUuanMKaW5kZXggMWI2MDI2YjE5MGNlZDUwYThi ZjMxMTcxZTM4Y2RmZWJkOGY3MDliZS4uYThkZjMzMDczZTUyZjljZjc0MGE5MTUxZGNlNjU4NWJk ZWQ4OTUxMSAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2J1aWx0aW5zL1JlZ0V4 cFByb3RvdHlwZS5qcworKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvYnVpbHRpbnMvUmVnRXhw UHJvdG90eXBlLmpzCkBAIC00Nyw3ICs0Nyw3IEBAIGZ1bmN0aW9uIG1hdGNoKHN0cikKIHsKICAg ICAidXNlIHN0cmljdCI7CiAKLSAgICBpZiAoISh0aGlzIGluc3RhbmNlb2YgQE9iamVjdCkpCisg ICAgaWYgKCFAaXNPYmplY3QodGhpcykpCiAgICAgICAgIHRocm93IG5ldyBAVHlwZUVycm9yKCJS ZWdFeHAucHJvdG90eXBlLkBAbWF0Y2ggcmVxdWlyZXMgdGhhdCB8dGhpc3wgYmUgYW4gT2JqZWN0 Iik7CiAKICAgICBsZXQgcmVnZXhwID0gQE9iamVjdCh0aGlzKTsKZGlmZiAtLWdpdCBhL1NvdXJj ZS9KYXZhU2NyaXB0Q29yZS9qc2MuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2pzYy5jcHAK aW5kZXggMGEzNDAwYTg5MDFlZmVjZTRmMjIyZjcxNDhjOWI2NjI2ZWU2ZDJmYy4uOTRkZDFhMGU1 MjU4NTE3MjYzYmEwNWE5ZTk5ZjlhYmNlMzM1MzdlNyAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFT Y3JpcHRDb3JlL2pzYy5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2pzYy5jcHAKQEAg LTU1NSw2ICs1NTUsNyBAQCBzdGF0aWMgRW5jb2RlZEpTVmFsdWUgSlNDX0hPU1RfQ0FMTCBmdW5j dGlvbkNyZWF0ZVJ1bnRpbWVBcnJheShFeGVjU3RhdGUqKTsKIHN0YXRpYyBFbmNvZGVkSlNWYWx1 ZSBKU0NfSE9TVF9DQUxMIGZ1bmN0aW9uQ3JlYXRlSW1wdXJlR2V0dGVyKEV4ZWNTdGF0ZSopOwog c3RhdGljIEVuY29kZWRKU1ZhbHVlIEpTQ19IT1NUX0NBTEwgZnVuY3Rpb25DcmVhdGVDdXN0b21H ZXR0ZXJPYmplY3QoRXhlY1N0YXRlKik7CiBzdGF0aWMgRW5jb2RlZEpTVmFsdWUgSlNDX0hPU1Rf Q0FMTCBmdW5jdGlvbkNyZWF0ZUJ1aWx0aW4oRXhlY1N0YXRlKik7CitzdGF0aWMgRW5jb2RlZEpT VmFsdWUgSlNDX0hPU1RfQ0FMTCBmdW5jdGlvbkNyZWF0ZUdsb2JhbE9iamVjdChFeGVjU3RhdGUq KTsKIHN0YXRpYyBFbmNvZGVkSlNWYWx1ZSBKU0NfSE9TVF9DQUxMIGZ1bmN0aW9uU2V0SW1wdXJl R2V0dGVyRGVsZWdhdGUoRXhlY1N0YXRlKik7CiAKIHN0YXRpYyBFbmNvZGVkSlNWYWx1ZSBKU0Nf SE9TVF9DQUxMIGZ1bmN0aW9uU2V0RWxlbWVudFJvb3QoRXhlY1N0YXRlKik7CkBAIC04MDAsNiAr ODAxLDcgQEAgY2xhc3MgR2xvYmFsT2JqZWN0IDogcHVibGljIEpTR2xvYmFsT2JqZWN0IHsKICAg ICAgICAgYWRkRnVuY3Rpb24odm0sICJjcmVhdGVJbXB1cmVHZXR0ZXIiLCBmdW5jdGlvbkNyZWF0 ZUltcHVyZUdldHRlciwgMSk7CiAgICAgICAgIGFkZEZ1bmN0aW9uKHZtLCAiY3JlYXRlQ3VzdG9t R2V0dGVyT2JqZWN0IiwgZnVuY3Rpb25DcmVhdGVDdXN0b21HZXR0ZXJPYmplY3QsIDApOwogICAg ICAgICBhZGRGdW5jdGlvbih2bSwgImNyZWF0ZUJ1aWx0aW4iLCBmdW5jdGlvbkNyZWF0ZUJ1aWx0 aW4sIDIpOworICAgICAgICBhZGRGdW5jdGlvbih2bSwgImNyZWF0ZUdsb2JhbE9iamVjdCIsIGZ1 bmN0aW9uQ3JlYXRlR2xvYmFsT2JqZWN0LCAwKTsKICAgICAgICAgYWRkRnVuY3Rpb24odm0sICJz ZXRJbXB1cmVHZXR0ZXJEZWxlZ2F0ZSIsIGZ1bmN0aW9uU2V0SW1wdXJlR2V0dGVyRGVsZWdhdGUs IDIpOwogCiAgICAgICAgIGFkZEZ1bmN0aW9uKHZtLCAiZHVtcFR5cGVzRm9yQWxsVmFyaWFibGVz IiwgZnVuY3Rpb25EdW1wVHlwZXNGb3JBbGxWYXJpYWJsZXMgLCAwKTsKQEAgLTE3NzYsNiArMTc3 OCwxMSBAQCBFbmNvZGVkSlNWYWx1ZSBKU0NfSE9TVF9DQUxMIGZ1bmN0aW9uQ3JlYXRlQnVpbHRp bihFeGVjU3RhdGUqIGV4ZWMpCiAgICAgcmV0dXJuIEpTVmFsdWU6OmVuY29kZShmdW5jKTsKIH0K IAorRW5jb2RlZEpTVmFsdWUgSlNDX0hPU1RfQ0FMTCBmdW5jdGlvbkNyZWF0ZUdsb2JhbE9iamVj dChFeGVjU3RhdGUqIGV4ZWMpCit7CisgICAgcmV0dXJuIEpTVmFsdWU6OmVuY29kZShHbG9iYWxP YmplY3Q6OmNyZWF0ZShleGVjLT52bSgpLCBHbG9iYWxPYmplY3Q6OmNyZWF0ZVN0cnVjdHVyZShl eGVjLT52bSgpLCBqc051bGwoKSksIFZlY3RvcjxTdHJpbmc+KCkpKTsKK30KKwogRW5jb2RlZEpT VmFsdWUgSlNDX0hPU1RfQ0FMTCBmdW5jdGlvbkNoZWNrTW9kdWxlU3ludGF4KEV4ZWNTdGF0ZSog ZXhlYykKIHsKICAgICBTdHJpbmcgc291cmNlID0gZXhlYy0+YXJndW1lbnQoMCkudG9TdHJpbmco ZXhlYyktPnZhbHVlKGV4ZWMpOwpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3Rl c3RzL3N0cmVzcy9yZWdleHAtbWF0Y2gtaW4tb3RoZXItcmVhbG0tc2hvdWxkLXdvcmsuanMgYi9T b3VyY2UvSmF2YVNjcmlwdENvcmUvdGVzdHMvc3RyZXNzL3JlZ2V4cC1tYXRjaC1pbi1vdGhlci1y ZWFsbS1zaG91bGQtd29yay5qcwpuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwLi4zYTljOGU4ZTZkY2JjMTU4NzNiM2MyOWFh YTA4MzgyZDNiZjM2MGQ2Ci0tLSAvZGV2L251bGwKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3Jl L3Rlc3RzL3N0cmVzcy9yZWdleHAtbWF0Y2gtaW4tb3RoZXItcmVhbG0tc2hvdWxkLXdvcmsuanMK QEAgLTAsMCArMSwxMCBAQAorZnVuY3Rpb24gc2hvdWxkQmUoYWN0dWFsLCBleHBlY3RlZCkgewor ICAgIGlmIChhY3R1YWwgIT09IGV4cGVjdGVkKQorICAgICAgICB0aHJvdyBuZXcgRXJyb3IoYGJh ZCB2YWx1ZTogJHtTdHJpbmcoYWN0dWFsKX1gKTsKK30KKwordmFyIHJlZ2V4cCA9IC9IZWxsby87 Cit2YXIgc3RyaW5nID0gIkhlbGxvIjsKK3ZhciBvdGhlclJlYWxtID0gY3JlYXRlR2xvYmFsT2Jq ZWN0KCk7CitzaG91bGRCZShvdGhlclJlYWxtLlJlZ0V4cC5wcm90b3R5cGVbU3ltYm9sLm1hdGNo XS5jYWxsKHJlZ2V4cCwgc3RyaW5nKVswXSwgc3RyaW5nKQorCmRpZmYgLS1naXQgYS9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvdGVzdHMvc3RyZXNzL3JlZ2V4cC1tYXRjaC1zaG91bGQtd29yay13aXRo LW9iamVjdHMtbm90LWluaGVyaXRpbmctb2JqZWN0LXByb3RvdHlwZS5qcyBiL1NvdXJjZS9KYXZh U2NyaXB0Q29yZS90ZXN0cy9zdHJlc3MvcmVnZXhwLW1hdGNoLXNob3VsZC13b3JrLXdpdGgtb2Jq ZWN0cy1ub3QtaW5oZXJpdGluZy1vYmplY3QtcHJvdG90eXBlLmpzCm5ldyBmaWxlIG1vZGUgMTAw NjQ0CmluZGV4IDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAuLmEwNDc2 MjM0ZDQ2ODc0ZjM0MzY3NGY1ZTBjYjAxMTk1NzFlZGFmZmUKLS0tIC9kZXYvbnVsbAorKysgYi9T b3VyY2UvSmF2YVNjcmlwdENvcmUvdGVzdHMvc3RyZXNzL3JlZ2V4cC1tYXRjaC1zaG91bGQtd29y ay13aXRoLW9iamVjdHMtbm90LWluaGVyaXRpbmctb2JqZWN0LXByb3RvdHlwZS5qcwpAQCAtMCww ICsxLDExIEBACitmdW5jdGlvbiBzaG91bGRCZShhY3R1YWwsIGV4cGVjdGVkKSB7CisgICAgaWYg KGFjdHVhbCAhPT0gZXhwZWN0ZWQpCisgICAgICAgIHRocm93IG5ldyBFcnJvcihgYmFkIHZhbHVl OiAke1N0cmluZyhhY3R1YWwpfWApOworfQordmFyIHJlZ2V4cCA9IE9iamVjdC5jcmVhdGUobnVs bCk7CityZWdleHAucmVnID0gL0hlbGxvLzsKK3JlZ2V4cC5leGVjID0gZnVuY3Rpb24gKHZhbHVl KSB7CisgICAgcmV0dXJuIHJlZ2V4cC5yZWcuZXhlYyh2YWx1ZSk7Cit9OwordmFyIHN0cmluZyA9 ICJIZWxsbyI7CitzaG91bGRCZShSZWdFeHAucHJvdG90eXBlW1N5bWJvbC5tYXRjaF0uY2FsbChy ZWdleHAsIHN0cmluZylbMF0sIHN0cmluZykK