Replace raw pointers to GC allocated objects by members in stack allocated objects.

Replace raw pointers to GC allocated objects by members in stack allocated objects.

R=haraken@chromium.org
BUG=

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

[haraken, 2014-03-24 07:05:02]

LGTM

https://codereview.chromium.org/208313010/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleResolverState.cpp (right):

https://codereview.chromium.org/208313010/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleResolverState.cpp:42: , m_currentRule(nullptr)

Can we detect an error if we forget to initialize a Member in a stack-allocated
object with nullptr ?

https://codereview.chromium.org/208313010/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/208313010/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleResolverState.h:46:
WTF_MAKE_NONCOPYABLE(StyleResolverState);

Nit: Fix indentation.

https://codereview.chromium.org/208313010/diff/1/Source/modules/filesystem/Sy...
File Source/modules/filesystem/SyncCallbackHelper.h (right):

https://codereview.chromium.org/208313010/diff/1/Source/modules/filesystem/Sy...
Source/modules/filesystem/SyncCallbackHelper.h:157: struct EmptyType : public
RefCountedWillBeGarbageCollected<EmptyType> {

I don't know why this change is needed, but it looks OK.

[Mads Ager (chromium), 2014-03-24 07:24:42]

LGTM

https://codereview.chromium.org/208313010/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleResolverState.cpp (right):

https://codereview.chromium.org/208313010/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleResolverState.cpp:42: , m_currentRule(nullptr)
On 2014/03/24 07:05:03, haraken wrote:
> 
> Can we detect an error if we forget to initialize a Member in a
stack-allocated
> object with nullptr ?

Drive-by answer: Member initializes itself with zero in any case, so we do not
have to initialize them with nullptr. The thing that becomes dangerous is
RawPtr. In the end those will become raw pointers that we have to properly
initialize.

Come to think of it, maybe we should initialize RawPtr with a recognizable zap
value instead of 0. They represent raw pointers and therefore they should always
be explicitly initialized to zero if that is what we want. Otherwise, we are in
for surprises when we move from: RawPtr<A> a; to A* a;

[haraken, 2014-03-24 07:36:21]

https://codereview.chromium.org/208313010/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleResolverState.cpp (right):

https://codereview.chromium.org/208313010/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleResolverState.cpp:42: , m_currentRule(nullptr)
> Come to think of it, maybe we should initialize RawPtr with a recognizable zap
> value instead of 0. They represent raw pointers and therefore they should
always
> be explicitly initialized to zero if that is what we want. Otherwise, we are
in
> for surprises when we move from: RawPtr<A> a; to A* a;

This sounds like a good idea.

[wibling-chromium, 2014-03-24 08:18:37]

lgtm

https://codereview.chromium.org/208313010/diff/1/Source/bindings/v8/Dictionary.h
File Source/bindings/v8/Dictionary.h (right):

https://codereview.chromium.org/208313010/diff/1/Source/bindings/v8/Dictionar...
Source/bindings/v8/Dictionary.h:62: public:
Can't we add STACK_ALLOCATED() here?

https://codereview.chromium.org/208313010/diff/1/Source/modules/speech/Speech...
File Source/modules/speech/SpeechRecognitionEvent.h (left):

https://codereview.chromium.org/208313010/diff/1/Source/modules/speech/Speech...
Source/modules/speech/SpeechRecognitionEvent.h:41: DISALLOW_ALLOCATION();
It is a bit unfortunate that we cannot see this object is STACK_ALLOCATED unless
looking at EventInit. Not sure how to fix it though as adding STACK_ALLOCATED is
seems too much.

[zerny-chromium, 2014-03-24 08:32:27]

Thanks for the reviews

https://codereview.chromium.org/208313010/diff/1/Source/bindings/v8/Dictionary.h
File Source/bindings/v8/Dictionary.h (right):

https://codereview.chromium.org/208313010/diff/1/Source/bindings/v8/Dictionar...
Source/bindings/v8/Dictionary.h:62: public:
On 2014/03/24 08:18:37, wibling-chromium wrote:
> Can't we add STACK_ALLOCATED() here?

Good point. I'll add that and remove the explicit new below.

https://codereview.chromium.org/208313010/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/208313010/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleResolverState.h:46:
WTF_MAKE_NONCOPYABLE(StyleResolverState);
On 2014/03/24 07:05:03, haraken wrote:
> 
> Nit: Fix indentation.

Done.

https://codereview.chromium.org/208313010/diff/1/Source/modules/filesystem/Sy...
File Source/modules/filesystem/SyncCallbackHelper.h (right):

https://codereview.chromium.org/208313010/diff/1/Source/modules/filesystem/Sy...
Source/modules/filesystem/SyncCallbackHelper.h:157: struct EmptyType : public
RefCountedWillBeGarbageCollected<EmptyType> {
On 2014/03/24 07:05:03, haraken wrote:
> 
> I don't know why this change is needed, but it looks OK.

We can't put non-GC allocated types in members as statically asserted in the
Member constructors.

https://codereview.chromium.org/208313010/diff/1/Source/modules/speech/Speech...
File Source/modules/speech/SpeechRecognitionEvent.h (left):

https://codereview.chromium.org/208313010/diff/1/Source/modules/speech/Speech...
Source/modules/speech/SpeechRecognitionEvent.h:41: DISALLOW_ALLOCATION();
On 2014/03/24 08:18:37, wibling-chromium wrote:
> It is a bit unfortunate that we cannot see this object is STACK_ALLOCATED
unless
> looking at EventInit. Not sure how to fix it though as adding STACK_ALLOCATED
is
> seems too much.

I agree, but requiring it to be added is too intrusive.

[zerny-chromium, 2014-03-24 10:08:24]

https://codereview.chromium.org/208313010/diff/1/Source/bindings/v8/Dictionary.h
File Source/bindings/v8/Dictionary.h (right):

https://codereview.chromium.org/208313010/diff/1/Source/bindings/v8/Dictionar...
Source/bindings/v8/Dictionary.h:62: public:
On 2014/03/24 08:32:28, zerny-chromium wrote:
> On 2014/03/24 08:18:37, wibling-chromium wrote:
> > Can't we add STACK_ALLOCATED() here?
> 
> Good point. I'll add that and remove the explicit new below.

QQ: it needs to be ALLOW_ONLY_INLINE_ALLOCATION().

[zerny-chromium, 2014-03-24 10:12:00]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-03-24 10:12:11]

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

[commit-bot: I haz the power, 2014-03-24 12:11:48]

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

[commit-bot: I haz the power, 2014-03-24 12:11:49]

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

[zerny-chromium, 2014-03-24 13:10:49]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-03-24 13:10:50]

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

[commit-bot: I haz the power, 2014-03-24 14:21:18]

Change committed as 169847