[webnfc] Cancel nfc.push operation when timeout expires

[webnfc] Cancel nfc.push operation when timeout expires

This CL adds missing timeout handling functionality [1]. When user
provides timeout value for the push operation, and NFC tag is not
in proximity for the duration of timeout value, operation must be
rejected with timeout error. Unit test is added to test new
functionality.

https://w3c.github.io/web-nfc/#the-push-method (Step 14).

BUG=714037
Review-Url: https://codereview.chromium.org/2851453004
Cr-Commit-Position: refs/heads/master@{#469294}
Committed: https://chromium.googlesource.com/chromium/src/+/b49d34931454bc1fdffdba99d6a0b344430746b5

[shalamov, 2017-04-28 11:33:15]

The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 11:33:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 12:19:58]

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

[commit-bot: I haz the power, 2017-04-28 12:19:59]

Dry run: This issue passed the CQ dry run.

[shalamov, 2017-04-28 12:36:46]

alexander.shalamov@intel.com changed reviewers:
+ reillyg@chromium.org

[shalamov, 2017-04-28 12:36:47]

Reilly, could you please take a look? Thanks.

[Reilly Grant (use Gerrit), 2017-04-28 16:10:28]

https://codereview.chromium.org/2851453004/diff/1/device/nfc/android/java/src...
File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right):

https://codereview.chromium.org/2851453004/diff/1/device/nfc/android/java/src...
device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:183:
mPendingPushOperation.complete(createError(NfcErrorType.OPERATION_CANCELLED));
Call cancelPushTimeoutTask() here so that we can assert that there is no pending
timeout task in schedulePushTimeoutTask().

[shalamov, 2017-05-02 09:51:02]

The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 09:51:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shalamov, 2017-05-02 09:51:22]

https://codereview.chromium.org/2851453004/diff/1/device/nfc/android/java/src...
File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right):

https://codereview.chromium.org/2851453004/diff/1/device/nfc/android/java/src...
device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:183:
mPendingPushOperation.complete(createError(NfcErrorType.OPERATION_CANCELLED));
On 2017/04/28 16:10:28, Reilly Grant wrote:
> Call cancelPushTimeoutTask() here so that we can assert that there is no
pending
> timeout task in schedulePushTimeoutTask().

Done.

[shalamov, 2017-05-02 09:53:03]

On 2017/04/28 16:10:28, Reilly Grant wrote:
> Call cancelPushTimeoutTask() here so that we can assert that there is no
pending
> timeout task in schedulePushTimeoutTask().

Should I add assert in schedulePushTimeoutTask()?

[shalamov, 2017-05-02 10:34:01]

Patchset #2 (id:20001) has been deleted

[shalamov, 2017-05-02 10:34:50]

The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 10:35:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 11:22:46]

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

[commit-bot: I haz the power, 2017-05-02 11:22:47]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2017-05-02 15:41:23]

On 2017/05/02 09:53:03, shalamov wrote:
> On 2017/04/28 16:10:28, Reilly Grant wrote:
> > Call cancelPushTimeoutTask() here so that we can assert that there is no
> pending
> > timeout task in schedulePushTimeoutTask().
> 
> Should I add assert in schedulePushTimeoutTask()?

Yes. We should make the expected state transitions as clear as possible.

[Alexander Shalamov, 2017-05-02 17:03:54]

On 2017/05/02 15:41:23, Reilly Grant wrote:
> On 2017/05/02 09:53:03, shalamov wrote:
> > On 2017/04/28 16:10:28, Reilly Grant wrote:
> > > Call cancelPushTimeoutTask() here so that we can assert that there is no
> > pending
> > > timeout task in schedulePushTimeoutTask().
> > 
> > Should I add assert in schedulePushTimeoutTask()?
> 
> Yes. We should make the expected state transitions as clear as possible.

Added.
Line 627: assert mPushTimeoutRunnable == null;

[Reilly Grant (use Gerrit), 2017-05-02 17:52:12]

https://codereview.chromium.org/2851453004/diff/40001/device/nfc/android/java...
File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right):

https://codereview.chromium.org/2851453004/diff/40001/device/nfc/android/java...
device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:183:
completePendingPushOperation(createError(NfcErrorType.OPERATION_CANCELLED));
The reason I suggested calling cancelPushTimeoutTask instead of
completePendingPushOperation is that as written we will now always disable
reader mode and then re-enable it. I'm not sure that churn is desirable.

[shalamov, 2017-05-03 11:46:55]

The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 11:47:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shalamov, 2017-05-03 11:49:00]

On 2017/05/02 17:52:12, Reilly Grant wrote:
>
https://codereview.chromium.org/2851453004/diff/40001/device/nfc/android/java...
> File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right):
> 
>
https://codereview.chromium.org/2851453004/diff/40001/device/nfc/android/java...
> device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:183:
> completePendingPushOperation(createError(NfcErrorType.OPERATION_CANCELLED));
> The reason I suggested calling cancelPushTimeoutTask instead of
> completePendingPushOperation is that as written we will now always disable
> reader mode and then re-enable it. I'm not sure that churn is desirable.

Thanks, I missed that part when I refactored code. I added more tests for
different cases related to timeout handling.

[commit-bot: I haz the power, 2017-05-03 12:38:38]

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

[commit-bot: I haz the power, 2017-05-03 12:38:39]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2017-05-03 20:54:03]

lgtm with nit

https://codereview.chromium.org/2851453004/diff/60001/device/nfc/android/java...
File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right):

https://codereview.chromium.org/2851453004/diff/60001/device/nfc/android/java...
device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:647: return
mPushTimeoutHandler.postDelayed(mPushTimeoutRunnable, (long) options.timeout);
Since tasks are not guaranteed to run it isn't worth checking this return value.

[shalamov, 2017-05-04 07:37:56]

The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 07:38:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 08:20:04]

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

[commit-bot: I haz the power, 2017-05-04 08:20:05]

Dry run: This issue passed the CQ dry run.

[shalamov, 2017-05-04 08:40:14]

The CQ bit was checked by alexander.shalamov@intel.com

[shalamov, 2017-05-04 08:40:14]

The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2851453004/#ps80001
(title: "Don't check return value from postDelayed")

[commit-bot: I haz the power, 2017-05-04 08:40:28]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 08:45:00]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493887214207600,
"parent_rev": "11b6b3790f4138d11f81a3af384d129383ea976a", "commit_rev":
"b49d34931454bc1fdffdba99d6a0b344430746b5"}

[commit-bot: I haz the power, 2017-05-04 08:45:11]

Description was changed from

==========
[webnfc] Cancel nfc.push operation when timeout expires

This CL adds missing timeout handling functionality [1]. When user
provides timeout value for the push operation, and NFC tag is not
in proximity for the duration of timeout value, operation must be
rejected with timeout error. Unit test is added to test new
functionality.

https://w3c.github.io/web-nfc/#the-push-method (Step 14).

BUG=714037
==========

to

==========
[webnfc] Cancel nfc.push operation when timeout expires

This CL adds missing timeout handling functionality [1]. When user
provides timeout value for the push operation, and NFC tag is not
in proximity for the duration of timeout value, operation must be
rejected with timeout error. Unit test is added to test new
functionality.

https://w3c.github.io/web-nfc/#the-push-method (Step 14).

BUG=714037

Review-Url: https://codereview.chromium.org/2851453004
Cr-Commit-Position: refs/heads/master@{#469294}
Committed:
https://chromium.googlesource.com/chromium/src/+/b49d34931454bc1fdffdba99d6a0...
==========

[commit-bot: I haz the power, 2017-05-04 08:45:12]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b49d34931454bc1fdffdba99d6a0...