Enable WTF::HashSet<OwnPtr<> >

Enable WTF::HashSet<OwnPtr<> >

Allow HashSet to have OwnPtr elements, this helps in
cases when the container needs to own its elements.
The client does not need to care about explicit calling
'deleteAllValues' funcion (it can be deprecated now).

The patch contains unit tests, and examples of real
using HashSet<OwnPtr<> > in XPathParser.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172738

[Mikhail, 2014-04-28 06:22:01]

PTAL

[eseidel, 2014-04-28 06:31:34]

lgtm

So much better. thank you.

Erik Corry, jyasskin are better template reviewers than I, but I like the idea.

[Erik Corry, 2014-04-28 07:53:02]

LGTM

https://codereview.chromium.org/252633005/diff/40001/Source/wtf/HashFunctions.h
File Source/wtf/HashFunctions.h (right):

https://codereview.chromium.org/252633005/diff/40001/Source/wtf/HashFunctions...
Source/wtf/HashFunctions.h:164: RELEASE_ASSERT(!a || a.get() != b.get());
I understand why you have this, but I think RELEASE_ASSERT is too heavy for
something that gets called multiple times for each add or lookup in the set.  I
don't think there's a very good chance that this catches anything.

https://codereview.chromium.org/252633005/diff/40001/Source/wtf/HashSetTest.cpp
File Source/wtf/HashSetTest.cpp (right):

https://codereview.chromium.org/252633005/diff/40001/Source/wtf/HashSetTest.c...
Source/wtf/HashSetTest.cpp:130: set.clear();
It would be great if you could make the test also test that calling the
destructor of a non-empty set calls the destructor of the ownptr.

https://codereview.chromium.org/252633005/diff/40001/Source/wtf/HashSetTest.c...
Source/wtf/HashSetTest.cpp:146: static int m_refInvokesCount;
Should be called s_refInvokesCount.

[Mikhail, 2014-04-28 08:00:39]

Thanks for having a look!

https://codereview.chromium.org/252633005/diff/40001/Source/wtf/HashFunctions.h
File Source/wtf/HashFunctions.h (right):

https://codereview.chromium.org/252633005/diff/40001/Source/wtf/HashFunctions...
Source/wtf/HashFunctions.h:164: RELEASE_ASSERT(!a || a.get() != b.get());
On 2014/04/28 07:53:03, Erik Corry wrote:
> I understand why you have this, but I think RELEASE_ASSERT is too heavy for
> something that gets called multiple times for each add or lookup in the set. 
I
> don't think there's a very good chance that this catches anything.

Oh, thanks for the catch! I've put it for debugging and forgot to remove before
uploading..

https://codereview.chromium.org/252633005/diff/40001/Source/wtf/HashSetTest.cpp
File Source/wtf/HashSetTest.cpp (right):

https://codereview.chromium.org/252633005/diff/40001/Source/wtf/HashSetTest.c...
Source/wtf/HashSetTest.cpp:130: set.clear();
On 2014/04/28 07:53:03, Erik Corry wrote:
> It would be great if you could make the test also test that calling the
> destructor of a non-empty set calls the destructor of the ownptr.

Will add.

[Mikhail, 2014-04-28 09:02:03]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[commit-bot: I haz the power, 2014-04-28 09:02:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/252633005...

[commit-bot: I haz the power, 2014-04-28 09:54:18]

Change committed as 172738