Bug 82358 – [Mac] Stop using NSMapTable in FormDataStreamMac.mm

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Alexey Proskuryakov]

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>).

[Alexey Proskuryakov]

Created attachment 134103 [details]
proposed patch

[Darin Adler]

Comment on attachment 134103 [details]
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.

[Alexey Proskuryakov]

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).

[Darin Adler]

(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.