Move color chooser objects to the Oilpan heap.

Move color chooser objects to the Oilpan heap.

Keep these on the heap so that their raw pointers to heap objects can be
correctly traced.

R=mkwst,haraken,tkent,keishi
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183538

[sof, 2014-10-06 17:46:20]

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

[sof, 2014-10-06 17:46:21]

Please take a look.

Removing yet another LocalFrame raw pointer reference (and a few other ones
besides.)

[Mike West, 2014-10-06 18:45:31]

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

[Mike West, 2014-10-06 18:45:32]

https://codereview.chromium.org/631833002/diff/1/Source/core/html/forms/Color...
File Source/core/html/forms/ColorInputType.cpp (left):

https://codereview.chromium.org/631833002/diff/1/Source/core/html/forms/Color...
Source/core/html/forms/ColorInputType.cpp:85: endColorChooser();
Why is it safe to drop this call?

https://codereview.chromium.org/631833002/diff/1/Source/web/ChromeClientImpl.cpp
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/631833002/diff/1/Source/web/ChromeClientImpl....
Source/web/ChromeClientImpl.cpp:584:
OwnPtrWillBeRawPtr<ColorChooserUIController> controller = nullptr;
Do you have to set this explicitly? Does the default constructor not suffice?

Actually, looking at the code below, a ternary if might avoid the question
entirely (e.g. `controller = RuntimeEnabledFeatures::pagePopupEnabled() ? ... :
...`)

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
File Source/web/ColorChooserPopupUIController.h (right):

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
Source/web/ColorChooserPopupUIController.h:65: virtual void didClosePopup()
override;
C++11!1!!!1!

[sof, 2014-10-06 19:19:29]

https://codereview.chromium.org/631833002/diff/1/Source/core/html/forms/Color...
File Source/core/html/forms/ColorInputType.cpp (left):

https://codereview.chromium.org/631833002/diff/1/Source/core/html/forms/Color...
Source/core/html/forms/ColorInputType.cpp:85: endColorChooser();
On 2014/10/06 18:45:32, Mike West wrote:
> Why is it safe to drop this call?

Sorry, should have made that clear with a review comment.

The call hasn't been dropped but pushed down to the ColorChooser finalizers. As
ColorChooser objects are also on the heap, that's where this call have to be
with Oilpan(*)..and it makes sense to (I think) to have the ColorChooser take
care of their own shutdown rather than have its owning object do it.

* Oilpan finalizers cannot touch other heap objects, as we have no finalization
ordering.

https://codereview.chromium.org/631833002/diff/1/Source/web/ChromeClientImpl.cpp
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/631833002/diff/1/Source/web/ChromeClientImpl....
Source/web/ChromeClientImpl.cpp:584:
OwnPtrWillBeRawPtr<ColorChooserUIController> controller = nullptr;
On 2014/10/06 18:45:32, Mike West wrote:
> Do you have to set this explicitly? Does the default constructor not suffice?
> 

Yes, RawPtr<> isn't zero initialized. This is the style we consistently follow,
irrespective of how the reference is initialized in the code that follows.

> Actually, looking at the code below, a ternary if might avoid the question
> entirely (e.g. `controller = RuntimeEnabledFeatures::pagePopupEnabled() ? ...
:
> ...`)

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
File Source/web/ColorChooserPopupUIController.h (right):

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
Source/web/ColorChooserPopupUIController.h:65: virtual void didClosePopup()
override;
On 2014/10/06 18:45:32, Mike West wrote:
> C++11!1!!!1!

I know there are renaming CLs coming down the chute, so not sure I ought to be
doing this (and instead rely on them to mop up.)

But it looked weird to add a use of 'override' for trace() and leave the rest
with the old school OVERRIDE.

[Mike West, 2014-10-06 19:29:29]

core/ LGTM.

https://codereview.chromium.org/631833002/diff/1/Source/core/html/forms/Color...
File Source/core/html/forms/ColorInputType.cpp (left):

https://codereview.chromium.org/631833002/diff/1/Source/core/html/forms/Color...
Source/core/html/forms/ColorInputType.cpp:85: endColorChooser();
On 2014/10/06 19:19:29, sof wrote:
> On 2014/10/06 18:45:32, Mike West wrote:
> > Why is it safe to drop this call?
> 
> Sorry, should have made that clear with a review comment.
> 
> The call hasn't been dropped but pushed down to the ColorChooser finalizers.
As
> ColorChooser objects are also on the heap, that's where this call have to be
> with Oilpan(*)..and it makes sense to (I think) to have the ColorChooser take
> care of their own shutdown rather than have its owning object do it.
> 
> * Oilpan finalizers cannot touch other heap objects, as we have no
finalization
> ordering.

Makes sense, thank you.

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
File Source/web/ColorChooserPopupUIController.h (right):

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
Source/web/ColorChooserPopupUIController.h:65: virtual void didClosePopup()
override;
On 2014/10/06 19:19:29, sof wrote:
> On 2014/10/06 18:45:32, Mike West wrote:
> > C++11!1!!!1!
> 
> I know there are renaming CLs coming down the chute, so not sure I ought to be
> doing this (and instead rely on them to mop up.)
> 
> But it looked weird to add a use of 'override' for trace() and leave the rest
> with the old school OVERRIDE.

I agree. You should feel free to clean up as you go. Let the big, mechanical CL
writers deal with the big mechanical rebases. :)

[Mike West, 2014-10-06 19:29:53]

(web/ LGTM too, but you'll need an OWNER to sign off on it.)

[tkent, 2014-10-07 02:24:59]

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

[tkent, 2014-10-07 02:25:00]

lgtm

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
File Source/web/ColorChooserPopupUIController.cpp (right):

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
Source/web/ColorChooserPopupUIController.cpp:58: endChooser();
endChooser() touches m_chooser and m_popup, and both of them are off-heap.  This
is safe.

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
File Source/web/ColorChooserPopupUIController.h (right):

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
Source/web/ColorChooserPopupUIController.h:65: virtual void didClosePopup()
override;
FYI.  Our new rule is to remove |virtual| if a function has |override| or
|final|.  So, even if you replace all of |OVERRIDE| with |override| in this CL,
we need to update all of lines again to remove |virtual|.

[haraken, 2014-10-07 03:31:28]

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

[haraken, 2014-10-07 03:31:30]

LGTM

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
File Source/web/ColorChooserPopupUIController.cpp (right):

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
Source/web/ColorChooserPopupUIController.cpp:56:
ColorChooserPopupUIController::~ColorChooserPopupUIController()

Can we add ASSERT(!m_popup) ?

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
Source/web/ColorChooserPopupUIController.cpp:58: endChooser();

Both ~ColorChooserPopupUIController and ~ColorChooserUIController call
endChooser(), which is not that nice.

Can we rewrite the code like this?

virtual ~ColorChooserUIController() {
  endChooser();
}

virtual ~ColorChooserPopupUIController {
  if (m_popup)
    closePopup();
}

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
File Source/web/ColorChooserPopupUIController.h (right):

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
Source/web/ColorChooserPopupUIController.h:74: PagePopup* m_popup;

This raw pointer looks safe since it's cleared in didClosePopup().

[sof, 2014-10-07 06:35:20]

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
File Source/web/ColorChooserPopupUIController.cpp (right):

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
Source/web/ColorChooserPopupUIController.cpp:56:
ColorChooserPopupUIController::~ColorChooserPopupUIController()
On 2014/10/07 03:31:29, haraken wrote:
> 
> Can we add ASSERT(!m_popup) ?

I'm quite likely missing some invariant, but why can we assume so?

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
Source/web/ColorChooserPopupUIController.cpp:58: endChooser();
On 2014/10/07 03:31:29, haraken wrote:
> 
> Both ~ColorChooserPopupUIController and ~ColorChooserUIController call
> endChooser(), which is not that nice.
> 
> Can we rewrite the code like this?
> 
> virtual ~ColorChooserUIController() {
>   endChooser();
> }
> 
> virtual ~ColorChooserPopupUIController {
>   if (m_popup)
>     closePopup();
> }

That changes the order for the popup controller, closing the popup page
earlier/first.

endChooser() isn't invoked twice, somewhat stodgily achieved perhaps, but I
prefer to keep it the way it is now/here.

[haraken, 2014-10-07 06:43:59]

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
File Source/web/ColorChooserPopupUIController.cpp (right):

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
Source/web/ColorChooserPopupUIController.cpp:56:
ColorChooserPopupUIController::~ColorChooserPopupUIController()
On 2014/10/07 06:35:19, sof wrote:
> On 2014/10/07 03:31:29, haraken wrote:
> > 
> > Can we add ASSERT(!m_popup) ?
> 
> I'm quite likely missing some invariant, but why can we assume so?

Sorry, I was misreading the code. We cannot assume that.

https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
Source/web/ColorChooserPopupUIController.cpp:58: endChooser();
On 2014/10/07 06:35:19, sof wrote:
> On 2014/10/07 03:31:29, haraken wrote:
> > 
> > Both ~ColorChooserPopupUIController and ~ColorChooserUIController call
> > endChooser(), which is not that nice.
> > 
> > Can we rewrite the code like this?
> > 
> > virtual ~ColorChooserUIController() {
> >   endChooser();
> > }
> > 
> > virtual ~ColorChooserPopupUIController {
> >   if (m_popup)
> >     closePopup();
> > }
> 
> That changes the order for the popup controller, closing the popup page
> earlier/first.
> 
> endChooser() isn't invoked twice, somewhat stodgily achieved perhaps, but I
> prefer to keep it the way it is now/here.

Does the order matter?

Not related to this CL, it looks not nice to call endChooser() in both 
~ColorChooserPopupUIController and ~ColorChooserUIController. If the order
doesn't matter, it looks better to remove endChooser() in
~ColorChooserPopupUIController.

[sof, 2014-10-07 06:59:56]

On 2014/10/07 06:43:59, haraken wrote:
>
https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
> File Source/web/ColorChooserPopupUIController.cpp (right):
> 
>
https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
> Source/web/ColorChooserPopupUIController.cpp:56:
> ColorChooserPopupUIController::~ColorChooserPopupUIController()
> On 2014/10/07 06:35:19, sof wrote:
> > On 2014/10/07 03:31:29, haraken wrote:
> > > 
> > > Can we add ASSERT(!m_popup) ?
> > 
> > I'm quite likely missing some invariant, but why can we assume so?
> 
> Sorry, I was misreading the code. We cannot assume that.
> 
>
https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
> Source/web/ColorChooserPopupUIController.cpp:58: endChooser();
> On 2014/10/07 06:35:19, sof wrote:
> > On 2014/10/07 03:31:29, haraken wrote:
> > > 
> > > Both ~ColorChooserPopupUIController and ~ColorChooserUIController call
> > > endChooser(), which is not that nice.
> > > 
> > > Can we rewrite the code like this?
> > > 
> > > virtual ~ColorChooserUIController() {
> > >   endChooser();
> > > }
> > > 
> > > virtual ~ColorChooserPopupUIController {
> > >   if (m_popup)
> > >     closePopup();
> > > }
> > 
> > That changes the order for the popup controller, closing the popup page
> > earlier/first.
> > 
> > endChooser() isn't invoked twice, somewhat stodgily achieved perhaps, but I
> > prefer to keep it the way it is now/here.
> 
> Does the order matter?
> 
> Not related to this CL, it looks not nice to call endChooser() in both 
> ~ColorChooserPopupUIController and ~ColorChooserUIController. If the order
> doesn't matter, it looks better to remove endChooser() in
> ~ColorChooserPopupUIController.

endChooser() isn't called twice currently. Nor will it be here.

Hmm, chasing through the back&forth between the chooser and the various client
abstractions that endChooser() triggers, there's a problem lurking here. i.e.,
we may end up back in ColorInputType::didEndChooser(), which may trigger an
event. But we're not allowed access of that heap object.

Will address that & with that part removed, I'm content that we can swap the
order without causing problems.

[sof, 2014-10-07 07:48:23]

On 2014/10/07 06:59:56, sof wrote:
> On 2014/10/07 06:43:59, haraken wrote:
> >
>
https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
> > File Source/web/ColorChooserPopupUIController.cpp (right):
> > 
> >
>
https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
> > Source/web/ColorChooserPopupUIController.cpp:56:
> > ColorChooserPopupUIController::~ColorChooserPopupUIController()
> > On 2014/10/07 06:35:19, sof wrote:
> > > On 2014/10/07 03:31:29, haraken wrote:
> > > > 
> > > > Can we add ASSERT(!m_popup) ?
> > > 
> > > I'm quite likely missing some invariant, but why can we assume so?
> > 
> > Sorry, I was misreading the code. We cannot assume that.
> > 
> >
>
https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopup...
> > Source/web/ColorChooserPopupUIController.cpp:58: endChooser();
> > On 2014/10/07 06:35:19, sof wrote:
> > > On 2014/10/07 03:31:29, haraken wrote:
> > > > 
> > > > Both ~ColorChooserPopupUIController and ~ColorChooserUIController call
> > > > endChooser(), which is not that nice.
> > > > 
> > > > Can we rewrite the code like this?
> > > > 
> > > > virtual ~ColorChooserUIController() {
> > > >   endChooser();
> > > > }
> > > > 
> > > > virtual ~ColorChooserPopupUIController {
> > > >   if (m_popup)
> > > >     closePopup();
> > > > }
> > > 
> > > That changes the order for the popup controller, closing the popup page
> > > earlier/first.
> > > 
> > > endChooser() isn't invoked twice, somewhat stodgily achieved perhaps, but
I
> > > prefer to keep it the way it is now/here.
> > 
> > Does the order matter?
> > 
> > Not related to this CL, it looks not nice to call endChooser() in both 
> > ~ColorChooserPopupUIController and ~ColorChooserUIController. If the order
> > doesn't matter, it looks better to remove endChooser() in
> > ~ColorChooserPopupUIController.
> 
> endChooser() isn't called twice currently. Nor will it be here.
> 
> Hmm, chasing through the back&forth between the chooser and the various client
> abstractions that endChooser() triggers, there's a problem lurking here. i.e.,
> we may end up back in ColorInputType::didEndChooser(), which may trigger an
> event. But we're not allowed access of that heap object.
> 
> Will address that & with that part removed, I'm content that we can swap the
> order without causing problems.

Now done; ptal?

[Mike West, 2014-10-07 07:53:18]

Still LGTM.

[sof, 2014-10-07 07:59:22]

https://codereview.chromium.org/631833002/diff/20001/Source/web/ColorChooserP...
File Source/web/ColorChooserPopupUIController.h (right):

https://codereview.chromium.org/631833002/diff/20001/Source/web/ColorChooserP...
Source/web/ColorChooserPopupUIController.h:73:
RawPtrWillBeMember<ColorChooserClient> m_client;
Wait a sec, this is redundant - the UIController parent already keeps a client
reference.

[sof, 2014-10-07 08:09:25]

On 2014/10/07 07:59:22, sof wrote:
>
https://codereview.chromium.org/631833002/diff/20001/Source/web/ColorChooserP...
> File Source/web/ColorChooserPopupUIController.h (right):
> 
>
https://codereview.chromium.org/631833002/diff/20001/Source/web/ColorChooserP...
> Source/web/ColorChooserPopupUIController.h:73:
> RawPtrWillBeMember<ColorChooserClient> m_client;
> Wait a sec, this is redundant - the UIController parent already keeps a client
> reference.

Sorry about the churn; now tidied away.

[haraken, 2014-10-07 08:27:36]

https://codereview.chromium.org/631833002/diff/40001/Source/core/html/forms/C...
File Source/core/html/forms/ColorInputType.cpp (right):

https://codereview.chromium.org/631833002/diff/40001/Source/core/html/forms/C...
Source/core/html/forms/ColorInputType.cpp:87: // its ColorChooserClient when
doing so (Oilpan friendly finalization.)

Hmm, this looks a bit weird. Can we either:

- make the ColorChooser send a notification to the clients so that the clients
can dispatch the event; or

- make the client observe the ColorChooser (using a weak member), register the
event dispatch in the weak processing, and actually dispatch the event in a
later event loop?

https://codereview.chromium.org/631833002/diff/40001/Source/core/html/forms/C...
Source/core/html/forms/ColorInputType.cpp:91: didEndChooser();

Is it safe to dispatch an event inside the destructor? I'm afraid it can lead to
script execution inside the destructor.

https://codereview.chromium.org/631833002/diff/40001/Source/web/ColorChooserU...
File Source/web/ColorChooserUIController.cpp (right):

https://codereview.chromium.org/631833002/diff/40001/Source/web/ColorChooserU...
Source/web/ColorChooserUIController.cpp:90: ASSERT(m_client);

I think you need to remove this ASSERT.

[sof, 2014-10-07 08:31:45]

https://codereview.chromium.org/631833002/diff/40001/Source/core/html/forms/C...
File Source/core/html/forms/ColorInputType.cpp (right):

https://codereview.chromium.org/631833002/diff/40001/Source/core/html/forms/C...
Source/core/html/forms/ColorInputType.cpp:87: // its ColorChooserClient when
doing so (Oilpan friendly finalization.)
On 2014/10/07 08:27:35, haraken wrote:
> 
> Hmm, this looks a bit weird. Can we either:
> 
> - make the ColorChooser send a notification to the clients so that the clients
> can dispatch the event; or
> 
> - make the client observe the ColorChooser (using a weak member), register the
> event dispatch in the weak processing, and actually dispatch the event in a
> later event loop?

That sounds needlessly complicated. Should we simply not dispatch the event?

[haraken, 2014-10-07 08:49:27]

https://codereview.chromium.org/631833002/diff/40001/Source/core/html/forms/C...
File Source/core/html/forms/ColorInputType.cpp (right):

https://codereview.chromium.org/631833002/diff/40001/Source/core/html/forms/C...
Source/core/html/forms/ColorInputType.cpp:87: // its ColorChooserClient when
doing so (Oilpan friendly finalization.)
On 2014/10/07 08:31:45, sof wrote:
> On 2014/10/07 08:27:35, haraken wrote:
> > 
> > Hmm, this looks a bit weird. Can we either:
> > 
> > - make the ColorChooser send a notification to the clients so that the
clients
> > can dispatch the event; or
> > 
> > - make the client observe the ColorChooser (using a weak member), register
the
> > event dispatch in the weak processing, and actually dispatch the event in a
> > later event loop?
> 
> That sounds needlessly complicated. Should we simply not dispatch the event?

I'm not sure. If it's OK to not dispatch the event, that should be the best
solution :)

tkent-san: Do you have any idea on this?

[sof, 2014-10-08 10:26:35]

Can ~ColorInputType() acceptably not call didEndChooser()?

https://codereview.chromium.org/631833002/diff/40001/Source/web/ColorChooserU...
File Source/web/ColorChooserUIController.cpp (right):

https://codereview.chromium.org/631833002/diff/40001/Source/web/ColorChooserU...
Source/web/ColorChooserUIController.cpp:90: ASSERT(m_client);
On 2014/10/07 08:27:35, haraken wrote:
> 
> I think you need to remove this ASSERT.

Done.

[haraken, 2014-10-08 11:42:18]

> Can ~ColorInputType() acceptably not call didEndChooser()?

I noticed that ~ColorInputType() cannot call didEndChooser() because
didEndChooser() touches element(), which is on-heap.

Can we probably add a strong pointer from the element to the ColorInputType and
thus guarantee that the ColorInputType and the element die together? Then we
don't need to call didEndChooser() in ~ColorInputType().

[sof, 2014-10-08 12:16:48]

On 2014/10/08 11:42:18, haraken wrote:
> > Can ~ColorInputType() acceptably not call didEndChooser()?
> 
> I noticed that ~ColorInputType() cannot call didEndChooser() because
> didEndChooser() touches element(), which is on-heap.
> 
> Can we probably add a strong pointer from the element to the ColorInputType
and
> thus guarantee that the ColorInputType and the element die together? Then we
> don't need to call didEndChooser() in ~ColorInputType().

Right, yes. HTMLInputElement::m_inputType and InputTypeView::m_element provide
that Member pairing.

(The call to endChooser() in ~ColorInputType() on ToT makes this a potential
problem already.)

[haraken, 2014-10-08 12:47:42]

On 2014/10/08 12:16:48, sof wrote:
> On 2014/10/08 11:42:18, haraken wrote:
> > > Can ~ColorInputType() acceptably not call didEndChooser()?
> > 
> > I noticed that ~ColorInputType() cannot call didEndChooser() because
> > didEndChooser() touches element(), which is on-heap.
> > 
> > Can we probably add a strong pointer from the element to the ColorInputType
> and
> > thus guarantee that the ColorInputType and the element die together? Then we
> > don't need to call didEndChooser() in ~ColorInputType().
> 
> Right, yes. HTMLInputElement::m_inputType and InputTypeView::m_element provide
> that Member pairing.
> 
> (The call to endChooser() in ~ColorInputType() on ToT makes this a potential
> problem already.)

Then I think we can (should) remove endChooser() in ~ColorInputType().

[sof, 2014-10-08 12:52:43]

sigbjornf@opera.com changed reviewers:
+ keishi@chromium.org

[sof, 2014-10-08 12:52:44]

+keishi

[sof, 2014-10-08 12:53:21]

On 2014/10/08 12:47:42, haraken wrote:
> On 2014/10/08 12:16:48, sof wrote:
> > On 2014/10/08 11:42:18, haraken wrote:
> > > > Can ~ColorInputType() acceptably not call didEndChooser()?
> > > 
> > > I noticed that ~ColorInputType() cannot call didEndChooser() because
> > > didEndChooser() touches element(), which is on-heap.
> > > 
> > > Can we probably add a strong pointer from the element to the
ColorInputType
> > and
> > > thus guarantee that the ColorInputType and the element die together? Then
we
> > > don't need to call didEndChooser() in ~ColorInputType().
> > 
> > Right, yes. HTMLInputElement::m_inputType and InputTypeView::m_element
provide
> > that Member pairing.
> > 
> > (The call to endChooser() in ~ColorInputType() on ToT makes this a potential
> > problem already.)
> 
> Then I think we can (should) remove endChooser() in ~ColorInputType().

Agree and now done.

[haraken, 2014-10-08 13:09:22]

LGTM, but keishi@ should confirm.

[sof, 2014-10-08 14:40:39]

On 2014/10/08 13:09:22, haraken wrote:
> LGTM, but keishi@ should confirm.

Checking the other InputTypes, BaseChooserOnlyDateAndTimeInputType's finalizer
is quite similar to this one, but the DateTimeChooserClient notification it
_might_ generate does not have this problem.

[sof, 2014-10-10 06:03:38]

keishi: looks acceptable?

[keishi, 2014-10-10 06:32:38]

On 2014/10/10 06:03:38, sof wrote:
> keishi: looks acceptable?

Sorry, yeah this looks okay. LGTM

[sof, 2014-10-10 06:41:45]

On 2014/10/10 06:32:38, keishi wrote:
> On 2014/10/10 06:03:38, sof wrote:
> > keishi: looks acceptable?
> 
> Sorry, yeah this looks okay. LGTM

Yay :) Thanks for the reviews, everyone.

[sof, 2014-10-10 06:42:10]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-10-10 06:42:53]

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

[commit-bot: I haz the power, 2014-10-10 08:31:03]

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

[commit-bot: I haz the power, 2014-10-10 08:31:05]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/31120)

[sof, 2014-10-10 09:42:24]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-10-10 09:43:35]

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

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

Committed patchset #5 (id:80001) as 183538