123644 2013-11-01 15:49:07 -0700 Removing unnecessary early returns in addTrack, removeTrack and removeRemoteSource methods 2013-11-01 18:19:05 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 123648 1 thiago.lacerda thiago.lacerda commit-queue eric.carlson glenn hta jer.noble tommyw oldest_to_newest 946068 0 thiago.lacerda 2013-11-01 15:49:07 -0700 We want it fail if an invalid vector of tracks is returned from getTrackVectorForType. ASSERT_NOT_REACHED in getTrackVectorForType is taking care of that. 946069 1 215772 thiago.lacerda 2013-11-01 15:51:32 -0700 Created attachment 215772 Patch 946081 2 215772 darin 2013-11-01 16:06:59 -0700 Comment on attachment 215772 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215772&action=review r=me because I think this change is OK, but I think we can do better > Source/WebCore/Modules/mediastream/MediaStream.cpp:166 > MediaStreamTrackVector* tracks = getTrackVectorForType(track->source()->type()); If this function is guaranteed to never return null, then it should return a reference rather than a pointer. Also, in our coding style we do not use the word “get” for a function with a return value. I’d fix those two things and then it will be more obvious that this change is correct. 946092 3 thiago.lacerda 2013-11-01 16:30:19 -0700 (In reply to comment #2) > (From update of attachment 215772 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215772&action=review > > r=me because I think this change is OK, but I think we can do better > > > Source/WebCore/Modules/mediastream/MediaStream.cpp:166 > > MediaStreamTrackVector* tracks = getTrackVectorForType(track->source()->type()); > > If this function is guaranteed to never return null, then it should return a reference rather than a pointer. Also, in our coding style we do not use the word “get” for a function with a return value. I’d fix those two things and then it will be more obvious that this change is correct. It is supposed to never return null. If for some reason it returns null the failure must be made clear, that is why there is an ASSERT_NOT_REACHED in the last switch case. That is why I'm not return a reference. Regarding the method name, I will change it 946101 4 215777 thiago.lacerda 2013-11-01 16:39:54 -0700 Created attachment 215777 Patch 946137 5 215777 commit-queue 2013-11-01 18:19:03 -0700 Comment on attachment 215777 Patch Clearing flags on attachment: 215777 Committed r158470: <http://trac.webkit.org/changeset/158470> 946138 6 commit-queue 2013-11-01 18:19:05 -0700 All reviewed patches have been landed. Closing bug. 215772 2013-11-01 15:51:32 -0700 2013-11-01 16:39:50 -0700 Patch bug-123644-20131101195130.patch text/plain 2186 thiago.lacerda U3VidmVyc2lvbiBSZXZpc2lvbjogMTU4NDYxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggM2YwNTFhZmI3ZjEyMmU0 YmNjOWJlMTdiODM1NWE1MjhkZTgxNGQ4MS4uMDJiNzQzMmU5MTJiZjliYWNkNTkzMjc2M2ExZGQy YjNmZmYxMjliYSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEzLTExLTAxICBUaGlh Z28gZGUgQmFycm9zIExhY2VyZGEgIDx0aGlhZ28ubGFjZXJkYUBvcGVuYm9zc2Eub3JnPgorCisg ICAgICAgIFJlbW92aW5nIHVubmVjZXNzYXJ5IGVhcmx5IHJldHVybnMgaW4gYWRkVHJhY2ssIHJl bW92ZVRyYWNrIGFuZCByZW1vdmVSZW1vdGVTb3VyY2UgbWV0aG9kcworICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTIzNjQ0CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzIG5lZWRlZC4KKwor ICAgICAgICAqIE1vZHVsZXMvbWVkaWFzdHJlYW0vTWVkaWFTdHJlYW0uY3BwOgorICAgICAgICAo V2ViQ29yZTo6TWVkaWFTdHJlYW06OmFkZFRyYWNrKToKKyAgICAgICAgKFdlYkNvcmU6Ok1lZGlh U3RyZWFtOjpyZW1vdmVUcmFjayk6CisgICAgICAgIChXZWJDb3JlOjpNZWRpYVN0cmVhbTo6cmVt b3ZlUmVtb3RlU291cmNlKToKKwogMjAxMy0xMS0wMSAgQW5kcmVhcyBLbGluZyAgPGFrbGluZ0Bh cHBsZS5jb20+CiAKICAgICAgICAgS2lsbCBSZW5kZXJBcmVuYS4KZGlmZiAtLWdpdCBhL1NvdXJj ZS9XZWJDb3JlL01vZHVsZXMvbWVkaWFzdHJlYW0vTWVkaWFTdHJlYW0uY3BwIGIvU291cmNlL1dl YkNvcmUvTW9kdWxlcy9tZWRpYXN0cmVhbS9NZWRpYVN0cmVhbS5jcHAKaW5kZXggZDdlNzViODcz MTFkNzdmMzM2MzQ1NjAwMmNhODlkMGU1OGM2NDg4ZS4uMzBlYWYyNmIxNTY5NjM1MmI2YjRlMDQx OGVlYjRkYTgzM2ViYTcyNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvTW9kdWxlcy9tZWRp YXN0cmVhbS9NZWRpYVN0cmVhbS5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvTW9kdWxlcy9tZWRp YXN0cmVhbS9NZWRpYVN0cmVhbS5jcHAKQEAgLTE2NSw5ICsxNjUsNiBAQCBib29sIE1lZGlhU3Ry ZWFtOjphZGRUcmFjayhQYXNzUmVmUHRyPE1lZGlhU3RyZWFtVHJhY2s+IHBycFRyYWNrKQogCiAg ICAgTWVkaWFTdHJlYW1UcmFja1ZlY3RvciogdHJhY2tzID0gZ2V0VHJhY2tWZWN0b3JGb3JUeXBl KHRyYWNrLT5zb3VyY2UoKS0+dHlwZSgpKTsKIAotICAgIGlmICghdHJhY2tzKQotICAgICAgICBy ZXR1cm4gZmFsc2U7Ci0KICAgICB0cmFja3MtPmFwcGVuZCh0cmFjayk7CiAgICAgdHJhY2stPmFk ZE9ic2VydmVyKHRoaXMpOwogICAgIG1fZGVzY3JpcHRvci0+YWRkVHJhY2soJnRyYWNrLT5wcml2 YXRlVHJhY2soKSk7CkBAIC0xOTYsOSArMTkzLDYgQEAgYm9vbCBNZWRpYVN0cmVhbTo6cmVtb3Zl VHJhY2soUGFzc1JlZlB0cjxNZWRpYVN0cmVhbVRyYWNrPiBwcnBUcmFjaykKICAgICBSZWZQdHI8 TWVkaWFTdHJlYW1UcmFjaz4gdHJhY2sgPSBwcnBUcmFjazsKICAgICBNZWRpYVN0cmVhbVRyYWNr VmVjdG9yKiB0cmFja3MgPSBnZXRUcmFja1ZlY3RvckZvclR5cGUodHJhY2stPnNvdXJjZSgpLT50 eXBlKCkpOwogCi0gICAgaWYgKCF0cmFja3MpCi0gICAgICAgIHJldHVybiBmYWxzZTsKLQogICAg IHNpemVfdCBwb3MgPSB0cmFja3MtPmZpbmQodHJhY2spOwogICAgIGlmIChwb3MgPT0gbm90Rm91 bmQpCiAgICAgICAgIHJldHVybiBmYWxzZTsKQEAgLTI5Miw5ICsyODYsNiBAQCB2b2lkIE1lZGlh U3RyZWFtOjpyZW1vdmVSZW1vdGVTb3VyY2UoTWVkaWFTdHJlYW1Tb3VyY2UqIHNvdXJjZSkKIAog ICAgIFZlY3RvcjxSZWZQdHI8TWVkaWFTdHJlYW1UcmFjaz4+KiB0cmFja3MgPSBnZXRUcmFja1Zl Y3RvckZvclR5cGUoc291cmNlLT50eXBlKCkpOwogCi0gICAgaWYgKCF0cmFja3MpCi0gICAgICAg IHJldHVybjsKLQogICAgIGZvciAoaW50IGkgPSB0cmFja3MtPnNpemUoKSAtIDE7IGkgPj0gMDsg LS1pKSB7CiAgICAgICAgIGlmICgoKnRyYWNrcylbaV0tPnNvdXJjZSgpICE9IHNvdXJjZSkKICAg ICAgICAgICAgIGNvbnRpbnVlOwo= 215777 2013-11-01 16:39:54 -0700 2013-11-01 18:19:03 -0700 Patch bug-123644-20131101203953.patch text/plain 4191 thiago.lacerda U3VidmVyc2lvbiBSZXZpc2lvbjogMTU4NDY1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNjcyZjBkNzAyNDY2Y2Yw OTk5OGQzMGEwMjRkZDZhNjk2MDEyNDcyMC4uYmM5Y2QyNTU0MjAxODczNjRmZGRmMzcwNjFlNGNh NGZjMDJiM2U5MCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDEzLTExLTAxICBUaGlh Z28gZGUgQmFycm9zIExhY2VyZGEgIDx0aGlhZ28ubGFjZXJkYUBvcGVuYm9zc2Eub3JnPgorCisg ICAgICAgIFJlbW92aW5nIHVubmVjZXNzYXJ5IGVhcmx5IHJldHVybnMgaW4gYWRkVHJhY2ssIHJl bW92ZVRyYWNrIGFuZCByZW1vdmVSZW1vdGVTb3VyY2UgbWV0aG9kcworICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTIzNjQ0CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzIG5lZWRlZC4KKwor ICAgICAgICAqIE1vZHVsZXMvbWVkaWFzdHJlYW0vTWVkaWFTdHJlYW0uY3BwOgorICAgICAgICAo V2ViQ29yZTo6TWVkaWFTdHJlYW06OmFkZFRyYWNrKToKKyAgICAgICAgKFdlYkNvcmU6Ok1lZGlh U3RyZWFtOjpyZW1vdmVUcmFjayk6CisgICAgICAgIChXZWJDb3JlOjpNZWRpYVN0cmVhbTo6cmVt b3ZlUmVtb3RlU291cmNlKToKKyAgICAgICAgKFdlYkNvcmU6Ok1lZGlhU3RyZWFtOjp0cmFja1Zl Y3RvckZvclR5cGUpOgorICAgICAgICAqIE1vZHVsZXMvbWVkaWFzdHJlYW0vTWVkaWFTdHJlYW0u aDoKKwogMjAxMy0xMS0wMSAgQW5kcmVhcyBLbGluZyAgPGFrbGluZ0BhcHBsZS5jb20+CiAKICAg ICAgICAgQ1NTICdpbml0aWFsJyBhbmQgJ2luaGVyaXQnIHZhbHVlIGNvbnN0cnVjdG9ycyBzaG91 bGQgcmV0dXJuIFBhc3NSZWYuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL21l ZGlhc3RyZWFtL01lZGlhU3RyZWFtLmNwcCBiL1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvbWVkaWFz dHJlYW0vTWVkaWFTdHJlYW0uY3BwCmluZGV4IGQ3ZTc1Yjg3MzExZDc3ZjMzNjM0NTYwMDJjYTg5 ZDBlNThjNjQ4OGUuLjQyNTQwY2Y0N2M3OWU2ODRhODNiYjkzYzQyYWU0YWQ5N2Q4ZWExNjEgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvbWVkaWFzdHJlYW0vTWVkaWFTdHJlYW0u Y3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvbWVkaWFzdHJlYW0vTWVkaWFTdHJlYW0u Y3BwCkBAIC0xNjMsMTAgKzE2Myw3IEBAIGJvb2wgTWVkaWFTdHJlYW06OmFkZFRyYWNrKFBhc3NS ZWZQdHI8TWVkaWFTdHJlYW1UcmFjaz4gcHJwVHJhY2spCiAgICAgaWYgKGdldFRyYWNrQnlJZCh0 cmFjay0+aWQoKSkpCiAgICAgICAgIHJldHVybiBmYWxzZTsKIAotICAgIE1lZGlhU3RyZWFtVHJh Y2tWZWN0b3IqIHRyYWNrcyA9IGdldFRyYWNrVmVjdG9yRm9yVHlwZSh0cmFjay0+c291cmNlKCkt PnR5cGUoKSk7Ci0KLSAgICBpZiAoIXRyYWNrcykKLSAgICAgICAgcmV0dXJuIGZhbHNlOworICAg IE1lZGlhU3RyZWFtVHJhY2tWZWN0b3IqIHRyYWNrcyA9IHRyYWNrVmVjdG9yRm9yVHlwZSh0cmFj ay0+c291cmNlKCktPnR5cGUoKSk7CiAKICAgICB0cmFja3MtPmFwcGVuZCh0cmFjayk7CiAgICAg dHJhY2stPmFkZE9ic2VydmVyKHRoaXMpOwpAQCAtMTk0LDEwICsxOTEsNyBAQCBib29sIE1lZGlh U3RyZWFtOjpyZW1vdmVUcmFjayhQYXNzUmVmUHRyPE1lZGlhU3RyZWFtVHJhY2s+IHBycFRyYWNr KQogICAgIC8vIFRoaXMgaXMgYSBjb21tb24gcGFydCB1c2VkIGJ5IHJlbW92ZVRyYWNrIGNhbGxl ZCBieSBKYXZhU2NyaXB0CiAgICAgLy8gYW5kIHJlbW92ZVJlbW90ZVRyYWNrIGFuZCBvbmx5IHJl bW92ZVJlbW90ZVRyYWNrIG11c3QgZmlyZSByZW1vdmV0cmFjayBldmVudAogICAgIFJlZlB0cjxN ZWRpYVN0cmVhbVRyYWNrPiB0cmFjayA9IHBycFRyYWNrOwotICAgIE1lZGlhU3RyZWFtVHJhY2tW ZWN0b3IqIHRyYWNrcyA9IGdldFRyYWNrVmVjdG9yRm9yVHlwZSh0cmFjay0+c291cmNlKCktPnR5 cGUoKSk7Ci0KLSAgICBpZiAoIXRyYWNrcykKLSAgICAgICAgcmV0dXJuIGZhbHNlOworICAgIE1l ZGlhU3RyZWFtVHJhY2tWZWN0b3IqIHRyYWNrcyA9IHRyYWNrVmVjdG9yRm9yVHlwZSh0cmFjay0+ c291cmNlKCktPnR5cGUoKSk7CiAKICAgICBzaXplX3QgcG9zID0gdHJhY2tzLT5maW5kKHRyYWNr KTsKICAgICBpZiAocG9zID09IG5vdEZvdW5kKQpAQCAtMjkwLDEwICsyODQsNyBAQCB2b2lkIE1l ZGlhU3RyZWFtOjpyZW1vdmVSZW1vdGVTb3VyY2UoTWVkaWFTdHJlYW1Tb3VyY2UqIHNvdXJjZSkK ICAgICBpZiAoZW5kZWQoKSkKICAgICAgICAgcmV0dXJuOwogCi0gICAgVmVjdG9yPFJlZlB0cjxN ZWRpYVN0cmVhbVRyYWNrPj4qIHRyYWNrcyA9IGdldFRyYWNrVmVjdG9yRm9yVHlwZShzb3VyY2Ut PnR5cGUoKSk7Ci0KLSAgICBpZiAoIXRyYWNrcykKLSAgICAgICAgcmV0dXJuOworICAgIFZlY3Rv cjxSZWZQdHI8TWVkaWFTdHJlYW1UcmFjaz4+KiB0cmFja3MgPSB0cmFja1ZlY3RvckZvclR5cGUo c291cmNlLT50eXBlKCkpOwogCiAgICAgZm9yIChpbnQgaSA9IHRyYWNrcy0+c2l6ZSgpIC0gMTsg aSA+PSAwOyAtLWkpIHsKICAgICAgICAgaWYgKCgqdHJhY2tzKVtpXS0+c291cmNlKCkgIT0gc291 cmNlKQpAQCAtMzcxLDIxICszNjIsMTcgQEAgVVJMUmVnaXN0cnkmIE1lZGlhU3RyZWFtOjpyZWdp c3RyeSgpIGNvbnN0CiAgICAgcmV0dXJuIE1lZGlhU3RyZWFtUmVnaXN0cnk6OnJlZ2lzdHJ5KCk7 CiB9CiAKLU1lZGlhU3RyZWFtVHJhY2tWZWN0b3IqIE1lZGlhU3RyZWFtOjpnZXRUcmFja1ZlY3Rv ckZvclR5cGUoTWVkaWFTdHJlYW1Tb3VyY2U6OlR5cGUgdHlwZSkKK01lZGlhU3RyZWFtVHJhY2tW ZWN0b3IqIE1lZGlhU3RyZWFtOjp0cmFja1ZlY3RvckZvclR5cGUoTWVkaWFTdHJlYW1Tb3VyY2U6 OlR5cGUgdHlwZSkKIHsKLSAgICBNZWRpYVN0cmVhbVRyYWNrVmVjdG9yKiB0cmFja3MgPSBudWxs cHRyOwogICAgIHN3aXRjaCAodHlwZSkgewogICAgIGNhc2UgTWVkaWFTdHJlYW1Tb3VyY2U6OkF1 ZGlvOgotICAgICAgICB0cmFja3MgPSAmbV9hdWRpb1RyYWNrczsKLSAgICAgICAgYnJlYWs7Cisg ICAgICAgIHJldHVybiAmbV9hdWRpb1RyYWNrczsKICAgICBjYXNlIE1lZGlhU3RyZWFtU291cmNl OjpWaWRlbzoKLSAgICAgICAgdHJhY2tzID0gJm1fdmlkZW9UcmFja3M7Ci0gICAgICAgIGJyZWFr OworICAgICAgICByZXR1cm4gJm1fdmlkZW9UcmFja3M7CiAgICAgY2FzZSBNZWRpYVN0cmVhbVNv dXJjZTo6Tm9uZToKICAgICAgICAgQVNTRVJUX05PVF9SRUFDSEVEKCk7Ci0gICAgICAgIGJyZWFr OwogICAgIH0KLSAgICByZXR1cm4gdHJhY2tzOworICAgIHJldHVybiBudWxscHRyOwogfQogCiB9 IC8vIG5hbWVzcGFjZSBXZWJDb3JlCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVz L21lZGlhc3RyZWFtL01lZGlhU3RyZWFtLmggYi9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL21lZGlh c3RyZWFtL01lZGlhU3RyZWFtLmgKaW5kZXggOGZlMTlkMzhiZDJlODlhOWMyOTgwY2U4NTI4Zjhh MWQ5ZDU2MmFjYy4uM2Q2NzRjNTZmYjc1MjdjMjFiYmRhOGUwNDQ5MGVhOTM0NDcwOTBiMyAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvTW9kdWxlcy9tZWRpYXN0cmVhbS9NZWRpYVN0cmVhbS5o CisrKyBiL1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvbWVkaWFzdHJlYW0vTWVkaWFTdHJlYW0uaApA QCAtMTEwLDcgKzExMCw3IEBAIHByaXZhdGU6CiAKICAgICB2b2lkIGNsb25lTWVkaWFTdHJlYW1U cmFja1ZlY3RvcihNZWRpYVN0cmVhbVRyYWNrVmVjdG9yJiwgY29uc3QgTWVkaWFTdHJlYW1UcmFj a1ZlY3RvciYpOwogCi0gICAgTWVkaWFTdHJlYW1UcmFja1ZlY3RvciogZ2V0VHJhY2tWZWN0b3JG b3JUeXBlKE1lZGlhU3RyZWFtU291cmNlOjpUeXBlKTsKKyAgICBNZWRpYVN0cmVhbVRyYWNrVmVj dG9yKiB0cmFja1ZlY3RvckZvclR5cGUoTWVkaWFTdHJlYW1Tb3VyY2U6OlR5cGUpOwogCiAgICAg UmVmUHRyPE1lZGlhU3RyZWFtRGVzY3JpcHRvcj4gbV9kZXNjcmlwdG9yOwogICAgIE1lZGlhU3Ry ZWFtVHJhY2tWZWN0b3IgbV9hdWRpb1RyYWNrczsK