Oilpan: fix build after r197063, move HitTestCache to the heap.

Oilpan: fix build after r197063, move HitTestCache to the heap.

Follow up r197063 and make HitTestCache compatible with Oilpan.

R=haraken
BUG=398920
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197095

[sof, 2015-06-13 06:09:21]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-06-13 06:09:21]

please take a look.

Please verify that the VectorTraits for HitTestResult is safe for our uses here.

[haraken, 2015-06-13 07:12:21]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-06-13 07:12:21]

https://codereview.chromium.org/1185923002/diff/1/Source/core/layout/HitTestR...
File Source/core/layout/HitTestResult.h (right):

https://codereview.chromium.org/1185923002/diff/1/Source/core/layout/HitTestR...
Source/core/layout/HitTestResult.h:176:
WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS(blink::HitTestResult);

The default constructor of blink::HitTestResult is setting  m_hitTestRequest to
HitTestRequest::ReadOnly|HitTestRequest::Active, but is it ok to initialize it
with memset?

[sof, 2015-06-13 12:30:31]

https://codereview.chromium.org/1185923002/diff/1/Source/core/layout/HitTestR...
File Source/core/layout/HitTestResult.h (right):

https://codereview.chromium.org/1185923002/diff/1/Source/core/layout/HitTestR...
Source/core/layout/HitTestResult.h:176:
WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS(blink::HitTestResult);
On 2015/06/13 07:12:21, haraken wrote:
> 
> The default constructor of blink::HitTestResult is setting  m_hitTestRequest
to
> HitTestRequest::ReadOnly|HitTestRequest::Active, but is it ok to initialize it
> with memset?

For the one use of HitTestResult in a vector (HitTestCache), it will be. The
alternative is to add a wrapper (heap) object indirection, which is somewhat
tedious.

[sof, 2015-06-13 17:00:03]

https://codereview.chromium.org/1185923002/diff/1/Source/core/layout/HitTestR...
File Source/core/layout/HitTestResult.h (right):

https://codereview.chromium.org/1185923002/diff/1/Source/core/layout/HitTestR...
Source/core/layout/HitTestResult.h:176:
WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS(blink::HitTestResult);
On 2015/06/13 12:30:31, sof wrote:
> On 2015/06/13 07:12:21, haraken wrote:
> > 
> > The default constructor of blink::HitTestResult is setting  m_hitTestRequest
> to
> > HitTestRequest::ReadOnly|HitTestRequest::Active, but is it ok to initialize
it
> > with memset?
> 
> For the one use of HitTestResult in a vector (HitTestCache), it will be. The
> alternative is to add a wrapper (heap) object indirection, which is somewhat
> tedious.

Is the HeapVector<> restriction accurate, or is it merely reusing
canInitializeWithMemset for convenience?

i.e., canInitializeWithMemset is used to determine if unused slots can be
cleared with memset(). So that we can guarantee that tracing those slots won't
run into trouble. Unused slots are otherwise not observable, so if they
represent valid objects or not, isn't relevant. Slots will otherwise be
initalize()d correctly.

[haraken, 2015-06-14 01:47:40]

On 2015/06/13 17:00:03, sof wrote:
>
https://codereview.chromium.org/1185923002/diff/1/Source/core/layout/HitTestR...
> File Source/core/layout/HitTestResult.h (right):
> 
>
https://codereview.chromium.org/1185923002/diff/1/Source/core/layout/HitTestR...
> Source/core/layout/HitTestResult.h:176:
> WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS(blink::HitTestResult);
> On 2015/06/13 12:30:31, sof wrote:
> > On 2015/06/13 07:12:21, haraken wrote:
> > > 
> > > The default constructor of blink::HitTestResult is setting 
m_hitTestRequest
> > to
> > > HitTestRequest::ReadOnly|HitTestRequest::Active, but is it ok to
initialize
> it
> > > with memset?
> > 
> > For the one use of HitTestResult in a vector (HitTestCache), it will be. The
> > alternative is to add a wrapper (heap) object indirection, which is somewhat
> > tedious.
> 
> Is the HeapVector<> restriction accurate, or is it merely reusing
> canInitializeWithMemset for convenience?
> 
> i.e., canInitializeWithMemset is used to determine if unused slots can be
> cleared with memset(). So that we can guarantee that tracing those slots won't
> run into trouble. Unused slots are otherwise not observable, so if they
> represent valid objects or not, isn't relevant. Slots will otherwise be
> initalize()d correctly.

Correct. Also it is used to determine if calling a destructor for a zeroed
memory doesn't cause an issue.

[sof, 2015-06-14 05:21:53]

On 2015/06/14 01:47:40, haraken wrote:
> On 2015/06/13 17:00:03, sof wrote:
> >
>
https://codereview.chromium.org/1185923002/diff/1/Source/core/layout/HitTestR...
> > File Source/core/layout/HitTestResult.h (right):
> > 
> >
>
https://codereview.chromium.org/1185923002/diff/1/Source/core/layout/HitTestR...
> > Source/core/layout/HitTestResult.h:176:
> > WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS(blink::HitTestResult);
> > On 2015/06/13 12:30:31, sof wrote:
> > > On 2015/06/13 07:12:21, haraken wrote:
> > > > 
> > > > The default constructor of blink::HitTestResult is setting 
> m_hitTestRequest
> > > to
> > > > HitTestRequest::ReadOnly|HitTestRequest::Active, but is it ok to
> initialize
> > it
> > > > with memset?
> > > 
> > > For the one use of HitTestResult in a vector (HitTestCache), it will be.
The
> > > alternative is to add a wrapper (heap) object indirection, which is
somewhat
> > > tedious.
> > 
> > Is the HeapVector<> restriction accurate, or is it merely reusing
> > canInitializeWithMemset for convenience?
> > 
> > i.e., canInitializeWithMemset is used to determine if unused slots can be
> > cleared with memset(). So that we can guarantee that tracing those slots
won't
> > run into trouble. Unused slots are otherwise not observable, so if they
> > represent valid objects or not, isn't relevant. Slots will otherwise be
> > initalize()d correctly.
> 
> Correct. Also it is used to determine if calling a destructor for a zeroed
> memory doesn't cause an issue.

Yes, that too. I suggest a trait for that is introduced and used by HeapVector
instead.

If agreed, I can follow up on that next. But to mend the tree's current broken
state, I've put in a TODO next to the current trait specialization. Landing that
now would be preferable, imho.

[sof, 2015-06-14 07:20:26]

On 2015/06/14 05:21:53, sof wrote:
> On 2015/06/14 01:47:40, haraken wrote:
> > On 2015/06/13 17:00:03, sof wrote:
> > >
> >
>
https://codereview.chromium.org/1185923002/diff/1/Source/core/layout/HitTestR...
> > > File Source/core/layout/HitTestResult.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1185923002/diff/1/Source/core/layout/HitTestR...
> > > Source/core/layout/HitTestResult.h:176:
> > > WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS(blink::HitTestResult);
> > > On 2015/06/13 12:30:31, sof wrote:
> > > > On 2015/06/13 07:12:21, haraken wrote:
> > > > > 
> > > > > The default constructor of blink::HitTestResult is setting 
> > m_hitTestRequest
> > > > to
> > > > > HitTestRequest::ReadOnly|HitTestRequest::Active, but is it ok to
> > initialize
> > > it
> > > > > with memset?
> > > > 
> > > > For the one use of HitTestResult in a vector (HitTestCache), it will be.
> The
> > > > alternative is to add a wrapper (heap) object indirection, which is
> somewhat
> > > > tedious.
> > > 
> > > Is the HeapVector<> restriction accurate, or is it merely reusing
> > > canInitializeWithMemset for convenience?
> > > 
> > > i.e., canInitializeWithMemset is used to determine if unused slots can be
> > > cleared with memset(). So that we can guarantee that tracing those slots
> won't
> > > run into trouble. Unused slots are otherwise not observable, so if they
> > > represent valid objects or not, isn't relevant. Slots will otherwise be
> > > initalize()d correctly.
> > 
> > Correct. Also it is used to determine if calling a destructor for a zeroed
> > memory doesn't cause an issue.
> 
> Yes, that too. I suggest a trait for that is introduced and used by HeapVector
> instead.
> 
> If agreed, I can follow up on that next. But to mend the tree's current broken
> state, I've put in a TODO next to the current trait specialization. Landing
that
> now would be preferable, imho.

https://codereview.chromium.org/1180383002/ would then address the followup.

[haraken, 2015-06-14 09:58:54]

On 2015/06/14 07:20:26, sof wrote:
> On 2015/06/14 05:21:53, sof wrote:
> > On 2015/06/14 01:47:40, haraken wrote:
> > > On 2015/06/13 17:00:03, sof wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1185923002/diff/1/Source/core/layout/HitTestR...
> > > > File Source/core/layout/HitTestResult.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1185923002/diff/1/Source/core/layout/HitTestR...
> > > > Source/core/layout/HitTestResult.h:176:
> > > > WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS(blink::HitTestResult);
> > > > On 2015/06/13 12:30:31, sof wrote:
> > > > > On 2015/06/13 07:12:21, haraken wrote:
> > > > > > 
> > > > > > The default constructor of blink::HitTestResult is setting 
> > > m_hitTestRequest
> > > > > to
> > > > > > HitTestRequest::ReadOnly|HitTestRequest::Active, but is it ok to
> > > initialize
> > > > it
> > > > > > with memset?
> > > > > 
> > > > > For the one use of HitTestResult in a vector (HitTestCache), it will
be.
> > The
> > > > > alternative is to add a wrapper (heap) object indirection, which is
> > somewhat
> > > > > tedious.
> > > > 
> > > > Is the HeapVector<> restriction accurate, or is it merely reusing
> > > > canInitializeWithMemset for convenience?
> > > > 
> > > > i.e., canInitializeWithMemset is used to determine if unused slots can
be
> > > > cleared with memset(). So that we can guarantee that tracing those slots
> > won't
> > > > run into trouble. Unused slots are otherwise not observable, so if they
> > > > represent valid objects or not, isn't relevant. Slots will otherwise be
> > > > initalize()d correctly.
> > > 
> > > Correct. Also it is used to determine if calling a destructor for a zeroed
> > > memory doesn't cause an issue.
> > 
> > Yes, that too. I suggest a trait for that is introduced and used by
HeapVector
> > instead.
> > 
> > If agreed, I can follow up on that next. But to mend the tree's current
broken
> > state, I've put in a TODO next to the current trait specialization. Landing
> that
> > now would be preferable, imho.
> 
> https://codereview.chromium.org/1180383002/ would then address the followup.

Thanks, LGTM.

[sof, 2015-06-14 10:37:30]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-06-14 10:37:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1185923002/20001

[commit-bot: I haz the power, 2015-06-14 10:40:53]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197095