Prevent double notifications from the date/time dialogs. This seems to have been happening in m18 a…

Prevent double notifications from the date/time dialogs. This seems to have been happening in m18 all the time but it produces render crashes when using the new WebCore:DateTimeChooser interface.


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

[Miguel Garcia, 2012-12-05 10:36:19]

Oli, since Marcus is travelling, do you mind having a look at this?

[olilan, 2012-12-05 11:17:10]

https://codereview.chromium.org/11434087/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java
(right):

https://codereview.chromium.org/11434087/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java:127:
mDialogAlreadyDismissed = true;
nit: indent 4 spaces

https://codereview.chromium.org/11434087/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java:145:
if (mShouldSendCancelation) {
I'm not sure what this is trying to do. So we only clear the focus if
mShouldSendCancelation is true? But clearing the focus isn't cancellation, it's
done so that the cursor isn't in the field when the dialog closes, which should
happen if a new value has been set as well.

https://codereview.chromium.org/11434087/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java:263:
mDialogAlreadyDismissed = true;
This looks ok. If we wanted to avoid getting multiple calls to the listeners at
all, we could alternatively change the listener of the "Set" button to just
close the dialog, to avoid the call to tryNotifyDateSet(). However we'd have to
make sure the current behaviour was preserved for ICS.

[Miguel Garcia, 2012-12-05 14:19:09]

Thanks for the review Oli, PTAL. Also any tests downstream I should be running
previous to submitting this?

https://codereview.chromium.org/11434087/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java
(right):

https://codereview.chromium.org/11434087/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java:127:
mDialogAlreadyDismissed = true;
On 2012/12/05 11:17:10, olilan wrote:
> nit: indent 4 spaces

Done.

https://codereview.chromium.org/11434087/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java:145:
if (mShouldSendCancelation) {
Ah I see, I misinterpreted the reasoning behind this message. I think I am going
to need a new message explicitly for cancellations then. I'll remove this from
the review for now.

On 2012/12/05 11:17:10, olilan wrote:
> I'm not sure what this is trying to do. So we only clear the focus if
> mShouldSendCancelation is true? But clearing the focus isn't cancellation,
it's
> done so that the cursor isn't in the field when the dialog closes, which
should
> happen if a new value has been set as well.

https://codereview.chromium.org/11434087/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java:263:
mDialogAlreadyDismissed = true;
I rather not have some explicit behaviour only form the ICS case. This seems to
be fine as is so let's keep it.

On 2012/12/05 11:17:10, olilan wrote:
> This looks ok. If we wanted to avoid getting multiple calls to the listeners
at
> all, we could alternatively change the listener of the "Set" button to just
> close the dialog, to avoid the call to tryNotifyDateSet(). However we'd have
to
> make sure the current behaviour was preserved for ICS.

[olilan, 2012-12-05 14:27:46]

lgtm (apart from a comment)

https://codereview.chromium.org/11434087/diff/8001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java
(right):

https://codereview.chromium.org/11434087/diff/8001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java:68:
// Prevents sending two notifications (from onClick and from onDismiss)
onDismiss isn't sending a notification. Perhaps "onStop" instead.

[Miguel Garcia, 2012-12-05 20:19:48]

Jay do you mind having a look at this since Marcus is travelling?

[Jay Civelli, 2012-12-05 21:05:30]

lgtm

[commit-bot: I haz the power, 2012-12-06 15:35:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miguelg@chromium.org/11434087/8001

[commit-bot: I haz the power, 2012-12-06 18:22:10]

Change committed as 171536