Bug 82358

Summary: [Mac] Stop using NSMapTable in FormDataStreamMac.mm
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: FormsAssignee: Alexey Proskuryakov <ap>
Severity: Normal CC: andersca, darin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
proposed patch darin: review+

Description Alexey Proskuryakov 2012-03-27 11:24:52 PDT
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>).
Comment 1 Alexey Proskuryakov 2012-03-27 11:26:53 PDT
Created attachment 134103 [details]
proposed patch
Comment 2 Darin Adler 2012-03-27 12:16:46 PDT
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:

    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.
Comment 3 Alexey Proskuryakov 2012-03-27 12:34:07 PDT
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).
Comment 4 Darin Adler 2012-03-27 12:38:34 PDT
(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.