Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define NDEFMessage's, NDEFRecord's and NDEFReadingEvent's constructors normatively #622

Open
rakuco opened this issue Sep 17, 2021 · 10 comments

Comments

@rakuco
Copy link
Member

rakuco commented Sep 17, 2021

We have the following in Web IDL:

interface NDEFMessage {
  constructor(NDEFMessageInit messageInit);
  // ...
};

interface NDEFRecord {
  constructor(NDEFRecordInit recordInit);
  // ...
};

interface NDEFReadingEvent : Event {
  constructor(DOMString type, NDEFReadingEventInit readingEventInitDict);
  /// ...
};
  • There is nothing in the spec saying what NDEFMessage's constructor is actually supposed to do and how it is supposed to use messageInit.
  • I can see some explanation about NDEFRecord and NDEFRecordInit here, but some of the description of what to do with some of recordInit's members is in this section and the rest spread across the spec, which makes it quite confusing to follow.
  • There is a little bit of explanation about NDEFReadingEvent, but the actual steps describing how readingEventInitDict.message leads to the creation of a new NDEFMessage are not there.
@rakuco
Copy link
Member Author

rakuco commented Sep 17, 2021

@beaufortfrancois this is also related to #620, as at the moment the only entry point for setting a recursion depth is NDEFReader.write(), but it's also possible to manually create NDEFMessage and NDEFRecord objects.

@rakuco rakuco changed the title Define NDEFMessage's and NDEFRecord's constructors normatively Define NDEFMessage's, NDEFRecord's and NDEFReadingEvent's constructors normatively Sep 17, 2021
@beaufortfrancois
Copy link
Collaborator

@zolkis Do you have time to look at it?

@zolkis
Copy link
Contributor

zolkis commented Sep 22, 2021

@zolkis Do you have time to look at it?

Not right now, unfortunately - and not until the end of next week.

@beaufortfrancois
Copy link
Collaborator

@zolkis gentle ping

@zolkis
Copy link
Contributor

zolkis commented Oct 18, 2021

IIUC, specifying a constructor for NDEFReadingEvent is not really necessary, as the default steps for "firing an event" apply and the rest (serialNumber and message) is specified in the dispatch step.
I made a section for that constructor, but feels trivial and obsolete. Anyway, if it helps clarity, I will include it.

@zolkis
Copy link
Contributor

zolkis commented Oct 18, 2021

Should we also make explicit steps for the NDEFReader empty constructor, including the initially empty internal slots? It would just repeat information from the descriptions. By default I'd omit it, but if implementers expect having it, we can make it clear it's a trivial constructor.

@zolkis
Copy link
Contributor

zolkis commented Oct 18, 2021

The "static" constructors for NDEFRecord and NDEFMessage would be trivial as well, since the steps to fill them up will depend on the dynamic context, specified in the Creating NDEF message section. However, here we might benefit from formal/normative steps, I am looking into what kind of text reshuffling would it cause in the spec.

@rakuco
Copy link
Member Author

rakuco commented Nov 5, 2021

  • NDEFReadingEvent
  • NDEFReader
  • NDEFRecord/NDEFMessage
    • I don't think the constructor is trivial, as it needs to process recordInit somehow, and that includes hooking into "Creating NDEF message" like you said. All I could find so far was https://w3c.github.io/web-nfc/#dom-ndefrecordinit saying "The NDEFRecordInit dictionary is used to initialize an NDEF record with its record type recordType, and optional record identifier id and payload data data". There is nothing saying how this initialization is supposed to take place though. Specifically, if I write new NDEFRecord(myRecordInit) there is nothing in the spec saying what must happen (including how to initialize recordsDepth).
    • The same applies to NDEFMessage and NDEFMessageInit in https://w3c.github.io/web-nfc/#the-ndefmessage-interface -- the constructor needs to perform non-trivial steps, and the text just says "The NDEFMessageInit dictionary is used to initialize an NDEF message" without saying how such initialization is supposed to happen (this isn't derived from DOM's Event interface, so you need to outline all the initalization steps).
    • With that said, section 9.10 ("Creating NDEF message") has an algorithmic descriptions of both Create NDEF record and Creating NDEF Message that pretty much correspond to the construction steps that are missing above. You could try doing something like this for NDEFRecord, for example:
      • Rename the "create NDEF record given record, context, and recordsDepth" operation to "initialize NDEF record given record, recordInit, context and recordsDepth". record would be an NDEFRecord object that you take as an argument rather than creating here (and the existing record argument becomes recordInit, which is also clearer).
      • Define e.g. NDEFRecord's construction steps as _"The new NDEFRecord(recordInit) constructor steps are to perform "initialize NDEF record" with this, recordInit, ??? [dunno what the context would be] and 0".
      • Other algorithms that invoke "create NDEF record" directly could be changed to something like "1) Let foo be a [=new=] NDEFRecord 2) Invoke "initialize NDEF record" with foo, recordInit, context, and recordsDepth".
    • Something similar would work for NDEFMessage, but it would also be good to specify what "add NDEFRecord to NDEFMessage" means in Add ndef to output.

@zolkis
Copy link
Contributor

zolkis commented Nov 16, 2021

Something similar would work for NDEFMessage, but it would also be good to specify what "add NDEFRecord to NDEFMessage" means in Add ndef to output.

Note that output (internal binary representation to be used by lower layers) is not the same as NDEFMessage (which is a developer input to generate output), i.e. we don't make the promise to update back NDEFMessage as output is being generated. I think for the methods of the spec it should be enough the specify how to generate output, rather than exacting the steps to make output presentable as NDEFMessage, as this was not an intent and it's quite a mine field. If that is needed, let's handle it separately, since it would require revamping a lot of algorithms. Needs more discussion.

@zolkis
Copy link
Contributor

zolkis commented Nov 16, 2021

There we have the problem that NDEFReadingEventInit contains an NDEFMessageInit, which contains a sequence (values), as opposed to NDEFMessage which contains a FrozenArray (references). I am not sure if the conversion steps are standard (citation possible?), or need to be specified in each spec. I wonder if we can have FrozenArray in an event (i.e. NDEFReadingEventInit would contain NDEFMessage instead of NDEFMessageInit).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants