[webkit-reviews] review granted: [Bug 82358] [Mac] Stop using NSMapTable in FormDataStreamMac.mm : [Attachment 134103] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 82358: [Mac] Stop using NSMapTable in FormDataStreamMac.mm
https://bugs.webkit.org/show_bug.cgi?id=82358

Attachment 134103: proposed patch
https://bugs.webkit.org/attachment.cgi?id=134103&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.