Don't store the DateTimePicker client in a scoped_ptr.

Reset the date/time picker client in the RenderViewImpl destructor.

Because the RenderDateTimePicker class is a RenderViewObserver, it'll
register itself on the RenderViewImpl class causing it to be destroyed
as the very last thing in ~RenderViewImpl() user code.  However, because
it's stored in a scoped_ptr<> we can end up deleting it twice.

BUG=176495
NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183482

[Peter Beverloo, 2013-02-18 17:37:55]

+miguelg, avi/jam for review.

This patch only affects Android and has been marked as a M25 release blocker.

[Miguel Garcia, 2013-02-18 17:48:48]

lgtm

https://codereview.chromium.org/12298022/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12298022/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:6478: if (date_time_picker_client_ != NULL)
(I introduced the original bug so thanks a lot for fixing!)

This seems like the simpler approach to fix the problem right now, I would
nevertheless like some feedback on how to solve this in the more general case.
The DateTimePicker client is an observer and as such it seems it should not go
inside a scoped_pointer because observers are destroyed in its own way. However
the picker needs to be regenerated with new input from WebKit.

I see two ways I guess;

Split the picker functionality in two completely different objects (even though
they are very related) and let them be destroyed separately.

Add an "update" method or the like and reuse the picker instead of regenerating
it every time.

Feedback is welcome for the future fix indeed. Note that we have to patch this
in M25 so I would vouch for this simple fix first.

[jam, 2013-02-19 16:24:43]

it seems simpler to have "date_time_picker_client_.reset();" in the destructor?

[Peter Beverloo, 2013-02-19 16:43:59]

On 2013/02/19 16:24:43, jam wrote:
> it seems simpler to have "date_time_picker_client_.reset();" in the
destructor?

Yes, that is simpler. Thank you for the suggestion!
New patch set uploaded.

[jam, 2013-02-19 18:24:36]

lgtm

[commit-bot: I haz the power, 2013-02-19 18:25:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@chromium.org/12298022/2002

[klobag.chromium, 2013-02-20 01:48:29]

On 2013/02/19 18:25:31, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/peter%40chromium.org/12298022/2002

The change is under #ifdef ANDROID. It can't be the reason to break win7_aura.
Mark NOTRY as we need it.

[commit-bot: I haz the power, 2013-02-20 10:16:49]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-02-20 11:24:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@chromium.org/12298022/2002

[commit-bot: I haz the power, 2013-02-20 11:45:57]

Change committed as 183482