Fix PersistentHeapHashMap to work with stack allocated, heap backed weak collections

Fix PersistentHeapHashMap to work with stack allocated, heap backed weak collections

R=ager@chromium.org, sof@chromium.org
BUG=

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

[Mads Ager (chromium), 2014-03-12 15:46:50]

LGTM when the comments have been updated.

https://codereview.chromium.org/197343003/diff/1/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/197343003/diff/1/Source/heap/Heap.h#newcode782
Source/heap/Heap.h:782: static void pushWeakObjectPointerCallback(void* closure,
void* containerObject, WeakPointerCallback);
Please update the comment with an explanation of what decided the thread and
what gets the callback. We might want to rename containerObject here?

Also, maybe add a comment that closure and object are usually the same but that
for heap collections, they are different because the front object needs to get
the callback but the backing store is the only thing that is guaranteed to be in
the heap.

[sof, 2014-03-12 15:48:15]

https://codereview.chromium.org/197343003/diff/1/Source/wtf/HashTable.h
File Source/wtf/HashTable.h (right):

https://codereview.chromium.org/197343003/diff/1/Source/wtf/HashTable.h#newco...
Source/wtf/HashTable.h:1074: Allocator::registerWeakMembers(visitor, this,
m_table, WeakProcessingHashTableHelper<Traits::isWeak, Key, Value, Extractor,
HashFunctions, Traits, KeyTraits, Allocator>::process);
Can m_table be 0 ?

[Mads Ager (chromium), 2014-03-12 15:53:49]

https://codereview.chromium.org/197343003/diff/1/Source/wtf/HashTable.h
File Source/wtf/HashTable.h (right):

https://codereview.chromium.org/197343003/diff/1/Source/wtf/HashTable.h#newco...
Source/wtf/HashTable.h:1074: Allocator::registerWeakMembers(visitor, this,
m_table, WeakProcessingHashTableHelper<Traits::isWeak, Key, Value, Extractor,
HashFunctions, Traits, KeyTraits, Allocator>::process);
On 2014/03/12 15:48:15, sof wrote:
> Can m_table be 0 ?

Good catch! It most definitely can. In that case we don't need a weak callback,
so we just need to not register weak members if m_table is null.

[sof, 2014-03-12 16:20:54]

On 2014/03/12 15:53:49, Mads Ager (chromium) wrote:
> https://codereview.chromium.org/197343003/diff/1/Source/wtf/HashTable.h
> File Source/wtf/HashTable.h (right):
> 
>
https://codereview.chromium.org/197343003/diff/1/Source/wtf/HashTable.h#newco...
> Source/wtf/HashTable.h:1074: Allocator::registerWeakMembers(visitor, this,
> m_table, WeakProcessingHashTableHelper<Traits::isWeak, Key, Value, Extractor,
> HashFunctions, Traits, KeyTraits, Allocator>::process);
> On 2014/03/12 15:48:15, sof wrote:
> > Can m_table be 0 ?
> 
> Good catch! It most definitely can. In that case we don't need a weak
callback,
> so we just need to not register weak members if m_table is null.

Yes. But looking at the full method now, there is an explicit null check above,
so this is fine. lgtm from here.

[haraken, 2014-03-13 00:49:04]

LGTM

[Erik Corry, 2014-03-13 07:46:21]

https://codereview.chromium.org/197343003/diff/1/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/197343003/diff/1/Source/heap/Heap.h#newcode782
Source/heap/Heap.h:782: static void pushWeakObjectPointerCallback(void* closure,
void* containerObject, WeakPointerCallback);
On 2014/03/12 15:46:50, Mads Ager (chromium) wrote:
> Please update the comment with an explanation of what decided the thread and
> what gets the callback. We might want to rename containerObject here?
> 
> Also, maybe add a comment that closure and object are usually the same but
that
> for heap collections, they are different because the front object needs to get
> the callback but the backing store is the only thing that is guaranteed to be
in
> the heap.

Done.

[Erik Corry, 2014-03-13 07:50:47]

The CQ bit was checked by erik.corry@gmail.com

[commit-bot: I haz the power, 2014-03-13 07:50:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erik.corry@gmail.com/197343003/20001

[commit-bot: I haz the power, 2014-03-13 08:01:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 08:01:13]

Try jobs failed on following builders:
  tryserver.chromium on blink_presubmit

[jochen (gone - plz use gerrit), 2014-03-13 08:29:01]

lgtm

[Erik Corry, 2014-03-13 08:54:46]

The CQ bit was checked by erik.corry@gmail.com

[commit-bot: I haz the power, 2014-03-13 08:54:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erik.corry@gmail.com/197343003/20001

[commit-bot: I haz the power, 2014-03-13 09:33:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 09:33:52]

Try jobs failed on following builders:
  tryserver.chromium on linux_blink

[Erik Corry, 2014-03-13 12:15:24]

The CQ bit was checked by erik.corry@gmail.com

[commit-bot: I haz the power, 2014-03-13 12:15:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erik.corry@gmail.com/197343003/20001

[commit-bot: I haz the power, 2014-03-13 13:32:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 13:32:09]

Try jobs failed on following builders:
  tryserver.chromium on linux_blink

[Erik Corry, 2014-03-13 13:33:54]

The CQ bit was checked by erik.corry@gmail.com

[commit-bot: I haz the power, 2014-03-13 13:33:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erik.corry@gmail.com/197343003/20001

[commit-bot: I haz the power, 2014-03-13 14:52:56]

Change committed as 169131