Oilpan: Add a test to confirm that pre-finalization hook can be implmemented with HeapHashSet<WeakM…

Oilpan: Add a test to confirm that pre-finalization hook can be implmemented with HeapHashSet<WeakMmber<T>, U>.

BUG=

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

[Mads Ager (chromium), 2014-03-19 06:38:22]

LGTM

https://codereview.chromium.org/204313002/diff/1/Source/heap/HeapTest.cpp
File Source/heap/HeapTest.cpp (right):

https://codereview.chromium.org/204313002/diff/1/Source/heap/HeapTest.cpp#new...
Source/heap/HeapTest.cpp:946: static ObserverMap& observe(Observable& target)
I would just pass in the pointer here instead of a reference. Changing from
pointer to reference and then back to pointer doesn't seem to add much?

https://codereview.chromium.org/204313002/diff/1/Source/heap/HeapTest.cpp#new...
Source/heap/HeapTest.cpp:2677: foo = nullptr;
As the try bots say, this needs to just be 0 for the compilers that do not have
real nullptr support yet. :(

[tkent, 2014-03-19 08:00:13]

https://codereview.chromium.org/204313002/diff/1/Source/heap/HeapTest.cpp
File Source/heap/HeapTest.cpp (right):

https://codereview.chromium.org/204313002/diff/1/Source/heap/HeapTest.cpp#new...
Source/heap/HeapTest.cpp:946: static ObserverMap& observe(Observable& target)
On 2014/03/19 06:38:23, Mads Ager (chromium) wrote:
> I would just pass in the pointer here instead of a reference. Changing from
> pointer to reference and then back to pointer doesn't seem to add much?

We prefer references in Blink if values can't be null.

https://codereview.chromium.org/204313002/diff/1/Source/heap/HeapTest.cpp#new...
Source/heap/HeapTest.cpp:2677: foo = nullptr;
On 2014/03/19 06:38:23, Mads Ager (chromium) wrote:
> As the try bots say, this needs to just be 0 for the compilers that do not
have
> real nullptr support yet. :(

Done.

[tkent, 2014-03-19 08:00:17]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-19 08:00:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tkent@chromium.org/204313002/20001

[haraken, 2014-03-19 08:28:11]

LGTM

https://codereview.chromium.org/204313002/diff/20001/Source/heap/HeapTest.cpp
File Source/heap/HeapTest.cpp (right):

https://codereview.chromium.org/204313002/diff/20001/Source/heap/HeapTest.cpp...
Source/heap/HeapTest.cpp:939: FinalizationObserverWithHashMap(Observable&
target) : m_target(target) { }

Add explicit.

[commit-bot: I haz the power, 2014-03-19 08:30:57]

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

[commit-bot: I haz the power, 2014-03-19 08:30:58]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[tkent, 2014-03-19 08:52:52]

https://codereview.chromium.org/204313002/diff/20001/Source/heap/HeapTest.cpp
File Source/heap/HeapTest.cpp (right):

https://codereview.chromium.org/204313002/diff/20001/Source/heap/HeapTest.cpp...
Source/heap/HeapTest.cpp:939: FinalizationObserverWithHashMap(Observable&
target) : m_target(target) { }
On 2014/03/19 08:28:12, haraken wrote:
> 
> Add explicit.

Done.

[tkent, 2014-03-19 08:53:01]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-19 08:53:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tkent@chromium.org/204313002/40001

[commit-bot: I haz the power, 2014-03-19 08:55:58]

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

[commit-bot: I haz the power, 2014-03-19 08:55:59]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[tkent, 2014-03-19 08:57:04]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-19 08:57:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tkent@chromium.org/204313002/40001

[Mads Ager (chromium), 2014-03-19 09:40:22]

https://codereview.chromium.org/204313002/diff/1/Source/heap/HeapTest.cpp
File Source/heap/HeapTest.cpp (right):

https://codereview.chromium.org/204313002/diff/1/Source/heap/HeapTest.cpp#new...
Source/heap/HeapTest.cpp:946: static ObserverMap& observe(Observable& target)
On 2014/03/19 08:00:14, tkent wrote:
> On 2014/03/19 06:38:23, Mads Ager (chromium) wrote:
> > I would just pass in the pointer here instead of a reference. Changing from
> > pointer to reference and then back to pointer doesn't seem to add much?
> 
> We prefer references in Blink if values can't be null.

Well, this object is dynamically allocated. If you are using new, there is no
guarantee that it cannot be null. In this particular case the object is in the
heap and therefore if allocation fail we will terminate instead of returning 0.
However, in general there is no guarantee that something that you dynamically
allocate cannot be null. I find references reasonable for stack allocated
objects and part objects. For dynamically allocated objects like these I find it
misleading and I don't think we should use it here. It makes the code harder to
read for no reason.

[commit-bot: I haz the power, 2014-03-19 10:01:28]

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

[commit-bot: I haz the power, 2014-03-19 10:01:29]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[tkent, 2014-03-19 23:48:29]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-19 23:48:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tkent@chromium.org/204313002/40001

[commit-bot: I haz the power, 2014-03-20 01:05:42]

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

[commit-bot: I haz the power, 2014-03-20 01:05:43]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[tkent, 2014-03-24 05:18:20]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-24 05:18:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tkent@chromium.org/204313002/40001

[commit-bot: I haz the power, 2014-03-24 06:47:24]

Change committed as 169821