Join NDEFReader and NDEFWriter

[zolkis]

Re-applying #568.

---

Preview | Diff

[beaufortfrancois]

Quick question: Is it clear for web developers that onerror will get fired only when scan has started.
I can see why some web developers would think this code would work.

const ndef = new NDEFReader();
ndef.onerror = event => {
  /* Was there an error while writing? */
}
ndef.write('hello world');

[zolkis]

Good question, we need to clarify that with the two objects merged.

[zolkis]

So should we rename onerror to onreaderror, or get rid of it alltogether and rely on the Promise, like with write()?

[beaufortfrancois]

Would you mind also updating the the EXPLAINER.md file and other files where NDEFWriter is referenced?

[zolkis]

Amended the TypeScript definition to also remove ignoreRead and scan options no longer supported.

[arskama]

Quick question: Is it clear for web developers that onerror will get fired only when scan has started.
I can see why some web developers would think this code would work.

const ndef = new NDEFReader();
ndef.onerror = event => {
  /* Was there an error while writing? */
}
ndef.write('hello world');

@beaufortfrancois
I would like to have a clarification on your comment (it s me not understanding :) )

On the chromium implementation, there are 2 places where onerror is fired.
https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/modules/nfc/ndef_reader.cc;l=217;drc=126a458910eb392beb0c56601c89233cd82d80b5

and

https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/modules/nfc/nfc_proxy.cc;l=122;drc=126a458910eb392beb0c56601c89233cd82d80b5

which are 2 different cases, right?
So I think we need two different handling for those, since the first one is not really a readingerror?

[zolkis]

Since we merged NDEFReader (which had an onerror event) and NDEFWriter (which did not have it), someone may assume that in the joined NDEFReader onerror is also relevant to writing, which is not the case. Therefore we renamed onerror to onreadingerror.

In the impl onerror conveys the NFC read errors (either due to access problems, or technical faults) to the readers.

Looks like since filters are gone, we have only one reader, and NFCProxy could be simplified (readers_ removed etc), in conformance with the new internal slots (PendingReader instead of ActivatedReaderList) of NDEFReader.

[beaufortfrancois]

Argh I see now... You're right @arskama, the first one "NDEFReader::OnMojoConnectionError()" is not per-say a reading error but an internal communication issue that can happen in Chromium.
If I remember correctly though, there's no way to recover from this.

The old behaviour still triggered an error even though it was NOT specifically a reading error.

With that in mind, maybe keeping onerror makes sense.
For info, this will also get triggered if an internal error happens during a write NFC operation and write() promise will reject as well...

Let's keep onerror.
What do you folks think?

[zolkis]

Let's keep onerror. What do you folks think?

So that error could have happened with the write(), too.
When does that error come up typically? Can we handle that via the scan() and write() promise?

[zolkis]

"NDEFReader::OnMojoConnectionError()" is not per-say a reading error but an internal communication issue that can happen in Chromium.

Do I read that correctly: it seems to reject the promise with that error and also fire an onerror? Is the latter needed?
Edit: it's covering the case the error comes after the promise is settled. But does that actually (typically) happen?

[beaufortfrancois]

Let's keep onerror. What do you folks think?

So that error could have happened with the write(), too.
When does that error come up typically? Can we handle that via the scan() and write() promise?

scan will succeed when scan has started successfully, but this error could happen way after.
re: write, it won't be an issue as if this error happens, the promise will reject right away, and the onerror event will also get fired.

"NDEFReader::OnMojoConnectionError()" is not per-say a reading error but an internal communication issue that can happen in Chromium.

Do I read that correctly: it seems to reject the promise with that error and also fire an onerror? Is the latter needed?
Edit: it's covering the case the error comes after the promise is settled. But does that actually (typically) happen?

it is an internal issue which "shouldn't" happen but can happen.

[zolkis]

OK, I will add a commit the revert to onerror and explain the behavior for reading and writing.

[zolkis]

Question, @beaufortfrancois @kenchris

With filters gone, we can still create multiple NDEFReader instances, each of which has a scan() method that operate on the same NFC HW and have an onreading event that can take callbacks.

How should we handle this?

1. Allow all the NDEFReader object instances, but keep a single reader in the internal slot and add the event listeners to its event.
2. Allow all the object instance, and keep all of these in the activated reader list. In the impl, there is a single NFC proxy, though.
3. Allow a single instance of NDEFReader in the given context and decide how to expose it: provide a getter/factory method for it, or let it be a static object, e.g. navigator.nfc (we used to be there).

[zolkis]

Sorry, I needed to amend since I forgot to update the explainer and the TS definitions.
So, as agreed, now there is only one NDEFReader object that is accessed via navigator.nfc.ndef.
NFC state could be also moved to the navigator.nfc object, but for the moment it looked good on the settings object.
However, I moved all relevant content (NFC state, handling NFC adapters, obtaining permission, handling visibility change. releasing NFC) under navigator.nfc.

[beaufortfrancois]

Here's my first feedback to get your thoughts.

[beaufortfrancois]

Shall we use NFC tags consistently?

[zolkis]

True, changed. I was thinking not all NFC tags were NDEF, so using "NDEF tag" was more appropriate for NDEFReader.

[beaufortfrancois]

What does happen with code below?

const promise1 = navigator.nfc.ndef.scan();
const promise2 = navigator.nfc.ndef.scan();

Which promise1 of promise2 reject?
It is not clear to me.

Same for write()...

[beaufortfrancois]

We shall update Separate objects vs navigator.nfc and Separate objects for reader/writer sections in EXPLAINER.md

[zolkis]

Summarized the discussion about constructors vs navigator.nfc in #602.
Also, some options to address remaining issues like the ones @beaufortfrancois mentioned above.

[zolkis]

OK, now we have navigator.nfc that contains ndef of type NDEFReader (exposing scan(), write(), onreading, onreadingerror).

I will make a branch from here to explore option 2 from #602 comment. That is, navigator.nfc would contain ontagfound, onerror, and ndef of type NDEFReader that contains read() and write().
Scanning (NFC polling) is separated from reads, ignoreRead is not needed any more since there is no automatic content read from tags, and internal slots for pending read and write are not needed, either. Reading and dispatching algorithms become much simpler. Also, this would be closer to the Android NFC API usage flow, and more future proof.

This would permit later improvements, such as the ontagfound event could contain NFC technologies supported, but since that is restricted to NDEF at the moment, we can use a simple event. However, we could use flags already available from Android, such as writeable, formattable etc.
Future NFC tech could get their API namespace under navigator.nfc, for instance navigator.nfc.isodep exposing transceive() etc. The same navigator.nfc.ontagfound could be used (with expanded content, so backwards compatible).
Also, a navigator.nfc.ontagreleased event could be added later, if a developer need arises (it's quite common in other APIs, but will only add here when there are clear use cases for it).

The minimal delta from this point that supports the current functionality would be quite small, but let's see how

[zolkis]

Reverted attaching to navigator. That version is saved in PR #605

[beaufortfrancois]

Here's my feedback for the explainer.
I'll now review spec changes.