Oilpan: move BaseChooserOnlyDateAndTimeInputType to the heap.

Oilpan: move BaseChooserOnlyDateAndTimeInputType to the heap.

Move this !ENABLE(INPUT_MULTIPLE_FIELDS_UI) object to the Oilpan heap,
where it belongs as it derives from the on-heap BaseDateAndTimeInputType
class.

At the same time, turn DateTimeChooserClient into a GC mixin.

R=haraken
BUG=553613
Committed: https://crrev.com/eaa56600a21f66e4e441aca32c5fa3c32d139643
Cr-Commit-Position: refs/heads/master@{#361071}

[sof, 2015-11-21 16:28:46]

Description was changed from

==========
WIP: Test oilpan compilation on Android targets.

R=
BUG=
==========

to

==========
Oilpan: move BaseChooserOnlyDateAndTimeInputType to the heap.

Move this !ENABLE(INPUT_MULTIPLE_FIELDS_UI) object to the Oilpan heap,
where it belongs as it derives from the on-heap BaseDateAndTimeInputType
class. 

At the same time, turn DateTimeChooserClient into a GC mixin.

R=
BUG=553613
==========

[sof, 2015-11-21 16:29:05]

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

[sof, 2015-11-21 16:29:06]

please take a look.

[haraken, 2015-11-21 16:46:39]

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

[haraken, 2015-11-21 16:46:39]

https://codereview.chromium.org/1466113002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp
(right):

https://codereview.chromium.org/1466113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp:43:
closeDateTimeChooser();

This touches m_dateTimeChooser...

[sof, 2015-11-21 16:59:44]

https://codereview.chromium.org/1466113002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp
(right):

https://codereview.chromium.org/1466113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp:43:
closeDateTimeChooser();
On 2015/11/21 16:46:39, haraken wrote:
> 
> This touches m_dateTimeChooser...

Thanks, thought I'd checked every destructor here. Added a prefinalizer here
also.

[haraken, 2015-11-21 17:10:23]

https://codereview.chromium.org/1466113002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp
(right):

https://codereview.chromium.org/1466113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp:53:
#endif

Maybe worth adding ASSERT(!m_datatimeChooser).

https://codereview.chromium.org/1466113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/MonthInputType.h (right):

https://codereview.chromium.org/1466113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/MonthInputType.h:46:
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(MonthInputType);

I'm a bit confused. Why does this need to use USING_GC_MIXIN? Both
BaseMultipleFieldsDateAndTimeInputType and BaseChooserOnlyDateAndTimeInputType
inherits from GarbageCollected and no mixin class is inherited by
MonthInputType...

(I think I'm missing something.)

[sof, 2015-11-21 17:26:20]

https://codereview.chromium.org/1466113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/MonthInputType.h (right):

https://codereview.chromium.org/1466113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/MonthInputType.h:46:
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(MonthInputType);
On 2015/11/21 17:10:22, haraken wrote:
> 
> I'm a bit confused. Why does this need to use USING_GC_MIXIN? Both
> BaseMultipleFieldsDateAndTimeInputType and BaseChooserOnlyDateAndTimeInputType
> inherits from GarbageCollected and no mixin class is inherited by
> MonthInputType...
> 
> (I think I'm missing something.)

It only needs it !ENABLE(INPUT_MULTIPLE_FIELDS_UI) for DateTimeChooserClient.

[sof, 2015-11-21 17:41:52]

https://codereview.chromium.org/1466113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/MonthInputType.h (right):

https://codereview.chromium.org/1466113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/MonthInputType.h:46:
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(MonthInputType);
On 2015/11/21 17:26:20, sof wrote:
> On 2015/11/21 17:10:22, haraken wrote:
> > 
> > I'm a bit confused. Why does this need to use USING_GC_MIXIN? Both
> > BaseMultipleFieldsDateAndTimeInputType and
BaseChooserOnlyDateAndTimeInputType
> > inherits from GarbageCollected and no mixin class is inherited by
> > MonthInputType...
> > 
> > (I think I'm missing something.)
> 
> It only needs it !ENABLE(INPUT_MULTIPLE_FIELDS_UI) for DateTimeChooserClient.

I'll make that more precise, at the cost of some #if-ery.

[haraken, 2015-11-22 01:01:39]

https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp
(right):

https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp:53:
#endif

ASSERT(!m_dateTimeChooser).

https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/DateInputType.h (right):

https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/DateInputType.h:46:
WILL_BE_USING_DATETIMERCHOOSERCLIENT_GC_MIXIN(DateInputType);

Help me understand: Why can't we make BaseChooserOnlyDateAndTimeInputType a GC
mixin instead of making DateInputType a GC mixin? It seems more straightforward
since the DateTimeChooserClient is inherited by
BaseChooserOnlyDateAndTimeInputType.

[sof, 2015-11-22 07:48:55]

https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/DateInputType.h (right):

https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/DateInputType.h:46:
WILL_BE_USING_DATETIMERCHOOSERCLIENT_GC_MIXIN(DateInputType);
On 2015/11/22 01:01:39, haraken wrote:
> 
> Help me understand: Why can't we make BaseChooserOnlyDateAndTimeInputType a GC
> mixin instead of making DateInputType a GC mixin? It seems more
straightforward
> since the DateTimeChooserClient is inherited by
> BaseChooserOnlyDateAndTimeInputType.

Isn't it the rule that the leftmost base should define itself as a GC mixin?

[sof, 2015-11-22 08:39:06]

https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp
(right):

https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp:53:
#endif
On 2015/11/22 01:01:39, haraken wrote:
> 
> ASSERT(!m_dateTimeChooser).

Done.

[haraken, 2015-11-22 14:27:09]

https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/DateInputType.h (right):

https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/DateInputType.h:46:
WILL_BE_USING_DATETIMERCHOOSERCLIENT_GC_MIXIN(DateInputType);
On 2015/11/22 07:48:55, sof wrote:
> On 2015/11/22 01:01:39, haraken wrote:
> > 
> > Help me understand: Why can't we make BaseChooserOnlyDateAndTimeInputType a
GC
> > mixin instead of making DateInputType a GC mixin? It seems more
> straightforward
> > since the DateTimeChooserClient is inherited by
> > BaseChooserOnlyDateAndTimeInputType.
> 
> Isn't it the rule that the leftmost base should define itself as a GC mixin?

I think the rule is as follows.

  class A : public GarbageCollectedMixin { };
  class B : public GarbageCollected<B> { };
  class C : public B, public A { };
  class D : public C { };

We need the USING_GARBAGE_COLLECTED_MIXIN for C but don't need it for D. Am I
wrong?

[sof, 2015-11-22 14:34:37]

On 2015/11/22 14:27:09, haraken wrote:
>
https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/forms/DateInputType.h (right):
> 
>
https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/forms/DateInputType.h:46:
> WILL_BE_USING_DATETIMERCHOOSERCLIENT_GC_MIXIN(DateInputType);
> On 2015/11/22 07:48:55, sof wrote:
> > On 2015/11/22 01:01:39, haraken wrote:
> > > 
> > > Help me understand: Why can't we make BaseChooserOnlyDateAndTimeInputType
a
> GC
> > > mixin instead of making DateInputType a GC mixin? It seems more
> > straightforward
> > > since the DateTimeChooserClient is inherited by
> > > BaseChooserOnlyDateAndTimeInputType.
> > 
> > Isn't it the rule that the leftmost base should define itself as a GC mixin?
> 
> I think the rule is as follows.
> 
>   class A : public GarbageCollectedMixin { };
>   class B : public GarbageCollected<B> { };
>   class C : public B, public A { };
>   class D : public C { };
> 
> We need the USING_GARBAGE_COLLECTED_MIXIN for C but don't need it for D. Am I
> wrong?

Assuming the classes here actually define methods (including virtuals), I
believe you are wrong -- we have to get to the head of the object allocation via
these mixin overrides (e.g., so as to have isHeapObjectAlive() work as expected
over the object header.)

If you position it here for C, I don't think that holds.

[haraken, 2015-11-22 15:07:17]

On 2015/11/22 14:34:37, sof wrote:
> On 2015/11/22 14:27:09, haraken wrote:
> >
>
https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/html/forms/DateInputType.h (right):
> > 
> >
>
https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/html/forms/DateInputType.h:46:
> > WILL_BE_USING_DATETIMERCHOOSERCLIENT_GC_MIXIN(DateInputType);
> > On 2015/11/22 07:48:55, sof wrote:
> > > On 2015/11/22 01:01:39, haraken wrote:
> > > > 
> > > > Help me understand: Why can't we make
BaseChooserOnlyDateAndTimeInputType
> a
> > GC
> > > > mixin instead of making DateInputType a GC mixin? It seems more
> > > straightforward
> > > > since the DateTimeChooserClient is inherited by
> > > > BaseChooserOnlyDateAndTimeInputType.
> > > 
> > > Isn't it the rule that the leftmost base should define itself as a GC
mixin?
> > 
> > I think the rule is as follows.
> > 
> >   class A : public GarbageCollectedMixin { };
> >   class B : public GarbageCollected<B> { };
> >   class C : public B, public A { };
> >   class D : public C { };
> > 
> > We need the USING_GARBAGE_COLLECTED_MIXIN for C but don't need it for D. Am
I
> > wrong?
> 
> Assuming the classes here actually define methods (including virtuals), I
> believe you are wrong -- we have to get to the head of the object allocation
via
> these mixin overrides (e.g., so as to have isHeapObjectAlive() work as
expected
> over the object header.)
> 
> If you position it here for C, I don't think that holds.

Just to confirm: A problem happens when you instantiate an object o type D and
create a Member<A> pointing to the object. If we don't have
USING_GARBAGE_COLLECTED_MIXIN on D, D::trace is not called. As a result, we miss
tracing D's members. This is the problem you're saying, right?

Yeah, I think you're right... I wasn't aware of the problem and unfortunately we
have a bunch of classes that should have USING_GARBAGE_COLLECTED_MIXIN but
don't. For example, HTMLVideoElement and HTMLAudioElement don't have
USING_GARBAGE_COLLECTED_MIXIN.

[sof, 2015-11-22 21:14:45]

On 2015/11/22 15:07:17, haraken wrote:
> On 2015/11/22 14:34:37, sof wrote:
> > On 2015/11/22 14:27:09, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/html/forms/DateInputType.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/html/forms/DateInputType.h:46:
> > > WILL_BE_USING_DATETIMERCHOOSERCLIENT_GC_MIXIN(DateInputType);
> > > On 2015/11/22 07:48:55, sof wrote:
> > > > On 2015/11/22 01:01:39, haraken wrote:
> > > > > 
> > > > > Help me understand: Why can't we make
> BaseChooserOnlyDateAndTimeInputType
> > a
> > > GC
> > > > > mixin instead of making DateInputType a GC mixin? It seems more
> > > > straightforward
> > > > > since the DateTimeChooserClient is inherited by
> > > > > BaseChooserOnlyDateAndTimeInputType.
> > > > 
> > > > Isn't it the rule that the leftmost base should define itself as a GC
> mixin?
> > > 
> > > I think the rule is as follows.
> > > 
> > >   class A : public GarbageCollectedMixin { };
> > >   class B : public GarbageCollected<B> { };
> > >   class C : public B, public A { };
> > >   class D : public C { };
> > > 
> > > We need the USING_GARBAGE_COLLECTED_MIXIN for C but don't need it for D.
Am
> I
> > > wrong?
> > 
> > Assuming the classes here actually define methods (including virtuals), I
> > believe you are wrong -- we have to get to the head of the object allocation
> via
> > these mixin overrides (e.g., so as to have isHeapObjectAlive() work as
> expected
> > over the object header.)
> > 
> > If you position it here for C, I don't think that holds.
> 
> Just to confirm: A problem happens when you instantiate an object o type D and
> create a Member<A> pointing to the object. If we don't have
> USING_GARBAGE_COLLECTED_MIXIN on D, D::trace is not called. As a result, we
miss
> tracing D's members. This is the problem you're saying, right?
> 

That's what I thought, but as trace() (and isHeapObjectAlive()) are virtual
methods for GCMixins, this will really work as expected/wanted. I went over the
implementation of GarbageCollectedMixin in some detail (yet again), and virtual
method dispatch does correctly allow what you suggested (C).

If you've got a Member<A> holding an instance of D, then the vptr of that
reference will point to a vtbl having D::trace, not C's, so the D::trace will be
invoked when tracing Member<A>. Which will invoke C::trace() in turn,
presumably.

As far being able to get to the heap object header from a mixin when e.g.
marking Member<A>, an adjustAndMark() virtual method needs to be defined -- it
doesn't have to be defined on the leftmost class (D) either. The object pointer
that adjustAndMark() is invoked over will be whatever instance implementing the
GC mixin -- D if D::create() was used to instantiate it, C if C::create(). Which
will be at the head of the object, meaning that the object header is reachable
from it.

> Yeah, I think you're right... I wasn't aware of the problem and unfortunately
we
> have a bunch of classes that should have USING_GARBAGE_COLLECTED_MIXIN but
> don't. For example, HTMLVideoElement and HTMLAudioElement don't have
> USING_GARBAGE_COLLECTED_MIXIN.

Apologies for getting you worried, it is fine to declare
USING_GARBAGE_COLLECTED_MIXIN() where the GC base class "meets" the GC mixins (C
in the above) -- USING_GARBAGE_COLLECTED_MIXIN() doesn't have to be pushed out
to the leftmost class (D). Virtual method dispatch makes it work out in either
position.

(I'll separately followup with a unit test for this, as I don't see one.)

With all of the above clarified (which is a good thing), I've dropped back to
just using USING_GARBAGE_COLLECTED_MIXIN() on
BaseChooserOnlyDateAndTimeInputType

[haraken, 2015-11-23 02:39:20]

On 2015/11/22 21:14:45, sof wrote:
> On 2015/11/22 15:07:17, haraken wrote:
> > On 2015/11/22 14:34:37, sof wrote:
> > > On 2015/11/22 14:27:09, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/core/html/forms/DateInputType.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1466113002/diff/60001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/core/html/forms/DateInputType.h:46:
> > > > WILL_BE_USING_DATETIMERCHOOSERCLIENT_GC_MIXIN(DateInputType);
> > > > On 2015/11/22 07:48:55, sof wrote:
> > > > > On 2015/11/22 01:01:39, haraken wrote:
> > > > > > 
> > > > > > Help me understand: Why can't we make
> > BaseChooserOnlyDateAndTimeInputType
> > > a
> > > > GC
> > > > > > mixin instead of making DateInputType a GC mixin? It seems more
> > > > > straightforward
> > > > > > since the DateTimeChooserClient is inherited by
> > > > > > BaseChooserOnlyDateAndTimeInputType.
> > > > > 
> > > > > Isn't it the rule that the leftmost base should define itself as a GC
> > mixin?
> > > > 
> > > > I think the rule is as follows.
> > > > 
> > > >   class A : public GarbageCollectedMixin { };
> > > >   class B : public GarbageCollected<B> { };
> > > >   class C : public B, public A { };
> > > >   class D : public C { };
> > > > 
> > > > We need the USING_GARBAGE_COLLECTED_MIXIN for C but don't need it for D.
> Am
> > I
> > > > wrong?
> > > 
> > > Assuming the classes here actually define methods (including virtuals), I
> > > believe you are wrong -- we have to get to the head of the object
allocation
> > via
> > > these mixin overrides (e.g., so as to have isHeapObjectAlive() work as
> > expected
> > > over the object header.)
> > > 
> > > If you position it here for C, I don't think that holds.
> > 
> > Just to confirm: A problem happens when you instantiate an object o type D
and
> > create a Member<A> pointing to the object. If we don't have
> > USING_GARBAGE_COLLECTED_MIXIN on D, D::trace is not called. As a result, we
> miss
> > tracing D's members. This is the problem you're saying, right?
> > 
> 
> That's what I thought, but as trace() (and isHeapObjectAlive()) are virtual
> methods for GCMixins, this will really work as expected/wanted. I went over
the
> implementation of GarbageCollectedMixin in some detail (yet again), and
virtual
> method dispatch does correctly allow what you suggested (C).
> 
> If you've got a Member<A> holding an instance of D, then the vptr of that
> reference will point to a vtbl having D::trace, not C's, so the D::trace will
be
> invoked when tracing Member<A>. Which will invoke C::trace() in turn,
> presumably.
> 
> As far being able to get to the heap object header from a mixin when e.g.
> marking Member<A>, an adjustAndMark() virtual method needs to be defined -- it
> doesn't have to be defined on the leftmost class (D) either. The object
pointer
> that adjustAndMark() is invoked over will be whatever instance implementing
the
> GC mixin -- D if D::create() was used to instantiate it, C if C::create().
Which
> will be at the head of the object, meaning that the object header is reachable
> from it.
> 
> > Yeah, I think you're right... I wasn't aware of the problem and
unfortunately
> we
> > have a bunch of classes that should have USING_GARBAGE_COLLECTED_MIXIN but
> > don't. For example, HTMLVideoElement and HTMLAudioElement don't have
> > USING_GARBAGE_COLLECTED_MIXIN.
> 
> Apologies for getting you worried, it is fine to declare
> USING_GARBAGE_COLLECTED_MIXIN() where the GC base class "meets" the GC mixins
(C
> in the above) -- USING_GARBAGE_COLLECTED_MIXIN() doesn't have to be pushed out
> to the leftmost class (D). Virtual method dispatch makes it work out in either
> position.
> 
> (I'll separately followup with a unit test for this, as I don't see one.)
> 
> With all of the above clarified (which is a good thing), I've dropped back to
> just using USING_GARBAGE_COLLECTED_MIXIN() on
> BaseChooserOnlyDateAndTimeInputType

Thanks for the clarification! LGTM.

[sof, 2015-11-23 06:20:36]

Description was changed from

==========
Oilpan: move BaseChooserOnlyDateAndTimeInputType to the heap.

Move this !ENABLE(INPUT_MULTIPLE_FIELDS_UI) object to the Oilpan heap,
where it belongs as it derives from the on-heap BaseDateAndTimeInputType
class. 

At the same time, turn DateTimeChooserClient into a GC mixin.

R=
BUG=553613
==========

to

==========
Oilpan: move BaseChooserOnlyDateAndTimeInputType to the heap.

Move this !ENABLE(INPUT_MULTIPLE_FIELDS_UI) object to the Oilpan heap,
where it belongs as it derives from the on-heap BaseDateAndTimeInputType
class. 

At the same time, turn DateTimeChooserClient into a GC mixin.

R=haraken
BUG=553613
==========

[sof, 2015-11-23 06:42:18]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-11-23 06:42:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1466113002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1466113002/100001

[commit-bot: I haz the power, 2015-11-23 06:47:19]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-23 06:48:12]

Patchset 6 (id:??) landed as
https://crrev.com/eaa56600a21f66e4e441aca32c5fa3c32d139643
Cr-Commit-Position: refs/heads/master@{#361071}