Bug 82358 - [Mac] Stop using NSMapTable in FormDataStreamMac.mm
Summary: [Mac] Stop using NSMapTable in FormDataStreamMac.mm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-27 11:24 PDT by Alexey Proskuryakov
Modified: 2012-03-27 12:38 PDT (History)
2 users (show)

See Also:


Attachments
proposed patch (3.66 KB, patch)
2012-03-27 11:26 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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:

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