Dismissing of date/time dialogs should not set the value

Dismissing of date/time dialogs should not set the value

Tapping the back button, cancel button, or outside the dialog should not set the value.
DatePickerDialog has a bug since JellyBean where the onDateSet callback is called every time the dialog is dismissed.

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

[keishi, 2014-05-02 02:26:19]

JellyBean onwards seems to call onDateSet() on dismiss. We don't want onDateSet
to be called on tapping back button, cancel button, or outside the dialog.
https://code.google.com/p/android/issues/detail?id=34833

[Miguel Garcia, 2014-05-06 09:01:20]

https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java
(right):

https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java:1:
// Copyright 2012 The Chromium Authors. All rights reserved.
nit: 2014

https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java:13:
* DatePickerDialog has a bug JellyBean and onwards where
OndateSetListener.onDateSet() is called
Since it is not clear if it's a bug or a feature, can you reworded to something
like "The behavior of the DatePickerDialog changed after JellyBean so it now
calls OndateSetListener.onDateSet() even when..."

https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java:16:
class DatePickerDialog extends android.app.DatePickerDialog {
can you rename the class (ChromeDatePickerDialog) so that it does not clash with
the default Android one?

https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java:30:
tryNotifyDateSet();
I would remove the private method and just implement the functionality here.
Also add javadoc saying why we need to override it.

https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java:33:
private void tryNotifyDateSet() {
Shouldn't we call the clear values functionality if "which" is the clear button?

[Miguel Garcia, 2014-05-06 09:02:01]

Sorry for the delay, just wanted to double check that in fact there is no hidden
story of the sudden behaviour change, I CCed you on an internal thread about
that. Since there does not seem to be any agreement and I agree it's a bug let's
proceed with this.


On 2014/05/06 09:01:20, Miguel Garcia wrote:
>
https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java
> (right):
> 
>
https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
>
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java:1:
> // Copyright 2012 The Chromium Authors. All rights reserved.
> nit: 2014
> 
>
https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
>
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java:13:
> * DatePickerDialog has a bug JellyBean and onwards where
> OndateSetListener.onDateSet() is called
> Since it is not clear if it's a bug or a feature, can you reworded to
something
> like "The behavior of the DatePickerDialog changed after JellyBean so it now
> calls OndateSetListener.onDateSet() even when..."
> 
>
https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
>
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java:16:
> class DatePickerDialog extends android.app.DatePickerDialog {
> can you rename the class (ChromeDatePickerDialog) so that it does not clash
with
> the default Android one?
> 
>
https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
>
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java:30:
> tryNotifyDateSet();
> I would remove the private method and just implement the functionality here.
> Also add javadoc saying why we need to override it.
> 
>
https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
>
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java:33:
> private void tryNotifyDateSet() {
> Shouldn't we call the clear values functionality if "which" is the clear
button?

[keishi, 2014-05-07 06:27:57]

https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java
(right):

https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java:1:
// Copyright 2012 The Chromium Authors. All rights reserved.
On 2014/05/06 09:01:20, Miguel Garcia wrote:
> nit: 2014

Done.

https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java:13:
* DatePickerDialog has a bug JellyBean and onwards where
OndateSetListener.onDateSet() is called
On 2014/05/06 09:01:20, Miguel Garcia wrote:
> Since it is not clear if it's a bug or a feature, can you reworded to
something
> like "The behavior of the DatePickerDialog changed after JellyBean so it now
> calls OndateSetListener.onDateSet() even when..."

Done.

https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java:16:
class DatePickerDialog extends android.app.DatePickerDialog {
On 2014/05/06 09:01:20, Miguel Garcia wrote:
> can you rename the class (ChromeDatePickerDialog) so that it does not clash
with
> the default Android one?

Done.

https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java:30:
tryNotifyDateSet();
On 2014/05/06 09:01:20, Miguel Garcia wrote:
> I would remove the private method and just implement the functionality here.
> Also add javadoc saying why we need to override it.

Done.

https://codereview.chromium.org/260903006/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/DatePickerDialog.java:33:
private void tryNotifyDateSet() {
On 2014/05/06 09:01:20, Miguel Garcia wrote:
> Shouldn't we call the clear values functionality if "which" is the clear
button?

I think the clear button handling might be better where it is now, inside
InputDialogContainer.showPickerDialog because the code can be shared between the
ChromeDatePickerDialog,MultiFieldTimePickerDialog,DateTimePickerDialog,MonthPickerDialog,
and WeekPickerDialog.

[Miguel Garcia, 2014-05-07 07:55:57]

> 
> I think the clear button handling might be better where it is now, inside
> InputDialogContainer.showPickerDialog because the code can be shared between
the
>
ChromeDatePickerDialog,MultiFieldTimePickerDialog,DateTimePickerDialog,MonthPickerDialog,
> and WeekPickerDialog.

Makes sense

[Miguel Garcia, 2014-05-07 07:56:03]

lgtm

[keishi, 2014-05-07 08:55:03]

Adding owner aurimas@. PTAL. Thanks!

[aurimas (slooooooooow), 2014-05-08 19:00:54]

lgtm

[keishi, 2014-05-09 01:52:15]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-05-09 01:54:24]

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

[commit-bot: I haz the power, 2014-05-10 03:06:49]

Change committed as 269507