Fix error introduced in r167687 and add copy constructor to PersistentHeapVector.

Fix error introduced in r167687 and add copy constructor to PersistentHeapVector.

R=sigbjornf@opera.com, ager@chromium.org, oilpan-reviews@chromium.org

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

[zerny-chromium, 2014-02-26 09:34:45]

Error identified using the current Blink GC plugin for clang.

PTAL

[Mads Ager (chromium), 2014-02-26 09:38:41]

LGTM

https://codereview.chromium.org/181093003/diff/1/Source/core/html/shadow/Text...
File Source/core/html/shadow/TextControlInnerElements.h (right):

https://codereview.chromium.org/181093003/diff/1/Source/core/html/shadow/Text...
Source/core/html/shadow/TextControlInnerElements.h:137:
PersistentSpeechInputResultArray m_results;
WillBePersistentSpeechInputResultArray to make it clear that there is a
transition type involved here?

[zerny-chromium, 2014-02-26 09:39:52]

On 2014/02/26 09:38:41, Mads Ager (chromium) wrote:
> LGTM
> 
>
https://codereview.chromium.org/181093003/diff/1/Source/core/html/shadow/Text...
> File Source/core/html/shadow/TextControlInnerElements.h (right):
> 
>
https://codereview.chromium.org/181093003/diff/1/Source/core/html/shadow/Text...
> Source/core/html/shadow/TextControlInnerElements.h:137:
> PersistentSpeechInputResultArray m_results;
> WillBePersistentSpeechInputResultArray to make it clear that there is a
> transition type involved here?

Good point. Done.

[haraken, 2014-02-26 09:43:15]

LGTM

https://codereview.chromium.org/181093003/diff/1/Source/core/html/shadow/Text...
File Source/core/html/shadow/TextControlInnerElements.h (right):

https://codereview.chromium.org/181093003/diff/1/Source/core/html/shadow/Text...
Source/core/html/shadow/TextControlInnerElements.h:137:
PersistentSpeechInputResultArray m_results;
On 2014/02/26 09:38:41, Mads Ager (chromium) wrote:
> WillBePersistentSpeechInputResultArray to make it clear that there is a
> transition type involved here?

I slightly disagree. We've not used "WillBe" for typedefs. The advantage of not
using "WillBe" in typedefs is we don't need to edit a lot of places when
switching to real types :)

[Mads Ager (chromium), 2014-02-26 09:49:06]

https://codereview.chromium.org/181093003/diff/1/Source/core/html/shadow/Text...
File Source/core/html/shadow/TextControlInnerElements.h (right):

https://codereview.chromium.org/181093003/diff/1/Source/core/html/shadow/Text...
Source/core/html/shadow/TextControlInnerElements.h:137:
PersistentSpeechInputResultArray m_results;
On 2014/02/26 09:43:15, haraken wrote:
> On 2014/02/26 09:38:41, Mads Ager (chromium) wrote:
> > WillBePersistentSpeechInputResultArray to make it clear that there is a
> > transition type involved here?
> 
> I slightly disagree. We've not used "WillBe" for typedefs. The advantage of
not
> using "WillBe" in typedefs is we don't need to edit a lot of places when
> switching to real types :)

In this particular case, having Persistent in the name might be indication
enough. However, it is not a persistent when compiling without enable_oilpan so
the name is slightly misleading. If we could consistently use
SpeechInputResultArray I agree that we don't need the WillBe prefix. When we
move InputFieldSpeechButtonElement to the heap we can completely remove this new
typedef. It is just temporary naming so I don't feel that strongly about it. :-)

Overall, these typedefs are a little dangerous as this change shows. When using
a transition type in a typedef we really need to find all usages and check that
they are traced with enable_oilpan.

[zerny-chromium, 2014-02-26 10:07:15]

On 2014/02/26 09:49:06, Mads Ager (chromium) wrote:
>
https://codereview.chromium.org/181093003/diff/1/Source/core/html/shadow/Text...
> File Source/core/html/shadow/TextControlInnerElements.h (right):
> 
>
https://codereview.chromium.org/181093003/diff/1/Source/core/html/shadow/Text...
> Source/core/html/shadow/TextControlInnerElements.h:137:
> PersistentSpeechInputResultArray m_results;
> On 2014/02/26 09:43:15, haraken wrote:
> > On 2014/02/26 09:38:41, Mads Ager (chromium) wrote:
> > > WillBePersistentSpeechInputResultArray to make it clear that there is a
> > > transition type involved here?
> > 
> > I slightly disagree. We've not used "WillBe" for typedefs. The advantage of
> not
> > using "WillBe" in typedefs is we don't need to edit a lot of places when
> > switching to real types :)
> 
> In this particular case, having Persistent in the name might be indication
> enough. However, it is not a persistent when compiling without enable_oilpan
so
> the name is slightly misleading. If we could consistently use
> SpeechInputResultArray I agree that we don't need the WillBe prefix. When we
> move InputFieldSpeechButtonElement to the heap we can completely remove this
new
> typedef. It is just temporary naming so I don't feel that strongly about it.
:-)
> 
> Overall, these typedefs are a little dangerous as this change shows. When
using
> a transition type in a typedef we really need to find all usages and check
that
> they are traced with enable_oilpan.

As Mads says, the typedef with WillBePersistent is both transitional and more
explicit, so I'll keep it for now.

[zerny-chromium, 2014-02-26 10:07:31]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-02-26 10:07:46]

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

[haraken, 2014-02-26 10:31:59]

> Overall, these typedefs are a little dangerous as this change shows. When
using
> a transition type in a typedef we really need to find all usages and check
that
> they are traced with enable_oilpan.

Agreed...

- If the typedef is WillBeHeapVector, it's possible that we misuse the typedef
in a place that is not traced.
- If the typedef is WillBePersistentVector, it's possible that we misuse the
typedef and cause memory leaks.
- So, typedef is anyway dangerous in the transition period.

[commit-bot: I haz the power, 2014-02-26 12:23:18]

Change committed as 167902