Move speech module over to Oilpan.

Move speech module over to Oilpan.

This moves the speech/ interfaces over to using Oilpan transition
types. With the exception of the event objects, which will wait until
the base classes have been moved over.

R=haraken@chromium.org, zerny@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167088

[sof, 2014-02-11 19:30:23]

When you have some time, please take a look.

Trying to pick up on the practices and good patterns that the Oilpan team have
developed, but I'm quite sure I haven't grokked all yet. So, please let me know
where the changes are at odds with those.

Some meta comments / remarks:

 - Have not attempted to put various Vector<>s into the
   heap. Well, I attempted, but found myself wanting a
   transition type. Did I not look closely enough
   and/or should this be considered added?
 - The namespace aliases don't appear "namespace clean",
   in not qualifying the RefPtr<> et al RHSes wrt their
   namespace. But perhaps I'm missing something about
   intended use.
 - I held off on converting the various event objects, until
   Event has been moved over.

[tkent, 2014-02-12 04:02:06]

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.h:59: PassRefPtr<SpeechGrammarList>
grammars() { return m_grammars; }
Returning PassRefPtr looks a bug of the original code.  This should return
SpeechGrammarList*.

[haraken, 2014-02-12 05:18:44]

Sorry for the review delay.

https://codereview.chromium.org/139803012/diff/130001/Source/bindings/v8/Dict...
File Source/bindings/v8/Dictionary.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/bindings/v8/Dict...
Source/bindings/v8/Dictionary.h:165: bool convert(ConversionContext &, const
String&, RawPtr<T>&) const;

Nit: Unnecessary space before '&'.

https://codereview.chromium.org/139803012/diff/130001/Source/bindings/v8/Dict...
Source/bindings/v8/Dictionary.h:326: template<typename T> bool
Dictionary::convert(ConversionContext& context, const String& key, RawPtr<T>&
value) const

Nit: It's a shame that we have to duplicate the convert method a lot of times.
We should templatize the convert methods at some point.

https://codereview.chromium.org/139803012/diff/130001/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/heap/Handle.h#ne...
Source/heap/Handle.h:672: template<typename T> inline T* getPtr(const
WebCore::Member<T>& p)

I'm just curious: Where is this method needed?

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechGrammarList.cpp (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechGrammarList.cpp:68: #if ENABLE(OILPAN)

Is this #if needed?

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechGrammarList.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechGrammarList.h:55:
Vector<RefPtrWillBeMember<SpeechGrammar> > m_grammars;

This works, but we want to use
WillBeHeapVector<RefPtrWillBeMember<SpeechGrammar>>.

We're moving off-heap collections to on-heap collections.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.h:55: // FIXME (oilpan transition):
tidier way to express?

Use "FIXME: oilpan:". Blink doesn't use "FIXME(xxx)".

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.h:59: PassRefPtr<SpeechGrammarList>
grammars() { return m_grammars; }
On 2014/02/12 04:02:06, tkent wrote:
> Returning PassRefPtr looks a bug of the original code.  This should return
> SpeechGrammarList*.
> 

Yeah, let's fix this before landing this CL so that we don't need to add the #if
flag.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionEvent.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionEvent.h:39: struct
SpeechRecognitionEventInit : public EventInit {

Shall we add DISALLOW_ALLOCATION to indicate that this struct is allocated on
stack only? (Otherwise it's not safe to use a RawPtr.)

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResult.cpp (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.cpp:59: // Manual copying
Vector<RawPtr> to Vector<Member>.

That's why we want to use WillBeHeapVector<RefPtrWillBeMember>. Then we don't
need this kind of special code for oilpan. We can just copy the vector by
m_alternatives(alternatives).

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.cpp:68: #if ENABLE(OILPAN)

Is this #if needed?

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResult.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.h:41: // FIXME (oilpan): the
platform outer layer depends on holding a RefPtr to a result.

FIXME: oilpan:

Also move this FIXME to just above the "class SpeechRecognitionResult" line.
This explains why we have to use RefCountedGarbagedCollected.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.h:41: // FIXME (oilpan): the
platform outer layer depends on holding a RefPtr to a result.

BTW, who retains a RefPtr to the SpeechRecognitionResult?

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.h:43: static
PassRefPtr<SpeechRecognitionResult> create(const
Vector<RefPtrWillBeRawPtr<SpeechRecognitionAlternative> >&, bool final);

This could be WillBeHeapVector<RefPtrWillBeMember<>> (but I'm not sure if it's
easy to use WillBeHeapVector<RefPtrWillBeMember<>> in this CL since it will
depend on how the Vector is used in caller sites.)

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.h:52:
SpeechRecognitionResult(const
Vector<RefPtrWillBeRawPtr<SpeechRecognitionAlternative> >&, bool final);

Ditto.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.h:55:
Vector<RefPtrWillBeMember<SpeechRecognitionAlternative> > m_alternatives;

This works, but we want to use WillBeHeapVector<RefPtrWillBeMember<>>.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResultList.cpp (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResultList.cpp:54:
m_results.grow(results.size());

Ditto. We want to avoid the copying code. We want to replace all
Vector<RefPtr<SpeechRecognitionResult>> in the code base with
WillBeHeapVector<RefPtrWillBeMember<SpeechRecognitionResult>>.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResultList.cpp:62: #if ENABLE(OILPAN)

Is this #if needed?

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResultList.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResultList.h:50:
Vector<RefPtrWillBeMember<SpeechRecognitionResult> > m_results;

WillBeHeapVector<RefPtrWillBeMember<>>.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechSynthesis.cpp (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesis.cpp:234: #if ENABLE(OILPAN)

Is this #if needed?

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesis.cpp:239: visitor->trace(*it);

Can't you write this like visitor->trace(m_utteranceQueue) ? If no, we should
add the trace method to Handle.h.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechSynthesis.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesis.h:90:
Vector<RefPtrWillBeMember<SpeechSynthesisVoice> > m_voiceList;

WillBeHeapVector<RefPtrWillBeMember<>>.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesis.h:92:
Deque<RefPtrWillBeMember<SpeechSynthesisUtterance> > m_utteranceQueue;

We haven't yet supported HeapDeque, so it's OK to use
Deque<RefPtrWillBeMember<>> at the moment.

https://codereview.chromium.org/139803012/diff/130001/Source/web/WebSpeechRec...
File Source/web/WebSpeechRecognitionResult.cpp (right):

https://codereview.chromium.org/139803012/diff/130001/Source/web/WebSpeechRec...
Source/web/WebSpeechRecognitionResult.cpp:37: // Move them into WebCore for the
benefit of template aliases.

Agreed.

[sof, 2014-02-12 07:48:50]

Good stuff, thanks; will rebase and pick up the WillBeHeapVector transition
type, which should tidy things up further.

https://codereview.chromium.org/139803012/diff/130001/Source/bindings/v8/Dict...
File Source/bindings/v8/Dictionary.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/bindings/v8/Dict...
Source/bindings/v8/Dictionary.h:165: bool convert(ConversionContext &, const
String&, RawPtr<T>&) const;
On 2014/02/12 05:18:45, haraken wrote:
> 
> Nit: Unnecessary space before '&'.

Well spotted, fixed.

https://codereview.chromium.org/139803012/diff/130001/Source/bindings/v8/Dict...
Source/bindings/v8/Dictionary.h:326: template<typename T> bool
Dictionary::convert(ConversionContext& context, const String& key, RawPtr<T>&
value) const
On 2014/02/12 05:18:45, haraken wrote:
> 
> Nit: It's a shame that we have to duplicate the convert method a lot of times.
> We should templatize the convert methods at some point.

Seems ripe for that; we can handle that separately.

https://codereview.chromium.org/139803012/diff/130001/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/heap/Handle.h#ne...
Source/heap/Handle.h:672: template<typename T> inline T* getPtr(const
WebCore::Member<T>& p)
On 2014/02/12 05:18:45, haraken wrote:
> 
> I'm just curious: Where is this method needed?

Needed for 

interface SpeechSynthesis {
   ...
   sequence<SpeechSynthesisVoice> getVoices()
};

where the underlying member has type
Vector<RefPtrWillBeMember<SpeechSynthesisVoice> >.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechGrammarList.cpp (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechGrammarList.cpp:68: #if ENABLE(OILPAN)
On 2014/02/12 05:18:45, haraken wrote:
> 
> Is this #if needed?

Yes, I couldn't get it to compile without:

../../third_party/WebKit/Source/heap/Visitor.h:187:9: error: 'class
WTF::RefPtr<WebCore::SpeechGrammar>' has no member named 'trace'

(from
http://build.chromium.org/p/tryserver.chromium/builders/linux_blink_rel/build...
)

(Perhaps that is addressed by merely rebasing..)

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechGrammarList.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechGrammarList.h:55:
Vector<RefPtrWillBeMember<SpeechGrammar> > m_grammars;
On 2014/02/12 05:18:45, haraken wrote:
> 
> This works, but we want to use
> WillBeHeapVector<RefPtrWillBeMember<SpeechGrammar>>.
> 
> We're moving off-heap collections to on-heap collections.

Ah, I didn't see 

  https://codereview.chromium.org/150013006

go by after I started speech/. Great; will rebase & switch over the various
Vector<> uses here.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.h:59: PassRefPtr<SpeechGrammarList>
grammars() { return m_grammars; }
On 2014/02/12 05:18:45, haraken wrote:
> On 2014/02/12 04:02:06, tkent wrote:
> > Returning PassRefPtr looks a bug of the original code.  This should return
> > SpeechGrammarList*.
> > 
> 
> Yeah, let's fix this before landing this CL so that we don't need to add the
#if
> flag.

Good, good; switched to SpeechGrammarList*.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionEvent.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionEvent.h:39: struct
SpeechRecognitionEventInit : public EventInit {
On 2014/02/12 05:18:45, haraken wrote:
> 
> Shall we add DISALLOW_ALLOCATION to indicate that this struct is allocated on
> stack only? (Otherwise it's not safe to use a RawPtr.)

So it would be, interesting. Let's do that, it doesn't need to go on the heap.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResult.cpp (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.cpp:68: #if ENABLE(OILPAN)
On 2014/02/12 05:18:45, haraken wrote:
> 
> Is this #if needed?

It's like the earlier one, not being able to handle the inner type of the
Vector<> --

 ../../third_party/WebKit/Source/heap/Visitor.h:187:9: error: 'class
WTF::RefPtr<WebCore::SpeechRecognitionAlternative>' has no member named 'trace'

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResult.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.h:41: // FIXME (oilpan): the
platform outer layer depends on holding a RefPtr to a result.
On 2014/02/12 05:18:45, haraken wrote:
> 
> BTW, who retains a RefPtr to the SpeechRecognitionResult?

WebSpeechRecognitionResult holds a WebPrivatePtr to it.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.h:41: // FIXME (oilpan): the
platform outer layer depends on holding a RefPtr to a result.
On 2014/02/12 05:18:45, haraken wrote:
> 
> FIXME: oilpan:
> 
> Also move this FIXME to just above the "class SpeechRecognitionResult" line.
> This explains why we have to use RefCountedGarbagedCollected.

Done + added a mention of WebSpeecRecognitionResult.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResultList.cpp (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResultList.cpp:62: #if ENABLE(OILPAN)
On 2014/02/12 05:18:45, haraken wrote:
> 
> Is this #if needed?

Like the previous two involving Vector<RefPtr> in enable_oilpan=0 mode; see 


http://build.chromium.org/p/tryserver.chromium/builders/linux_blink_rel/build...

for the exact compile error

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechSynthesis.cpp (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesis.cpp:234: #if ENABLE(OILPAN)
On 2014/02/12 05:18:45, haraken wrote:
> 
> Is this #if needed?

Think so; one will be a Vector<RefPtr>, the other a RawPtr<>.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesis.cpp:239: visitor->trace(*it);
On 2014/02/12 05:18:45, haraken wrote:
> 
> Can't you write this like visitor->trace(m_utteranceQueue) ? If no, we should
> add the trace method to Handle.h.

This is over the Deque<>; should we add a trace() method for it, but not have a
WillBeHeapDeque transition type?

[haraken, 2014-02-12 08:04:49]

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechGrammarList.cpp (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechGrammarList.cpp:68: #if ENABLE(OILPAN)
On 2014/02/12 07:48:51, sof wrote:
> On 2014/02/12 05:18:45, haraken wrote:
> > 
> > Is this #if needed?
> 
> Yes, I couldn't get it to compile without:
> 
> ../../third_party/WebKit/Source/heap/Visitor.h:187:9: error: 'class
> WTF::RefPtr<WebCore::SpeechGrammar>' has no member named 'trace'
> 
> (from
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_blink_rel/build...
> )
> 
> (Perhaps that is addressed by merely rebasing..)

Shall we add trace() to RefPtr.h? It wouldn't be nice we (sometimes) have to add
#if to trace methods.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResult.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.h:41: // FIXME (oilpan): the
platform outer layer depends on holding a RefPtr to a result.
On 2014/02/12 07:48:51, sof wrote:
> On 2014/02/12 05:18:45, haraken wrote:
> > 
> > BTW, who retains a RefPtr to the SpeechRecognitionResult?
> 
> WebSpeechRecognitionResult holds a WebPrivatePtr to it.

I see. Let's fix it in a follow-up CL. You can refer to
Source/WebKit/chromium/public/WebRange.h in the experimental branch and see how
to address the WebPrivatePtr issue.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechSynthesis.cpp (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesis.cpp:239: visitor->trace(*it);
On 2014/02/12 07:48:51, sof wrote:
> On 2014/02/12 05:18:45, haraken wrote:
> > 
> > Can't you write this like visitor->trace(m_utteranceQueue) ? If no, we
should
> > add the trace method to Handle.h.
> 
> This is over the Deque<>; should we add a trace() method for it, but not have
a
> WillBeHeapDeque transition type?

I think we have trace() methods for Vector<Member> and HashSet<Member>, so it
would make sense to add a trace() method for Deque<Member> as well.

On the other hand, I begin to think that it's confusing to have trace() methods
for Vector<Member>, HashSet<Member> etc, since they shouldn't be used
(basically). They should be HeapVector<Member>, HashSet<Member> etc.

[sof, 2014-02-12 10:17:08]

Sorry about the delay, it takes a while to verify changes with&without Oilpan
enabled.

I've not added RefPtr<> trace overload, at least not yet. So there are
conditional trace() implementations here still. Is this something we want to do?
IMHO, not generating dead code inside trace()s with Oilpan off is acceptable.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResult.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.h:43: static
PassRefPtr<SpeechRecognitionResult> create(const
Vector<RefPtrWillBeRawPtr<SpeechRecognitionAlternative> >&, bool final);
On 2014/02/12 05:18:45, haraken wrote:
> 
> This could be WillBeHeapVector<RefPtrWillBeMember<>> (but I'm not sure if it's
> easy to use WillBeHeapVector<RefPtrWillBeMember<>> in this CL since it will
> depend on how the Vector is used in caller sites.)

I think it worked out (with side benefit of avoiding manual vector copying in
the constructor); please check that the WebSpeechRecognitionResult::assign()
invocation is acceptable.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.h:52:
SpeechRecognitionResult(const
Vector<RefPtrWillBeRawPtr<SpeechRecognitionAlternative> >&, bool final);
On 2014/02/12 05:18:45, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.h:55:
Vector<RefPtrWillBeMember<SpeechRecognitionAlternative> > m_alternatives;
On 2014/02/12 05:18:45, haraken wrote:
> 
> This works, but we want to use WillBeHeapVector<RefPtrWillBeMember<>>.

Done.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResultList.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResultList.h:50:
Vector<RefPtrWillBeMember<SpeechRecognitionResult> > m_results;
On 2014/02/12 05:18:45, haraken wrote:
> 
> WillBeHeapVector<RefPtrWillBeMember<>>.

Done.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechSynthesis.cpp (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesis.cpp:239: visitor->trace(*it);
On 2014/02/12 08:04:50, haraken wrote:
> On 2014/02/12 07:48:51, sof wrote:
> > On 2014/02/12 05:18:45, haraken wrote:
> > > 
> > > Can't you write this like visitor->trace(m_utteranceQueue) ? If no, we
> should
> > > add the trace method to Handle.h.
> > 
> > This is over the Deque<>; should we add a trace() method for it, but not
have
> a
> > WillBeHeapDeque transition type?
> 
> I think we have trace() methods for Vector<Member> and HashSet<Member>, so it
> would make sense to add a trace() method for Deque<Member> as well.
> 
> On the other hand, I begin to think that it's confusing to have trace()
methods
> for Vector<Member>, HashSet<Member> etc, since they shouldn't be used
> (basically). They should be HeapVector<Member>, HashSet<Member> etc.

Sorry, I spoke incorrectly. There is a Deque<> trace(), the problem here is/was
that the Deque<> will hold RefPtrs, which we cannot automatically trace.

I've updated the code to use the Deque's trace() (iff ENABLE(OILPAN).)

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechSynthesis.h (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesis.h:90:
Vector<RefPtrWillBeMember<SpeechSynthesisVoice> > m_voiceList;
On 2014/02/12 05:18:45, haraken wrote:
> 
> WillBeHeapVector<RefPtrWillBeMember<>>.

Done.

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesis.h:92:
Deque<RefPtrWillBeMember<SpeechSynthesisUtterance> > m_utteranceQueue;
On 2014/02/12 05:18:45, haraken wrote:
> 
> We haven't yet supported HeapDeque, so it's OK to use
> Deque<RefPtrWillBeMember<>> at the moment.

Done.

https://codereview.chromium.org/139803012/diff/280001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/139803012/diff/280001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:118:
Vector<RefPtr<SpeechRecognitionResult> > results(m_finalResults.size());
This is suboptimal, but it will require changing
web/SpeechRecognitionClientProxy to allocate a vector of member results. I held
off from doing that.

[haraken, 2014-02-12 11:12:53]

Actually I want to have one more reviewer to take a look at this CL. It's hard
to verify correctness with human eyes without Ian's plugin :)


> I've not added RefPtr<> trace overload, at least not yet. So there are
> conditional trace() implementations here still. Is this something we want to
do?

I think it would be nicer if we don't need to add #if branches to trace()
methods.

> IMHO, not generating dead code inside trace()s with Oilpan off is acceptable.

hmm, what do you mean specifically?

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:118:
Vector<RefPtr<SpeechRecognitionResult> > results(m_finalResults.size());

Can you simply use WillBeHeapVector<RefPtrWillBeMember<>> ? A HeapVector on a
stack is visited by the conservative scanning.

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:198: visitor->trace(m_grammars);

You're missing visitor->trace(m_finalResults).

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.h (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.h:120:
WillBeHeapVector<RefPtr<SpeechRecognitionResult> > m_finalResults;

This should be WillBeHeapVector<RefPtrWillBeMember<SpeechRecognitionResult>>

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResultList.cpp (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResultList.cpp:47:
SpeechRecognitionResultList::SpeechRecognitionResultList(const
Vector<RefPtr<SpeechRecognitionResult> >& results)

Is it hard to change this Vector<RefPtr> to WillBeHeapVector<RefPtrWillBeMember>
in this CL?

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechSynthesis.cpp (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesis.cpp:130:
RefPtrWillBeMember<SpeechSynthesisUtterance> current = m_currentSpeechUtterance;

This works, but we prefer using RawPtrs on a stack as much as possible. So
RefPtrWillBeMember => RefPtrWillBeRawPtr.

Pointers on a stack => RawPtr (which will eventually be converted to a real raw
pointer)
Pointers in an object => Member

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesis.cpp:167:
RefPtrWillBeMember<SpeechSynthesisUtterance> firstUtterance =
m_utteranceQueue.first();

Ditto. RefPtrWillBeRawPtr.

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechSynthesisVoice.h (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesisVoice.h:38: class SpeechSynthesisVoice
FINAL : public RefCountedWillBeGarbageCollected<SpeechSynthesisVoice>, public
ScriptWrappable {

RefCountedWillBeGarbageCollected => RefCountedWillBeGarbageCollectedFinalized.

This object has a RefPtr, which needs a destructor.

[zerny-chromium, 2014-02-12 11:44:53]

On 2014/02/12 11:12:53, haraken wrote:
> Actually I want to have one more reviewer to take a look at this CL. It's hard
> to verify correctness with human eyes without Ian's plugin :)

Indeed. I'll land basic tests this week, but we will need to compile clang
locally to start with.

> > I've not added RefPtr<> trace overload, at least not yet. So there are
> > conditional trace() implementations here still. Is this something we want to
> do?
> 
> I think it would be nicer if we don't need to add #if branches to trace()
> methods.

Yes. I thought we had a case for RefPtr already exactly for that.

[zerny-chromium, 2014-02-12 11:45:18]

A few more comments:

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechGrammarList.cpp (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechGrammarList.cpp:68: #if ENABLE(OILPAN)
Nit: the #if should not be needed here.

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:44:
RefPtrWillBeRawPtr<SpeechRecognition> speechRecognition(adoptRefWillBeNoop(new
SpeechRecognition(context)));
This needs to be adoptRefCountedWillBeRefCountedGarbageCollected so long as the
GC'ed type is RefCountedGarbageCollected.

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:120: results[i] = m_finalResults[i];
We should mark such oilpan introduced deficiencies with a FIXME: oilpan: so we
can find a fix when possible.

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:198: visitor->trace(m_grammars);
This needs to trace m_finalResults (if it becomes a heap-allocated vector)

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.h (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.h:120:
WillBeHeapVector<RefPtr<SpeechRecognitionResult> > m_finalResults;
This should just stay as a non-heap vector until the elements actually need
tracing. Also, when declared as a heap-allocated vector the member needs to be
traced to keep the backing store alive.

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResult.cpp (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.cpp:40: return adoptRef(new
SpeechRecognitionResult(alternatives, final));
Needs to be: adoptRefCountedWillBeRefCountedGarbageCollected

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResult.cpp:60: #if ENABLE(OILPAN)
This #if should be unneeded.

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResultList.cpp (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResultList.cpp:53: #if ENABLE(OILPAN)
Nit: maybe a add a fixme so we are sure to remove the copying again.

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResultList.cpp:62: #if ENABLE(OILPAN)
Nit: should not be needed

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechSynthesis.cpp (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesis.cpp:41: return adoptRefWillBeNoop(new
SpeechSynthesis(context));
adoptRefWillBeRefCountedGarbageCollected (alas, it hurts to write...)

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesis.cpp:234: #if ENABLE(OILPAN)
Nit: should not be needed

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechSynthesisUtterance.cpp (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesisUtterance.cpp:35: return
adoptRefWillBeNoop(new SpeechSynthesisUtterance(context, text));
adoptRefCountedWillBeGarbageCollected

[sof, 2014-02-12 11:48:32]

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechGrammarList.cpp (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechGrammarList.cpp:68: #if ENABLE(OILPAN)
On 2014/02/12 11:45:19, zerny-chromium wrote:
> Nit: the #if should not be needed here.

It is & was needed; not sure what you are suggesting.

[zerny-chromium, 2014-02-12 11:50:19]

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechGrammarList.cpp (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechGrammarList.cpp:68: #if ENABLE(OILPAN)
On 2014/02/12 11:48:33, sof wrote:
> On 2014/02/12 11:45:19, zerny-chromium wrote:
> > Nit: the #if should not be needed here.
> 
> It is & was needed; not sure what you are suggesting.

Does this give a compile error on a non-oilpan build? The visitor is fully
capable of tracing non-heap allocated collections (and needs to be because they
can contain heap-allocated elements).

[sof, 2014-02-12 11:54:43]

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechGrammarList.cpp (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechGrammarList.cpp:68: #if ENABLE(OILPAN)
On 2014/02/12 11:50:20, zerny-chromium wrote:
> On 2014/02/12 11:48:33, sof wrote:
> > On 2014/02/12 11:45:19, zerny-chromium wrote:
> > > Nit: the #if should not be needed here.
> > 
> > It is & was needed; not sure what you are suggesting.
> 
> Does this give a compile error on a non-oilpan build? The visitor is fully
> capable of tracing non-heap allocated collections (and needs to be because
they
> can contain heap-allocated elements).

As explained against an earlier patchset,


https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...

[sof, 2014-02-12 12:00:22]

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:118:
Vector<RefPtr<SpeechRecognitionResult> > results(m_finalResults.size());
On 2014/02/12 11:12:54, haraken wrote:
> 
> Can you simply use WillBeHeapVector<RefPtrWillBeMember<>> ? A HeapVector on a
> stack is visited by the conservative scanning.

As long as SpeechRecognitionEvent hasn't been moved over, passing in a
heap-allocated vector would be problematic? Unless we have it use a Persistent.

[zerny-chromium, 2014-02-12 12:13:41]

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechGrammarList.cpp (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechGrammarList.cpp:68: #if ENABLE(OILPAN)
On 2014/02/12 08:04:50, haraken wrote:
> On 2014/02/12 07:48:51, sof wrote:
> > On 2014/02/12 05:18:45, haraken wrote:
> > > 
> > > Is this #if needed?
> > 
> > Yes, I couldn't get it to compile without:
> > 
> > ../../third_party/WebKit/Source/heap/Visitor.h:187:9: error: 'class
> > WTF::RefPtr<WebCore::SpeechGrammar>' has no member named 'trace'
> > 
> > (from
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_blink_rel/build...
> > )
> > 
> > (Perhaps that is addressed by merely rebasing..)
> 
> Shall we add trace() to RefPtr.h? It wouldn't be nice we (sometimes) have to
add
> #if to trace methods.

Ah, we need to fix this for tracing elements that end up being RefPtr's. We
should be able to do this with one of the traits local to visitor. I think that
would be better then adding a trace to RefPtr.

[haraken, 2014-02-12 12:14:59]

> > Shall we add trace() to RefPtr.h? It wouldn't be nice we (sometimes) have to
> add
> > #if to trace methods.
> 
> Ah, we need to fix this for tracing elements that end up being RefPtr's. We
> should be able to do this with one of the traits local to visitor. I think
that
> would be better then adding a trace to RefPtr.

Agreed.

[zerny-chromium, 2014-02-12 12:16:44]

On 2014/02/12 12:14:59, haraken wrote:
> > > Shall we add trace() to RefPtr.h? It wouldn't be nice we (sometimes) have
to
> > add
> > > #if to trace methods.
> > 
> > Ah, we need to fix this for tracing elements that end up being RefPtr's. We
> > should be able to do this with one of the traits local to visitor. I think
> that
> > would be better then adding a trace to RefPtr.
> 
> Agreed.

FYI, Gustav is looking at fixing this issue in his current CL.

[sof, 2014-02-12 12:16:54]

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechGrammarList.cpp (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechGrammarList.cpp:68: #if ENABLE(OILPAN)
On 2014/02/12 12:13:41, zerny-chromium wrote:
> On 2014/02/12 08:04:50, haraken wrote:
> > On 2014/02/12 07:48:51, sof wrote:
> > > On 2014/02/12 05:18:45, haraken wrote:
> > > > 
> > > > Is this #if needed?
> > > 
> > > Yes, I couldn't get it to compile without:
> > > 
> > > ../../third_party/WebKit/Source/heap/Visitor.h:187:9: error: 'class
> > > WTF::RefPtr<WebCore::SpeechGrammar>' has no member named 'trace'
> > > 
> > > (from
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_blink_rel/build...
> > > )
> > > 
> > > (Perhaps that is addressed by merely rebasing..)
> > 
> > Shall we add trace() to RefPtr.h? It wouldn't be nice we (sometimes) have to
> add
> > #if to trace methods.
> 
> Ah, we need to fix this for tracing elements that end up being RefPtr's. We
> should be able to do this with one of the traits local to visitor. I think
that
> would be better then adding a trace to RefPtr.

alright, will try to bring that about.

[sof, 2014-02-12 16:18:12]

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
File Source/modules/speech/SpeechGrammarList.cpp (right):

https://codereview.chromium.org/139803012/diff/130001/Source/modules/speech/S...
Source/modules/speech/SpeechGrammarList.cpp:68: #if ENABLE(OILPAN)
On 2014/02/12 12:16:55, sof wrote:
> On 2014/02/12 12:13:41, zerny-chromium wrote:
> > On 2014/02/12 08:04:50, haraken wrote:
> > > On 2014/02/12 07:48:51, sof wrote:
> > > > On 2014/02/12 05:18:45, haraken wrote:
> > > > > 
> > > > > Is this #if needed?
> > > > 
> > > > Yes, I couldn't get it to compile without:
> > > > 
> > > > ../../third_party/WebKit/Source/heap/Visitor.h:187:9: error: 'class
> > > > WTF::RefPtr<WebCore::SpeechGrammar>' has no member named 'trace'
> > > > 
> > > > (from
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_blink_rel/build...
> > > > )
> > > > 
> > > > (Perhaps that is addressed by merely rebasing..)
> > > 
> > > Shall we add trace() to RefPtr.h? It wouldn't be nice we (sometimes) have
to
> > add
> > > #if to trace methods.
> > 
> > Ah, we need to fix this for tracing elements that end up being RefPtr's. We
> > should be able to do this with one of the traits local to visitor. I think
> that
> > would be better then adding a trace to RefPtr.
> 
> alright, will try to bring that about.

Left in the conditional ENABLE(OILPAN) in the trace() methods for now, spent too
much time with these layered traits.

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:44:
RefPtrWillBeRawPtr<SpeechRecognition> speechRecognition(adoptRefWillBeNoop(new
SpeechRecognition(context)));
On 2014/02/12 11:45:19, zerny-chromium wrote:
> This needs to be adoptRefCountedWillBeRefCountedGarbageCollected so long as
the
> GC'ed type is RefCountedGarbageCollected.

Thanks, done (and for the other types that are based on
RefCountedGarbageCollected.)

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:118:
Vector<RefPtr<SpeechRecognitionResult> > results(m_finalResults.size());
On 2014/02/12 12:00:23, sof wrote:
> On 2014/02/12 11:12:54, haraken wrote:
> > 
> > Can you simply use WillBeHeapVector<RefPtrWillBeMember<>> ? A HeapVector on
a
> > stack is visited by the conservative scanning.
> 
> As long as SpeechRecognitionEvent hasn't been moved over, passing in a
> heap-allocated vector would be problematic? Unless we have it use a
Persistent.

I took another pop at this, but the inter-relationships between the types here
is somewhat tight. Until the platform API layer and its WebPrivatePtrs have been
converted, I don't see how this can be done.

To go with that, m_finalResults is now a Vector<>, so the manual copying here
isn't needed.

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:120: results[i] = m_finalResults[i];
On 2014/02/12 11:45:19, zerny-chromium wrote:
> We should mark such oilpan introduced deficiencies with a FIXME: oilpan: so we
> can find a fix when possible.

Alright, thanks. The potential FIXME disappears here w/ m_finalResults no longer
be a heap vector.

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:198: visitor->trace(m_grammars);
On 2014/02/12 11:45:19, zerny-chromium wrote:
> This needs to trace m_finalResults (if it becomes a heap-allocated vector)

I missed the reqd tracing of it, sorry. But it is not heap allocated now.

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.h (right):

https://codereview.chromium.org/139803012/diff/320001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.h:120:
WillBeHeapVector<RefPtr<SpeechRecognitionResult> > m_finalResults;
On 2014/02/12 11:45:19, zerny-chromium wrote:
> This should just stay as a non-heap vector until the elements actually need
> tracing. Also, when declared as a heap-allocated vector the member needs to be
> traced to keep the backing store alive.

Changed back to a Vector<>

[sof, 2014-02-12 21:17:57]

I pulled in the TraceTrait<RefPtr<T>> specialization from
https://codereview.chromium.org/148523016/ (which has been temporarily reverted)
-- my attempts to have the Vector<> specialization use
CollectionBackingTraceTrait<> didn't go anywhere.

Also needed a trace(RawPtr<>) overload to handle RawPtrWillBeMember tracing.

The handful of #if ENABLE(OILPAN) uses in the speech/ trace() methods could as a
result be removed. Please take a look.

[haraken, 2014-02-13 01:21:20]

I double-checked. LGTM.

https://codereview.chromium.org/139803012/diff/560001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResultList.cpp (right):

https://codereview.chromium.org/139803012/diff/560001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResultList.cpp:53: #if ENABLE(OILPAN)

Are you planning to remove this code by replacing Vector<RefPtr> to
WillBeHeapVector<RefPtrWillBeMember> in a follow-up CL?

https://codereview.chromium.org/139803012/diff/560001/Source/modules/speech/S...
File Source/modules/speech/SpeechSynthesisVoice.h (right):

https://codereview.chromium.org/139803012/diff/560001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesisVoice.h:38: class SpeechSynthesisVoice
FINAL : public RefCountedWillBeGarbageCollected<SpeechSynthesisVoice>, public
ScriptWrappable {

This should be RefCountedWillBeGarbageCollectedFinalized, because of
ScriptWrappable and m_platformVoice.

[sof, 2014-02-13 06:47:30]

https://codereview.chromium.org/139803012/diff/560001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResultList.cpp (right):

https://codereview.chromium.org/139803012/diff/560001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResultList.cpp:53: #if ENABLE(OILPAN)
On 2014/02/13 01:21:21, haraken wrote:
> 
> Are you planning to remove this code by replacing Vector<RefPtr> to
> WillBeHeapVector<RefPtrWillBeMember> in a follow-up CL?

As I said in a comment somewhere else here, this really depends on addressing
how the Web* wrappers (WebSpeechRecognitionResultList in this case) hold onto GC
references.

So, once it is clear how that should be done, this can be addressed. WebRange.h
was mentioned as an example (on the oilpan branch), but I couldn't see it as
directly solving the issues.

[zerny-chromium, 2014-02-13 07:02:27]

lgtm

https://codereview.chromium.org/139803012/diff/560001/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/139803012/diff/560001/Source/heap/Handle.h#ne...
Source/heap/Handle.h:399: ASSERT_NOT_REACHED();
As you pointed out, this might not always be true if we have a heap collection
of RefPtrs. It would be weird to have a HeapVector<RefPtr<T>> but it is
reasonable to have a HeapHashMap where either the key or the value elements
remain in RefPtrs. We'll discuss this locally, so lets keep the assert here for
now.

https://codereview.chromium.org/139803012/diff/560001/Source/heap/Visitor.h
File Source/heap/Visitor.h (right):

https://codereview.chromium.org/139803012/diff/560001/Source/heap/Visitor.h#n...
Source/heap/Visitor.h:330: ASSERT_NOT_REACHED();
I think this should be the case, since the only "real" raw ptrs we trace are
union members and they are not wrapped in RawPtr. Also, I can think of no good
reason to put RawPtr in heap-allocated collections.

[sof, 2014-02-13 07:52:38]

https://codereview.chromium.org/139803012/diff/560001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResultList.cpp (right):

https://codereview.chromium.org/139803012/diff/560001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResultList.cpp:53: #if ENABLE(OILPAN)
On 2014/02/13 06:47:31, sof wrote:
> On 2014/02/13 01:21:21, haraken wrote:
> > 
> > Are you planning to remove this code by replacing Vector<RefPtr> to
> > WillBeHeapVector<RefPtrWillBeMember> in a follow-up CL?
> 
> As I said in a comment somewhere else here, this really depends on addressing
> how the Web* wrappers (WebSpeechRecognitionResultList in this case) hold onto
GC
> references.
> 
> So, once it is clear how that should be done, this can be addressed.
WebRange.h
> was mentioned as an example (on the oilpan branch), but I couldn't see it as
> directly solving the issues.

..and if it wasn't clear, I definitely want to tidy this bit up. :)

https://codereview.chromium.org/139803012/diff/560001/Source/modules/speech/S...
File Source/modules/speech/SpeechSynthesisVoice.h (right):

https://codereview.chromium.org/139803012/diff/560001/Source/modules/speech/S...
Source/modules/speech/SpeechSynthesisVoice.h:38: class SpeechSynthesisVoice
FINAL : public RefCountedWillBeGarbageCollected<SpeechSynthesisVoice>, public
ScriptWrappable {
On 2014/02/13 01:21:21, haraken wrote:
> 
> This should be RefCountedWillBeGarbageCollectedFinalized, because of
> ScriptWrappable and m_platformVoice.

Fixed, thanks.

[sof, 2014-02-13 08:50:09]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-02-13 08:50:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/139803012/660001

[commit-bot: I haz the power, 2014-02-13 09:34:39]

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

[commit-bot: I haz the power, 2014-02-13 09:34:40]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[sof, 2014-02-13 09:38:23]

On 2014/02/13 09:34:40, I haz the power (commit-bot) wrote:
> Retried try job too often on blink_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

Source/web/ change needs approval. tkent - could you have a look at some point?

[haraken, 2014-02-13 09:39:56]

tkent is off today.

jochen@: Could you rubberstamp to web/ ?

[jochen (gone - plz use gerrit), 2014-02-13 09:50:13]

lgtm

[sof, 2014-02-13 09:51:02]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-02-13 09:51:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/139803012/660001

[commit-bot: I haz the power, 2014-02-13 13:00:08]

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

[commit-bot: I haz the power, 2014-02-13 13:00:09]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[sof, 2014-02-13 13:07:51]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-02-13 13:08:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/139803012/660001

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

Change committed as 167088

[loislo, 2014-02-13 14:24:43]

A revert of this CL has been created in
https://codereview.chromium.org/163493003/ by loislo@chromium.org.

The reason for reverting is: compilation failed.

[loislo, 2014-02-13 14:25:20]

On 2014/02/13 14:24:43, loislo wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/163493003/ by mailto:loislo@chromium.org.
> 
> The reason for reverting is: compilation failed.

FAILED: /Volumes/data/b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/third_party/WebKit/Source/heap/blink_heap.Visitor.o.d
-DV8_DEPRECATION_WARNINGS -DCHROMIUM_BUILD -DUSE_LIBJPEG_TURBO=1
-DENABLE_ONE_CLICK_SIGNIN -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1
-DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH
-DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1
-DDISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
-DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DENABLE_EGLIMAGE=1
-DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1
-DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1
-DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1
-DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 -DENABLE_FULL_PRINTING=1
-DENABLE_PRINTING=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1
-DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_MANAGED_USERS=1
-DHEAP_IMPLEMENTATION=1 -DINSIDE_BLINK -DENABLE_CUSTOM_SCHEME_HANDLER=0
-DENABLE_SVG_FONTS=1 -DENABLE_GDI_FONTS_ON_WINDOWS=0
-DENABLE_HARFBUZZ_ON_WINDOWS=1 -DWTF_USE_CONCATENATED_IMPULSE_RESPONSES=1
-DENABLE_INPUT_SPEECH=1 -DENABLE_INPUT_MULTIPLE_FIELDS_UI=1
-DENABLE_LEGACY_NOTIFICATIONS=1 -DENABLE_MEDIA_CAPTURE=0 -DENABLE_WEB_AUDIO=1
-DU_USING_ICU_NAMESPACE=0 -DU_STATIC_IMPLEMENTATION -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0
-D_FORTIFY_SOURCE=2 -I../../third_party/WebKit/Source -I../../third_party/WebKit
-I../../third_party/icu/source/i18n -I../../third_party/icu/source/common
-isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk
-O3 -gdwarf-2 -fvisibility=hidden -Werror -Wnewline-eof
-mmacosx-version-min=10.6 -arch i386 -Wall -Wendif-labels -Wextra
-Wno-unused-parameter -Wno-missing-field-initializers -Wheader-hygiene
-Wno-c++11-narrowing -Wno-char-subscripts -Wno-unneeded-internal-declaration
-Wno-covered-switch-default -Wstring-conversion -Wno-deprecated-register
-Wglobal-constructors -std=gnu++11 -fno-rtti -fno-exceptions
-fvisibility-inlines-hidden -fno-threadsafe-statics -Xclang -load -Xclang
/Volumes/data/b/build/slave/GPU_Mac_Builder/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-Xclang -add-plugin -Xclang find-bad-constructs -fcolor-diagnostics
-fno-strict-aliasing  -c ../../third_party/WebKit/Source/heap/Visitor.cpp -o
obj/third_party/WebKit/Source/heap/blink_heap.Visitor.o
In file included from ../../third_party/WebKit/Source/heap/Visitor.cpp:34:
../../third_party/WebKit/Source/heap/Handle.h:402:7: error: redefinition of
'TraceTrait<RefPtr<type-parameter-0-0> >'
class TraceTrait<RefPtr<T> > {
      ^~~~~~~~~~~~~~~~~~~~~~
../../third_party/WebKit/Source/heap/Handle.h:391:7: note: previous definition
is here
class TraceTrait<RefPtr<T> > {
      ^
1 error generated.
FAILED: /Volumes/data/b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/third_party/WebKit/Source/heap/blink_heap.ThreadState.o.d
-DV8_DEPRECATION_WARNINGS -DCHROMIUM_BUILD -DUSE_LIBJPEG_TURBO=1
-DENABLE_ONE_CLICK_SIGNIN -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1
-DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH
-DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1
-DDISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
-DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DENABLE_EGLIMAGE=1
-DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1
-DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1
-DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1
-DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 -DENABLE_FULL_PRINTING=1
-DENABLE_PRINTING=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1
-DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_MANAGED_USERS=1
-DHEAP_IMPLEMENTATION=1 -DINSIDE_BLINK -DENABLE_CUSTOM_SCHEME_HANDLER=0
-DENABLE_SVG_FONTS=1 -DENABLE_GDI_FONTS_ON_WINDOWS=0
-DENABLE_HARFBUZZ_ON_WINDOWS=1 -DWTF_USE_CONCATENATED_IMPULSE_RESPONSES=1
-DENABLE_INPUT_SPEECH=1 -DENABLE_INPUT_MULTIPLE_FIELDS_UI=1
-DENABLE_LEGACY_NOTIFICATIONS=1 -DENABLE_MEDIA_CAPTURE=0 -DENABLE_WEB_AUDIO=1
-DU_USING_ICU_NAMESPACE=0 -DU_STATIC_IMPLEMENTATION -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0
-D_FORTIFY_SOURCE=2 -I../../third_party/WebKit/Source -I../../third_party/WebKit
-I../../third_party/icu/source/i18n -I../../third_party/icu/source/common
-isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk
-O3 -gdwarf-2 -fvisibility=hidden -Werror -Wnewline-eof
-mmacosx-version-min=10.6 -arch i386 -Wall -Wendif-labels -Wextra
-Wno-unused-parameter -Wno-missing-field-initializers -Wheader-hygiene
-Wno-c++11-narrowing -Wno-char-subscripts -Wno-unneeded-internal-declaration
-Wno-covered-switch-default -Wstring-conversion -Wno-deprecated-register
-Wglobal-constructors -std=gnu++11 -fno-rtti -fno-exceptions
-fvisibility-inlines-hidden -fno-threadsafe-statics -Xclang -load -Xclang
/Volumes/data/b/build/slave/GPU_Mac_Builder/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-Xclang -add-plugin -Xclang find-bad-constructs -fcolor-diagnostics
-fno-strict-aliasing  -c ../../third_party/WebKit/Source/heap/ThreadState.cpp -o
obj/third_party/WebKit/Source/heap/blink_heap.ThreadState.o
In file included from ../../third_party/WebKit/Source/heap/ThreadState.cpp:34:
../../third_party/WebKit/Source/heap/Handle.h:402:7: error: redefinition of
'TraceTrait<RefPtr<type-parameter-0-0> >'
class TraceTrait<RefPtr<T> > {
      ^~~~~~~~~~~~~~~~~~~~~~
../../third_party/WebKit/Source/heap/Handle.h:391:7: note: previous definition
is here
class TraceTrait<RefPtr<T> > {
      ^
1 error generated.
ninja: build stopped: subcommand failed.

[wibling-chromium, 2014-02-13 14:39:00]

On 2014/02/13 14:25:20, loislo wrote:
> On 2014/02/13 14:24:43, loislo wrote:
> > A revert of this CL has been created in
> > https://codereview.chromium.org/163493003/ by mailto:loislo@chromium.org.
> > 
> > The reason for reverting is: compilation failed.
> 
> FAILED: /Volumes/data/b/build/goma/gomacc
> ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
> obj/third_party/WebKit/Source/heap/blink_heap.Visitor.o.d
> -DV8_DEPRECATION_WARNINGS -DCHROMIUM_BUILD -DUSE_LIBJPEG_TURBO=1
> -DENABLE_ONE_CLICK_SIGNIN -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1
> -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH
> -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1
> -DDISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
> -DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DENABLE_EGLIMAGE=1
> -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1
> -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1
> -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1
> -DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 -DENABLE_FULL_PRINTING=1
> -DENABLE_PRINTING=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1
> -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_MANAGED_USERS=1
> -DHEAP_IMPLEMENTATION=1 -DINSIDE_BLINK -DENABLE_CUSTOM_SCHEME_HANDLER=0
> -DENABLE_SVG_FONTS=1 -DENABLE_GDI_FONTS_ON_WINDOWS=0
> -DENABLE_HARFBUZZ_ON_WINDOWS=1 -DWTF_USE_CONCATENATED_IMPULSE_RESPONSES=1
> -DENABLE_INPUT_SPEECH=1 -DENABLE_INPUT_MULTIPLE_FIELDS_UI=1
> -DENABLE_LEGACY_NOTIFICATIONS=1 -DENABLE_MEDIA_CAPTURE=0 -DENABLE_WEB_AUDIO=1
> -DU_USING_ICU_NAMESPACE=0 -DU_STATIC_IMPLEMENTATION -D__STDC_CONSTANT_MACROS
> -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0
> -D_FORTIFY_SOURCE=2 -I../../third_party/WebKit/Source
-I../../third_party/WebKit
> -I../../third_party/icu/source/i18n -I../../third_party/icu/source/common
> -isysroot
>
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk
> -O3 -gdwarf-2 -fvisibility=hidden -Werror -Wnewline-eof
> -mmacosx-version-min=10.6 -arch i386 -Wall -Wendif-labels -Wextra
> -Wno-unused-parameter -Wno-missing-field-initializers -Wheader-hygiene
> -Wno-c++11-narrowing -Wno-char-subscripts -Wno-unneeded-internal-declaration
> -Wno-covered-switch-default -Wstring-conversion -Wno-deprecated-register
> -Wglobal-constructors -std=gnu++11 -fno-rtti -fno-exceptions
> -fvisibility-inlines-hidden -fno-threadsafe-statics -Xclang -load -Xclang
>
/Volumes/data/b/build/slave/GPU_Mac_Builder/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
> -Xclang -add-plugin -Xclang find-bad-constructs -fcolor-diagnostics
> -fno-strict-aliasing  -c ../../third_party/WebKit/Source/heap/Visitor.cpp -o
> obj/third_party/WebKit/Source/heap/blink_heap.Visitor.o
> In file included from ../../third_party/WebKit/Source/heap/Visitor.cpp:34:
> ../../third_party/WebKit/Source/heap/Handle.h:402:7: error: redefinition of
> 'TraceTrait<RefPtr<type-parameter-0-0> >'
> class TraceTrait<RefPtr<T> > {
>       ^~~~~~~~~~~~~~~~~~~~~~
> ../../third_party/WebKit/Source/heap/Handle.h:391:7: note: previous definition
> is here
> class TraceTrait<RefPtr<T> > {
>       ^
> 1 error generated.
> FAILED: /Volumes/data/b/build/goma/gomacc
> ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
> obj/third_party/WebKit/Source/heap/blink_heap.ThreadState.o.d
> -DV8_DEPRECATION_WARNINGS -DCHROMIUM_BUILD -DUSE_LIBJPEG_TURBO=1
> -DENABLE_ONE_CLICK_SIGNIN -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1
> -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH
> -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1
> -DDISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
> -DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DENABLE_EGLIMAGE=1
> -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1
> -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1
> -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1
> -DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 -DENABLE_FULL_PRINTING=1
> -DENABLE_PRINTING=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1
> -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_MANAGED_USERS=1
> -DHEAP_IMPLEMENTATION=1 -DINSIDE_BLINK -DENABLE_CUSTOM_SCHEME_HANDLER=0
> -DENABLE_SVG_FONTS=1 -DENABLE_GDI_FONTS_ON_WINDOWS=0
> -DENABLE_HARFBUZZ_ON_WINDOWS=1 -DWTF_USE_CONCATENATED_IMPULSE_RESPONSES=1
> -DENABLE_INPUT_SPEECH=1 -DENABLE_INPUT_MULTIPLE_FIELDS_UI=1
> -DENABLE_LEGACY_NOTIFICATIONS=1 -DENABLE_MEDIA_CAPTURE=0 -DENABLE_WEB_AUDIO=1
> -DU_USING_ICU_NAMESPACE=0 -DU_STATIC_IMPLEMENTATION -D__STDC_CONSTANT_MACROS
> -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0
> -D_FORTIFY_SOURCE=2 -I../../third_party/WebKit/Source
-I../../third_party/WebKit
> -I../../third_party/icu/source/i18n -I../../third_party/icu/source/common
> -isysroot
>
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk
> -O3 -gdwarf-2 -fvisibility=hidden -Werror -Wnewline-eof
> -mmacosx-version-min=10.6 -arch i386 -Wall -Wendif-labels -Wextra
> -Wno-unused-parameter -Wno-missing-field-initializers -Wheader-hygiene
> -Wno-c++11-narrowing -Wno-char-subscripts -Wno-unneeded-internal-declaration
> -Wno-covered-switch-default -Wstring-conversion -Wno-deprecated-register
> -Wglobal-constructors -std=gnu++11 -fno-rtti -fno-exceptions
> -fvisibility-inlines-hidden -fno-threadsafe-statics -Xclang -load -Xclang
>
/Volumes/data/b/build/slave/GPU_Mac_Builder/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
> -Xclang -add-plugin -Xclang find-bad-constructs -fcolor-diagnostics
> -fno-strict-aliasing  -c ../../third_party/WebKit/Source/heap/ThreadState.cpp
-o
> obj/third_party/WebKit/Source/heap/blink_heap.ThreadState.o
> In file included from ../../third_party/WebKit/Source/heap/ThreadState.cpp:34:
> ../../third_party/WebKit/Source/heap/Handle.h:402:7: error: redefinition of
> 'TraceTrait<RefPtr<type-parameter-0-0> >'
> class TraceTrait<RefPtr<T> > {
>       ^~~~~~~~~~~~~~~~~~~~~~
> ../../third_party/WebKit/Source/heap/Handle.h:391:7: note: previous definition
> is here
> class TraceTrait<RefPtr<T> > {
>       ^
> 1 error generated.
> ninja: build stopped: subcommand failed.

This is odd. The problem seems to be that both Sigbjorn and I added the same
TraceTrait<RefPtr<T> > in heap/Handle.h, but I saw no merge conflict when trying
to apply my change?

[Vyacheslav Egorov (Chromium), 2014-02-14 14:01:40]

DBC

https://codereview.chromium.org/139803012/diff/660001/Source/heap/Visitor.h
File Source/heap/Visitor.h (right):

https://codereview.chromium.org/139803012/diff/660001/Source/heap/Visitor.h#n...
Source/heap/Visitor.h:330: ASSERT_NOT_REACHED();
This should never be instantiated when ENABLE(OILPAN) is on so I would just wrap
the whole method in the #if or have a compile_assert here.

https://codereview.chromium.org/139803012/diff/660001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.h (right):

https://codereview.chromium.org/139803012/diff/660001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.h:116: SpeechRecognitionController*
m_controller;
Raw pointers inside garbage collected objects are extremely dangerous because GC
objects are not promptly finalized, so lifetime connections between objects are
altered in an unpredictable way. I quickly checked the code and it does not seem
that this can be a problem in this particular case, but I recommend having FIXME
on them or something so that we don't forget them. This should become a member
eventually to ensure 100% safety.

https://codereview.chromium.org/139803012/diff/660001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResultList.cpp (right):

https://codereview.chromium.org/139803012/diff/660001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResultList.cpp:49: : m_results(results)
We could provide a correctly delegating copying constructor to avoid these
changes.

[sof, 2014-02-16 20:29:14]

Thanks for carefully going through & checking the changes made.

https://codereview.chromium.org/139803012/diff/660001/Source/heap/Visitor.h
File Source/heap/Visitor.h (right):

https://codereview.chromium.org/139803012/diff/660001/Source/heap/Visitor.h#n...
Source/heap/Visitor.h:330: ASSERT_NOT_REACHED();
On 2014/02/14 14:01:42, Vyacheslav Egorov wrote:
> This should never be instantiated when ENABLE(OILPAN) is on so I would just
wrap
> the whole method in the #if or have a compile_assert here.

Good idea, https://codereview.chromium.org/167443003/

https://codereview.chromium.org/139803012/diff/660001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResultList.cpp (right):

https://codereview.chromium.org/139803012/diff/660001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResultList.cpp:49: : m_results(results)
On 2014/02/14 14:01:42, Vyacheslav Egorov wrote:
> We could provide a correctly delegating copying constructor to avoid these
> changes.

Yes, the eyesore would disappear then, but I'd be interested to hear suggestions
of how to address the underlying problem here.

That is, has the project worked out how to handle 'Blink platform layer' objects
(I might be using the completely wrong terminology here, sorry, but I refer to
the objects exposed by public/web/ ) that hold references to Oilpan heap
objects? Without exposing the underlying Oilpan types in those interfaces.

[I've been experimenting with a WebPrivateHeapPtr type, which holds a persistent
reference when compiled with Oilpan enabled. It indirectly solves the above.]

[haraken, 2014-02-17 01:21:34]

https://codereview.chromium.org/139803012/diff/660001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionResultList.cpp (right):

https://codereview.chromium.org/139803012/diff/660001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionResultList.cpp:49: : m_results(results)
On 2014/02/16 20:29:15, sof wrote:
> On 2014/02/14 14:01:42, Vyacheslav Egorov wrote:
> > We could provide a correctly delegating copying constructor to avoid these
> > changes.
> 
> Yes, the eyesore would disappear then, but I'd be interested to hear
suggestions
> of how to address the underlying problem here.
> 
> That is, has the project worked out how to handle 'Blink platform layer'
objects
> (I might be using the completely wrong terminology here, sorry, but I refer to
> the objects exposed by public/web/ ) that hold references to Oilpan heap
> objects? Without exposing the underlying Oilpan types in those interfaces.
> 
> [I've been experimenting with a WebPrivateHeapPtr type, which holds a
persistent
> reference when compiled with Oilpan enabled. It indirectly solves the above.]

I think this is the right way to go. You can refer to You can refer to
Source/WebKit/chromium/public/WebRange.h in the experimental branch and see how
to address the WebPrivatePtr issue.

[sof, 2014-02-17 07:41:00]

On 2014/02/17 01:21:34, haraken wrote:
>
https://codereview.chromium.org/139803012/diff/660001/Source/modules/speech/S...
> File Source/modules/speech/SpeechRecognitionResultList.cpp (right):
> 
>
https://codereview.chromium.org/139803012/diff/660001/Source/modules/speech/S...
> Source/modules/speech/SpeechRecognitionResultList.cpp:49: : m_results(results)
> On 2014/02/16 20:29:15, sof wrote:
> > On 2014/02/14 14:01:42, Vyacheslav Egorov wrote:
> > > We could provide a correctly delegating copying constructor to avoid these
> > > changes.
> > 
> > Yes, the eyesore would disappear then, but I'd be interested to hear
> suggestions
> > of how to address the underlying problem here.
> > 
> > That is, has the project worked out how to handle 'Blink platform layer'
> objects
> > (I might be using the completely wrong terminology here, sorry, but I refer
to
> > the objects exposed by public/web/ ) that hold references to Oilpan heap
> > objects? Without exposing the underlying Oilpan types in those interfaces.
> > 
> > [I've been experimenting with a WebPrivateHeapPtr type, which holds a
> persistent
> > reference when compiled with Oilpan enabled. It indirectly solves the
above.]
> 
> I think this is the right way to go. You can refer to You can refer to
> Source/WebKit/chromium/public/WebRange.h in the experimental branch and see
how
> to address the WebPrivatePtr issue.

Thanks; to summarize, the opaque WebRangePrivate that WebRange keeps now
has a Persistent<> around the Range being wrapped.

Should I infer from that that the preferred mechanism is to have WebFoo add
a WebFooPrivate class that Persistent<>-wraps the heap reference? And if
oilpan is disabled, have it behave like a WebPrivatePtr.

[haraken, 2014-02-17 07:44:27]

> Should I infer from that that the preferred mechanism is to have WebFoo add
> a WebFooPrivate class that Persistent<>-wraps the heap reference? And if
> oilpan is disabled, have it behave like a WebPrivatePtr.

Yeah, but I found it hard to share code between the oilpan world and the
non-oilpan world (See my CL: https://codereview.chromium.org/169323002). So I
split the code using the #if ENABLE(OILPAN) flag. Let's wait for feedback to the
CL.

[sof, 2014-02-17 07:56:37]

On 2014/02/17 07:44:27, haraken wrote:
> > Should I infer from that that the preferred mechanism is to have WebFoo add
> > a WebFooPrivate class that Persistent<>-wraps the heap reference? And if
> > oilpan is disabled, have it behave like a WebPrivatePtr.
> 
> Yeah, but I found it hard to share code between the oilpan world and the
> non-oilpan world (See my CL: https://codereview.chromium.org/169323002). So I
> split the code using the #if ENABLE(OILPAN) flag. Let's wait for feedback to
the
> CL.

Will do.

FTR, https://codereview.chromium.org/169343002/ has the experimental bits
adding WebPrivateHeapPtr, along with some uses.