Oilpan: Prepare moving DateTimeChooser to Oilpan

Oilpan: Prepare moving DateTimeChooser to Oilpan

BUG=

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

[zerny-chromium, 2014-06-02 13:32:41]

lgtm

https://codereview.chromium.org/275043002/diff/60001/Source/core/html/forms/B...
File Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.h (right):

https://codereview.chromium.org/275043002/diff/60001/Source/core/html/forms/B...
Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.h:42: virtual void
trace(Visitor*);
Nit: OVERRIDE

https://codereview.chromium.org/275043002/diff/60001/Source/web/DateTimeChoos...
File Source/web/DateTimeChooserImpl.h (right):

https://codereview.chromium.org/275043002/diff/60001/Source/web/DateTimeChoos...
Source/web/DateTimeChooserImpl.h:51: virtual void trace(WebCore::Visitor*)
OVERRIDE;
Nit: You can omit the trace method from this definition since it simply calls
the parent trace.

[haraken, 2014-06-02 14:34:13]

https://codereview.chromium.org/275043002/diff/60001/Source/core/html/forms/B...
File Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp (right):

https://codereview.chromium.org/275043002/diff/60001/Source/core/html/forms/B...
Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp:42:
closeDateTimeChooser();

This touches m_dataTimeChooser (which is an on-heap object) from the destructor.

I haven't looked at details, but in order to remove this line from oilpan
builds, do we probably need to move ChromeClientImpl and PagePopup to the heap
at the same time (because DateTimeChooserImpl::endChooser() touches
ChromeClientImpl and PagePopup)?

https://codereview.chromium.org/275043002/diff/60001/Source/core/html/shadow/...
File Source/core/html/shadow/PickerIndicatorElement.cpp (right):

https://codereview.chromium.org/275043002/diff/60001/Source/core/html/shadow/...
Source/core/html/shadow/PickerIndicatorElement.cpp:63: closePopup();

Ditto. We're not allowed to touch on-heap objects in destructors.

https://codereview.chromium.org/275043002/diff/60001/Source/core/loader/Empty...
File Source/core/loader/EmptyClients.cpp (right):

https://codereview.chromium.org/275043002/diff/60001/Source/core/loader/Empty...
Source/core/loader/EmptyClients.cpp:91: return
PassRefPtrWillBeRawPtr<DateTimeChooser>();

Doesn't 'return nullptr' work?

https://codereview.chromium.org/275043002/diff/60001/Source/web/DateTimeChoos...
File Source/web/DateTimeChooserImpl.h (right):

https://codereview.chromium.org/275043002/diff/60001/Source/web/DateTimeChoos...
Source/web/DateTimeChooserImpl.h:68: WebCore::DateTimeChooserClient* m_client;

Can we make DateTimeChooserClient a GC-mixin and make this a Member?

[keishi, 2014-06-05 03:46:18]

https://codereview.chromium.org/275043002/diff/60001/Source/core/html/forms/B...
File Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp (right):

https://codereview.chromium.org/275043002/diff/60001/Source/core/html/forms/B...
Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp:42:
closeDateTimeChooser();
On 2014/06/02 14:34:13, haraken wrote:
> 
> This touches m_dataTimeChooser (which is an on-heap object) from the
destructor.
> 
> I haven't looked at details, but in order to remove this line from oilpan
> builds, do we probably need to move ChromeClientImpl and PagePopup to the heap
> at the same time (because DateTimeChooserImpl::endChooser() touches
> ChromeClientImpl and PagePopup)?

closePopupView() should handle the closing before destruction so I removed this.

https://codereview.chromium.org/275043002/diff/60001/Source/core/html/forms/B...
File Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.h (right):

https://codereview.chromium.org/275043002/diff/60001/Source/core/html/forms/B...
Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.h:42: virtual void
trace(Visitor*);
On 2014/06/02 13:32:42, zerny-chromium wrote:
> Nit: OVERRIDE

Done.

https://codereview.chromium.org/275043002/diff/60001/Source/core/html/shadow/...
File Source/core/html/shadow/PickerIndicatorElement.cpp (right):

https://codereview.chromium.org/275043002/diff/60001/Source/core/html/shadow/...
Source/core/html/shadow/PickerIndicatorElement.cpp:63: closePopup();
On 2014/06/02 14:34:13, haraken wrote:
> 
> Ditto. We're not allowed to touch on-heap objects in destructors.

closing should be handled by PickerIndicatorElement::detach before destruction
so I removed this.

https://codereview.chromium.org/275043002/diff/60001/Source/core/loader/Empty...
File Source/core/loader/EmptyClients.cpp (right):

https://codereview.chromium.org/275043002/diff/60001/Source/core/loader/Empty...
Source/core/loader/EmptyClients.cpp:91: return
PassRefPtrWillBeRawPtr<DateTimeChooser>();
On 2014/06/02 14:34:13, haraken wrote:
> 
> Doesn't 'return nullptr' work?

Done.

https://codereview.chromium.org/275043002/diff/60001/Source/web/DateTimeChoos...
File Source/web/DateTimeChooserImpl.h (right):

https://codereview.chromium.org/275043002/diff/60001/Source/web/DateTimeChoos...
Source/web/DateTimeChooserImpl.h:68: WebCore::DateTimeChooserClient* m_client;
On 2014/06/02 14:34:13, haraken wrote:
> 
> Can we make DateTimeChooserClient a GC-mixin and make this a Member?

Done.

[tkent, 2014-06-05 04:27:55]

https://codereview.chromium.org/275043002/diff/80001/Source/platform/DateTime...
File Source/platform/DateTimeChooserClient.h (right):

https://codereview.chromium.org/275043002/diff/80001/Source/platform/DateTime...
Source/platform/DateTimeChooserClient.h:40:
DECLARE_EMPTY_VIRTUAL_DESTRUCTOR_WILL_BE_REMOVED(DateTimeChooserClient);
This needs corresponding DEFINE_EMPTY_VIRTUAL_DESTRUCTOR_WILL_BE_REMOVED.

[keishi, 2014-06-05 04:54:58]

https://codereview.chromium.org/275043002/diff/80001/Source/platform/DateTime...
File Source/platform/DateTimeChooserClient.h (right):

https://codereview.chromium.org/275043002/diff/80001/Source/platform/DateTime...
Source/platform/DateTimeChooserClient.h:40:
DECLARE_EMPTY_VIRTUAL_DESTRUCTOR_WILL_BE_REMOVED(DateTimeChooserClient);
On 2014/06/05 04:27:56, tkent wrote:
> This needs corresponding DEFINE_EMPTY_VIRTUAL_DESTRUCTOR_WILL_BE_REMOVED.

Done.

[haraken, 2014-06-05 04:59:25]

LGTM

https://codereview.chromium.org/275043002/diff/80001/Source/core/html/forms/B...
File Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp (left):

https://codereview.chromium.org/275043002/diff/80001/Source/core/html/forms/B...
Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp:42:
closeDateTimeChooser();

Shall we add:

#if !ENABLE(OILPAN)
  ASSERT(!m_dataTimeChooser);
#endif

? I'm not an expert of the forms code, so it's up to you.

https://codereview.chromium.org/275043002/diff/80001/Source/core/html/forms/B...
File Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.h (right):

https://codereview.chromium.org/275043002/diff/80001/Source/core/html/forms/B...
Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.h:40:
BaseChooserOnlyDateAndTimeInputType(HTMLInputElement& element) :
BaseDateAndTimeInputType(element) { }

Add explicit.

https://codereview.chromium.org/275043002/diff/80001/Source/core/html/shadow/...
File Source/core/html/shadow/PickerIndicatorElement.cpp (left):

https://codereview.chromium.org/275043002/diff/80001/Source/core/html/shadow/...
Source/core/html/shadow/PickerIndicatorElement.cpp:66: ASSERT(!m_chooser);

Shall we leave this ASSERT in non-oilpan builds? It's up to you.

https://codereview.chromium.org/275043002/diff/80001/Source/web/DateTimeChoos...
File Source/web/DateTimeChooserImpl.h (right):

https://codereview.chromium.org/275043002/diff/80001/Source/web/DateTimeChoos...
Source/web/DateTimeChooserImpl.h:67: ChromeClientImpl* m_chromeClient;

Just to confirm: This raw pointer is safe because DateTimeChooser shouldn't
outlive ChromeClient, right?

https://codereview.chromium.org/275043002/diff/80001/Source/web/DateTimeChoos...
Source/web/DateTimeChooserImpl.h:69: WebCore::PagePopup* m_popup;

This raw pointer looks safe since it's explicitly cleared when the popup is
closed.

[tkent, 2014-06-05 05:14:04]

lgtm

[keishi, 2014-06-05 08:06:38]

https://codereview.chromium.org/275043002/diff/80001/Source/core/html/forms/B...
File Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp (left):

https://codereview.chromium.org/275043002/diff/80001/Source/core/html/forms/B...
Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp:42:
closeDateTimeChooser();
On 2014/06/05 04:59:25, haraken wrote:
> 
> Shall we add:
> 
> #if !ENABLE(OILPAN)
>   ASSERT(!m_dataTimeChooser);
> #endif
> 
> ? I'm not an expert of the forms code, so it's up to you.

m_dataTimeChooser is only cleared in didEndChooser() which is called when the
"didEnd" ipc is delivered from the browser process. So this ASSERT won't work.

https://codereview.chromium.org/275043002/diff/80001/Source/core/html/forms/B...
File Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.h (right):

https://codereview.chromium.org/275043002/diff/80001/Source/core/html/forms/B...
Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.h:40:
BaseChooserOnlyDateAndTimeInputType(HTMLInputElement& element) :
BaseDateAndTimeInputType(element) { }
On 2014/06/05 04:59:25, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/275043002/diff/80001/Source/core/html/shadow/...
File Source/core/html/shadow/PickerIndicatorElement.cpp (left):

https://codereview.chromium.org/275043002/diff/80001/Source/core/html/shadow/...
Source/core/html/shadow/PickerIndicatorElement.cpp:66: ASSERT(!m_chooser);
On 2014/06/05 04:59:25, haraken wrote:
> 
> Shall we leave this ASSERT in non-oilpan builds? It's up to you.

This ASSERT is working because PickerIndicatorElement is only used on Desktop
clients which don't use ipc to open the date time chooser. However
DateTimeChooserClient does not guarantee this behavior so I am removing it.

https://codereview.chromium.org/275043002/diff/80001/Source/web/DateTimeChoos...
File Source/web/DateTimeChooserImpl.h (right):

https://codereview.chromium.org/275043002/diff/80001/Source/web/DateTimeChoos...
Source/web/DateTimeChooserImpl.h:67: ChromeClientImpl* m_chromeClient;
On 2014/06/05 04:59:25, haraken wrote:
> 
> Just to confirm: This raw pointer is safe because DateTimeChooser shouldn't
> outlive ChromeClient, right?

Yes.

[keishi, 2014-06-06 03:57:35]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-06-06 03:57:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/275043002/120001

[commit-bot: I haz the power, 2014-06-06 05:28:35]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10920)

[commit-bot: I haz the power, 2014-06-06 07:07:45]

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

[commit-bot: I haz the power, 2014-06-06 07:07:46]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[keishi, 2014-06-12 03:33:39]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-06-12 03:34:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/275043002/140001

[commit-bot: I haz the power, 2014-06-12 07:35:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[commit-bot: I haz the power, 2014-06-12 09:10:04]

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

[commit-bot: I haz the power, 2014-06-12 09:10:05]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[keishi, 2014-06-14 01:32:50]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-06-14 01:33:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/275043002/140001

[commit-bot: I haz the power, 2014-06-14 04:33:25]

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

[commit-bot: I haz the power, 2014-06-14 04:33:26]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[sof, 2014-06-14 06:18:01]

https://codereview.chromium.org/275043002/diff/140001/Source/platform/DateTim...
File Source/platform/DateTimeChooserClient.h (right):

https://codereview.chromium.org/275043002/diff/140001/Source/platform/DateTim...
Source/platform/DateTimeChooserClient.h:40: class PLATFORM_EXPORT
DateTimeChooserClient : public WillBeGarbageCollectedMixin {
This subtly breaks the non-Oilpan component build, as it will instantiate
WebCore::DummyBase<void> in blink_platform _and_ implicitly export the derived
template instantiation (at least MSVC does, see
http://msdn.microsoft.com/en-us/library/twa2aw10.aspx 's last bullet point.) The
linker will then complain when it links blink_web, as it also has that
instantiation.

The best solution I could come up with was to let that DummyBase instantiation
be exported from blink_platform & have everyone else import it, see
https://codereview.chromium.org/302093011/patch/80001/90008. There are some
relevant comments on that review as well, I think. That CL is not going ahead,
so feel free to re-purpose :)

[sof, 2014-06-16 10:49:21]

On 2014/06/14 06:18:01, sof wrote:
>
https://codereview.chromium.org/275043002/diff/140001/Source/platform/DateTim...
> File Source/platform/DateTimeChooserClient.h (right):
> 
>
https://codereview.chromium.org/275043002/diff/140001/Source/platform/DateTim...
> Source/platform/DateTimeChooserClient.h:40: class PLATFORM_EXPORT
> DateTimeChooserClient : public WillBeGarbageCollectedMixin {
> This subtly breaks the non-Oilpan component build, as it will instantiate
> WebCore::DummyBase<void> in blink_platform _and_ implicitly export the derived
> template instantiation (at least MSVC does, see
> http://msdn.microsoft.com/en-us/library/twa2aw10.aspx 's last bullet point.)
The
> linker will then complain when it links blink_web, as it also has that
> instantiation.
> 
> The best solution I could come up with was to let that DummyBase instantiation
> be exported from blink_platform & have everyone else import it, see
> https://codereview.chromium.org/302093011/patch/80001/90008. There are some
> relevant comments on that review as well, I think. That CL is not going ahead,
> so feel free to re-purpose :)

For completeness, the alternative is to engage in CPPery and define a
DATETIMECHOOSERCLIENT_BASE_CLASSES (or some such) that is either empty or ":
public WebCore::GarbageCollectedMixin".

I prefer exposing this dummy class as a DLL export.

[keishi, 2014-06-17 04:37:51]

On 2014/06/16 10:49:21, sof wrote:
> On 2014/06/14 06:18:01, sof wrote:
> >
>
https://codereview.chromium.org/275043002/diff/140001/Source/platform/DateTim...
> > File Source/platform/DateTimeChooserClient.h (right):
> > 
> >
>
https://codereview.chromium.org/275043002/diff/140001/Source/platform/DateTim...
> > Source/platform/DateTimeChooserClient.h:40: class PLATFORM_EXPORT
> > DateTimeChooserClient : public WillBeGarbageCollectedMixin {
> > This subtly breaks the non-Oilpan component build, as it will instantiate
> > WebCore::DummyBase<void> in blink_platform _and_ implicitly export the
derived
> > template instantiation (at least MSVC does, see
> > http://msdn.microsoft.com/en-us/library/twa2aw10.aspx 's last bullet point.)
> The
> > linker will then complain when it links blink_web, as it also has that
> > instantiation.
> > 
> > The best solution I could come up with was to let that DummyBase
instantiation
> > be exported from blink_platform & have everyone else import it, see
> > https://codereview.chromium.org/302093011/patch/80001/90008. There are some
> > relevant comments on that review as well, I think. That CL is not going
ahead,
> > so feel free to re-purpose :)
> 
> For completeness, the alternative is to engage in CPPery and define a
> DATETIMECHOOSERCLIENT_BASE_CLASSES (or some such) that is either empty or ":
> public WebCore::GarbageCollectedMixin".
> 
> I prefer exposing this dummy class as a DLL export.

Thanks! I would have never figured it out myself.

[sof, 2014-06-17 06:23:56]

On 2014/06/17 04:37:51, keishi wrote:
> On 2014/06/16 10:49:21, sof wrote:
> > On 2014/06/14 06:18:01, sof wrote:
> > >
> >
>
https://codereview.chromium.org/275043002/diff/140001/Source/platform/DateTim...
> > > File Source/platform/DateTimeChooserClient.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/275043002/diff/140001/Source/platform/DateTim...
> > > Source/platform/DateTimeChooserClient.h:40: class PLATFORM_EXPORT
> > > DateTimeChooserClient : public WillBeGarbageCollectedMixin {
> > > This subtly breaks the non-Oilpan component build, as it will instantiate
> > > WebCore::DummyBase<void> in blink_platform _and_ implicitly export the
> derived
> > > template instantiation (at least MSVC does, see
> > > http://msdn.microsoft.com/en-us/library/twa2aw10.aspx 's last bullet
point.)
> > The
> > > linker will then complain when it links blink_web, as it also has that
> > > instantiation.
> > > 
> > > The best solution I could come up with was to let that DummyBase
> instantiation
> > > be exported from blink_platform & have everyone else import it, see
> > > https://codereview.chromium.org/302093011/patch/80001/90008. There are
some
> > > relevant comments on that review as well, I think. That CL is not going
> ahead,
> > > so feel free to re-purpose :)
> > 
> > For completeness, the alternative is to engage in CPPery and define a
> > DATETIMECHOOSERCLIENT_BASE_CLASSES (or some such) that is either empty or ":
> > public WebCore::GarbageCollectedMixin".
> > 
> > I prefer exposing this dummy class as a DLL export.
> 
> Thanks! I would have never figured it out myself.

looks ok to me, but someone else should look it over and approve.

[Mads Ager (chromium), 2014-06-17 06:32:27]

On 2014/06/17 06:23:56, sof wrote:
> On 2014/06/17 04:37:51, keishi wrote:
> > On 2014/06/16 10:49:21, sof wrote:
> > > On 2014/06/14 06:18:01, sof wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/275043002/diff/140001/Source/platform/DateTim...
> > > > File Source/platform/DateTimeChooserClient.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/275043002/diff/140001/Source/platform/DateTim...
> > > > Source/platform/DateTimeChooserClient.h:40: class PLATFORM_EXPORT
> > > > DateTimeChooserClient : public WillBeGarbageCollectedMixin {
> > > > This subtly breaks the non-Oilpan component build, as it will
instantiate
> > > > WebCore::DummyBase<void> in blink_platform _and_ implicitly export the
> > derived
> > > > template instantiation (at least MSVC does, see
> > > > http://msdn.microsoft.com/en-us/library/twa2aw10.aspx 's last bullet
> point.)
> > > The
> > > > linker will then complain when it links blink_web, as it also has that
> > > > instantiation.
> > > > 
> > > > The best solution I could come up with was to let that DummyBase
> > instantiation
> > > > be exported from blink_platform & have everyone else import it, see
> > > > https://codereview.chromium.org/302093011/patch/80001/90008. There are
> some
> > > > relevant comments on that review as well, I think. That CL is not going
> > ahead,
> > > > so feel free to re-purpose :)
> > > 
> > > For completeness, the alternative is to engage in CPPery and define a
> > > DATETIMECHOOSERCLIENT_BASE_CLASSES (or some such) that is either empty or
":
> > > public WebCore::GarbageCollectedMixin".
> > > 
> > > I prefer exposing this dummy class as a DLL export.
> > 
> > Thanks! I would have never figured it out myself.
> 
> looks ok to me, but someone else should look it over and approve.

Platform export change LGTM.

[haraken, 2014-06-17 06:33:48]

LGTM

[keishi, 2014-06-17 06:44:24]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-06-17 06:44:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/275043002/160001

[commit-bot: I haz the power, 2014-06-17 07:59:02]

Change committed as 176293