82358 2012-03-27 11:24:52 -0700 [Mac] Stop using NSMapTable in FormDataStreamMac.mm 2012-03-27 12:38:34 -0700 1 1 1 Unclassified WebKit Forms 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 ap ap andersca darin oldest_to_newest 589068 0 ap 2012-03-27 11:24:52 -0700 I'd like to make this file more cross-platform, and I can't figure out the reason why NSMapTable was used (introduced in <http://trac.webkit.org/changeset/101825>). 589071 1 134103 ap 2012-03-27 11:26:53 -0700 Created attachment 134103 proposed patch 589130 2 134103 darin 2012-03-27 12:16:46 -0700 Comment on attachment 134103 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=134103&action=review > Source/WebCore/platform/network/mac/FormDataStreamMac.mm:81 > +typedef HashMap<CFReadStreamRef, FormStreamFields*> StreamFieldsMap; I’m not overjoyed with the lifetime management here. I would like this better if it was an OwnPtr<FormStreamFields>, but it seems that’s not compatible with how we defer finalization to make sure it’s done on the main thread. > Source/WebCore/platform/network/mac/FormDataStreamMac.mm:194 > - ASSERT(!NSMapGet(streamFieldsMap(), stream)); > - NSMapInsertKnownAbsent(streamFieldsMap(), stream, newInfo); > + bool mapHadInfoForStream = streamFieldsMap().set(stream, newInfo).second; > + ASSERT_UNUSED(mapHadInfoForStream, !mapHadInfoForStream); I think it’s clearer to just write: ASSERT(!streamFieldsMap().contains(stream)); streamFieldsMap().add(stream, newInfo); It costs an extra hash table lookup in debug builds, but that seems fine. And I think add is more efficient than set; I believe set is implemented in terms of add. 589145 3 ap 2012-03-27 12:34:07 -0700 Committed <http://trac.webkit.org/changeset/112302>. > ASSERT(!streamFieldsMap().contains(stream)); > streamFieldsMap().add(stream, newInfo); Yes, changed to this. My code was actually broken, asserting the opposite (caught that when running tests). 589158 4 darin 2012-03-27 12:38:34 -0700 (In reply to comment #0) > I'd like to make this file more cross-platform, and I can't figure out the reason why NSMapTable was used (introduced in <http://trac.webkit.org/changeset/101825>). The reason you couldn’t figure that out is that the use of NSMapTable was not introduced in r101825. It was introduced in r101813. And back in that earlier revision you can see a check-in comment and comment in the code explaining why NSMapTable was used. Further, r101825 removed the need to use NSMapTable and so removed the comment explaining why it was needed. 134103 2012-03-27 11:26:53 -0700 2012-03-27 12:16:46 -0700 proposed patch NSMapTable.txt text/plain 3743 ap SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExMjI4NikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBACisyMDEyLTAzLTI3ICBBbGV4ZXkg UHJvc2t1cnlha292ICA8YXBAYXBwbGUuY29tPgorCisgICAgICAgIFtNYWNdIFN0b3AgdXNpbmcg TlNNYXBUYWJsZSBpbiBGb3JtRGF0YVN0cmVhbU1hYy5tbQorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODIzNTgKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHBsYXRmb3JtL25ldHdvcmsvbWFjL0Zvcm1EYXRh U3RyZWFtTWFjLm1tOiBVc2UgV1RGOjpIYXNoTWFwLCBhcyB3ZSBhbHdheXMgZG8uIEFsbCBhY2Nl c3NlcworICAgICAgICBhcmUgcHJvdGVjdGVkIHdpdGggYSBtdXRleCBhbnl3YXkuCisKIDIwMTIt MDMtMjcgIFNhbWkgS3lvc3RpbGEgIDxza3lvc3RpbEBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAg W2Nocm9taXVtXSBBZGQgVGV4dHVyZUNvcGllciBmb3IgY29weWluZyB0ZXh0dXJlIGNvbnRlbnRz CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL21hYy9Gb3JtRGF0YVN0cmVh bU1hYy5tbQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL21h Yy9Gb3JtRGF0YVN0cmVhbU1hYy5tbQkocmV2aXNpb24gMTEyMTY2KQorKysgU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vbmV0d29yay9tYWMvRm9ybURhdGFTdHJlYW1NYWMubW0JKHdvcmtpbmcgY29w eSkKQEAgLTEsNSArMSw1IEBACiAvKgotICogQ29weXJpZ2h0IChDKSAyMDA1LCAyMDA2LCAyMDA4 LCAyMDExIEFwcGxlIEluYy4gQWxsIHJpZ2h0cyByZXNlcnZlZC4KKyAqIENvcHlyaWdodCAoQykg MjAwNSwgMjAwNiwgMjAwOCwgMjAxMSwgMjAxMiBBcHBsZSBJbmMuIEFsbCByaWdodHMgcmVzZXJ2 ZWQuCiAgKgogICogUmVkaXN0cmlidXRpb24gYW5kIHVzZSBpbiBzb3VyY2UgYW5kIGJpbmFyeSBm b3Jtcywgd2l0aCBvciB3aXRob3V0CiAgKiBtb2RpZmljYXRpb24sIGFyZSBwZXJtaXR0ZWQgcHJv dmlkZWQgdGhhdCB0aGUgZm9sbG93aW5nIGNvbmRpdGlvbnMKQEAgLTU3LDEyICs1Nyw2IEBAIHN0 YXRpYyBNdXRleCYgc3RyZWFtRmllbGRzTWFwTXV0ZXgoKQogICAgIHJldHVybiBzdGF0aWNNdXRl eDsKIH0KIAotc3RhdGljIE5TTWFwVGFibGUgKnN0cmVhbUZpZWxkc01hcCgpCi17Ci0gICAgc3Rh dGljIE5TTWFwVGFibGUgKnN0cmVhbUZpZWxkc01hcCA9IE5TQ3JlYXRlTWFwVGFibGUoTlNOb25S ZXRhaW5lZE9iamVjdE1hcEtleUNhbGxCYWNrcywgTlNOb25Pd25lZFBvaW50ZXJNYXBWYWx1ZUNh bGxCYWNrcywgMSk7Ci0gICAgcmV0dXJuIHN0cmVhbUZpZWxkc01hcDsKLX0KLQogc3RhdGljIHZv aWQgZm9ybUV2ZW50Q2FsbGJhY2soQ0ZSZWFkU3RyZWFtUmVmIHN0cmVhbSwgQ0ZTdHJlYW1FdmVu dFR5cGUgdHlwZSwgdm9pZCogY29udGV4dCk7CiAKIHN0cnVjdCBGb3JtQ29udGV4dCB7CkBAIC04 NCw2ICs3OCwxMyBAQCBzdHJ1Y3QgRm9ybVN0cmVhbUZpZWxkcyB7CiAgICAgdW5zaWduZWQgbG9u ZyBsb25nIGJ5dGVzU2VudDsKIH07CiAKK3R5cGVkZWYgSGFzaE1hcDxDRlJlYWRTdHJlYW1SZWYs IEZvcm1TdHJlYW1GaWVsZHMqPiBTdHJlYW1GaWVsZHNNYXA7CitzdGF0aWMgU3RyZWFtRmllbGRz TWFwJiBzdHJlYW1GaWVsZHNNYXAoKQoreworICAgIERFRklORV9TVEFUSUNfTE9DQUwoU3RyZWFt RmllbGRzTWFwLCBzdHJlYW1GaWVsZHNNYXAsICgpKTsKKyAgICByZXR1cm4gc3RyZWFtRmllbGRz TWFwOworfQorCiBzdGF0aWMgdm9pZCBjbG9zZUN1cnJlbnRTdHJlYW0oRm9ybVN0cmVhbUZpZWxk cyAqZm9ybSkKIHsKICAgICBpZiAoZm9ybS0+Y3VycmVudFN0cmVhbSkgewpAQCAtMTg5LDggKzE5 MCw4IEBAIHN0YXRpYyB2b2lkKiBmb3JtQ3JlYXRlKENGUmVhZFN0cmVhbVJlZiAKICAgICAgICAg bmV3SW5mby0+cmVtYWluaW5nRWxlbWVudHMuYXBwZW5kKG5ld0luZm8tPmZvcm1EYXRhLT5lbGVt ZW50cygpW3NpemUgLSBpIC0gMV0pOwogCiAgICAgTXV0ZXhMb2NrZXIgbG9ja2VyKHN0cmVhbUZp ZWxkc01hcE11dGV4KCkpOwotICAgIEFTU0VSVCghTlNNYXBHZXQoc3RyZWFtRmllbGRzTWFwKCks IHN0cmVhbSkpOwotICAgIE5TTWFwSW5zZXJ0S25vd25BYnNlbnQoc3RyZWFtRmllbGRzTWFwKCks IHN0cmVhbSwgbmV3SW5mbyk7CisgICAgYm9vbCBtYXBIYWRJbmZvRm9yU3RyZWFtID0gc3RyZWFt RmllbGRzTWFwKCkuc2V0KHN0cmVhbSwgbmV3SW5mbykuc2Vjb25kOworICAgIEFTU0VSVF9VTlVT RUQobWFwSGFkSW5mb0ZvclN0cmVhbSwgIW1hcEhhZEluZm9Gb3JTdHJlYW0pOwogCiAgICAgcmV0 dXJuIG5ld0luZm87CiB9CkBAIC0yMDksMTMgKzIxMCwxMyBAQCBzdGF0aWMgdm9pZCBmb3JtRmlu YWxpemUoQ0ZSZWFkU3RyZWFtUmVmCiAgICAgTXV0ZXhMb2NrZXIgbG9ja2VyKHN0cmVhbUZpZWxk c01hcE11dGV4KCkpOwogCiAgICAgQVNTRVJUKGZvcm0tPmZvcm1TdHJlYW0gPT0gc3RyZWFtKTsK LSAgICBBU1NFUlQoTlNNYXBHZXQoc3RyZWFtRmllbGRzTWFwKCksIHN0cmVhbSkgPT0gY29udGV4 dCk7CisgICAgQVNTRVJUKHN0cmVhbUZpZWxkc01hcCgpLmdldChzdHJlYW0pID09IGNvbnRleHQp OwogCiAgICAgLy8gRG8gdGhpcyByaWdodCBhd2F5IGJlY2F1c2UgdGhlIENGUmVhZFN0cmVhbVJl ZiBpcyBiZWluZyBkZWFsbG9jYXRlZC4KICAgICAvLyBXZSBjYW4ndCB3YWl0IHRvIHJlbW92ZSB0 aGlzIGZyb20gdGhlIG1hcCB1bnRpbCB3ZSBmaW5pc2ggZmluYWxpemluZwogICAgIC8vIG9uIHRo ZSBtYWluIHRocmVhZCBiZWNhdXNlIGluIHRoZW9yeSB0aGUgZnJlZWQgbWVtb3J5IGNvdWxkIGJl IHJldXNlZAogICAgIC8vIGZvciBhIG5ldyBDRlJlYWRTdHJlYW0gYmVmb3JlIHRoYXQgcnVucy4K LSAgICBOU01hcFJlbW92ZShzdHJlYW1GaWVsZHNNYXAoKSwgc3RyZWFtKTsKKyAgICBzdHJlYW1G aWVsZHNNYXAoKS5yZW1vdmUoc3RyZWFtKTsKIAogICAgIGNhbGxPbk1haW5UaHJlYWQoZm9ybUZp bmlzaEZpbmFsaXphdGlvbk9uTWFpblRocmVhZCwgZm9ybSk7CiB9CkBAIC00MzUsNyArNDM2LDcg QEAgdm9pZCBzZXRIVFRQQm9keShOU011dGFibGVVUkxSZXF1ZXN0ICpyZQogRm9ybURhdGEqIGh0 dHBCb2R5RnJvbVN0cmVhbShOU0lucHV0U3RyZWFtKiBzdHJlYW0pCiB7CiAgICAgTXV0ZXhMb2Nr ZXIgbG9ja2VyKHN0cmVhbUZpZWxkc01hcE11dGV4KCkpOwotICAgIEZvcm1TdHJlYW1GaWVsZHMq IGZvcm1TdHJlYW0gPSBzdGF0aWNfY2FzdDxGb3JtU3RyZWFtRmllbGRzKj4oTlNNYXBHZXQoc3Ry ZWFtRmllbGRzTWFwKCksIHN0cmVhbSkpOworICAgIEZvcm1TdHJlYW1GaWVsZHMqIGZvcm1TdHJl YW0gPSBzdHJlYW1GaWVsZHNNYXAoKS5nZXQocmVpbnRlcnByZXRfY2FzdDxDRlJlYWRTdHJlYW1S ZWY+KHN0cmVhbSkpOwogICAgIGlmICghZm9ybVN0cmVhbSkKICAgICAgICAgcmV0dXJuIDA7CiAg ICAgcmV0dXJuIGZvcm1TdHJlYW0tPmZvcm1EYXRhLmdldCgpOwo=