- From: Jeffrey Yasskin <jyasskin@google.com>
- Date: Wed, 16 Sep 2015 12:53:59 -0700
- To: Anssi Kostiainen via GitHub <sysbot+gh@w3.org>
- Cc: "Web NFC (W3C)" <public-web-nfc@w3.org>, Zoltan Kis <zoltan.kis@intel.com>
- Message-ID: <CANh-dXm9eSL6LfsBrg_TsfsWzBALHONBk0ge2W5ypfVjJOPEzg@mail.gmail.com>
I think you can merge right away, and handle fixes in subsequent PRs. Comments follow: - In https://rawgit.com/zolkis/web-nfc/gh-pages/index.html#the-watch-method: - "Browsing contexts" ( https://html.spec.whatwg.org/multipage/browsers.html#browsing-context) don't have origins. They have an active document that has an origin, or you can talk about the incumbent settings object ( https://html.spec.whatwg.org/multipage/webappapis.html#incumbent-settings-object), which also has an origin. This appears in postMessage() too. - "If the first argument options contains invalid values" doesn't say what makes a value "invalid". You need to link to something that specifies that. - Usually, WebIDL will check the types of arguments for you, for example in https://heycam.github.io/webidl/#es-callback-function for the MessageCallback parameter. - "If the browsing context loses focus" doesn't really fit in the watch() algorithm. There should be another section that does things when focus is lost and regained. - In https://rawgit.com/zolkis/web-nfc/gh-pages/index.html#the-nfcwatchoptions-dictionary - Once you've given default values in the dictionary, you don't need to also give them in prose. e.g. you can remove "The default value is null." - You should probably refer to http://www.w3.org/TR/page-visibility/ when defining "focus". Do you really mean "focus"? If I click on my text editor or NFC debugger but can still see the NFC-using page, should it lose the ability to use NFC? - Does "trusted integrity NFC content" ever exist? How would a browser identify it? If we don't have a way to identify it, it probably shouldn't be in the spec. - In https://rawgit.com/zolkis/web-nfc/gh-pages/index.html#data-representation : - Saying "The NFCRecord.kind property represents the NFCRecordType…" is redundant with the IDL that declares kind as having the type NFCRecordType. - The "valid NFCRecord" definition is mostly redundant with the IDL definition of NFCRecord. You can remove the assertion on 'record.kind', and you should probably replace the assertion on record.data with a more precise union type for that field. "(DOMString or unrestricted double or object or BufferSource)?" should do it. - In https://rawgit.com/zolkis/web-nfc/gh-pages/index.html#dfn-receive-an-ndef-message : - "When the UA is to receive an NDEF message …" isn't quite the right phrasing for handling an event that comes in from outside the UA. Maybe "When the UA receives an NDEF message …". - Using "If the top-level browsing context loses focus" as step 1 of this algorithm is a little weird. Do you mean to say that if the browsing context loses focus during the execution of this algorithm, the algorithm needs to be paused until focus comes back, or that if the browsing context doesn't have focus when the algorithm starts, the steps should just be aborted? - "If options.url is defined" will always be true, because NFCWatchOptions gives it a default value. - I don't see any enforcement that pages only receive NDEF messages from their own origin. Where is it? I suspect once this enforcement is in place, you won't need to check for "https" in the Web NFC Id, since the "secure context" restriction will handle it for you. - In the urn:nfc:ext:w3.org:webnfc branch, you probably need to continue to the next record, to avoid adding the uninitialized record to `message`. - These sub-steps should all be ordered lists, not bullets. - https://rawgit.com/zolkis/web-nfc/gh-pages/index.html#URL_pattern should mention whether "https://*mydomain.com/" is a valid pattern, or whether the "*" has to appear right before a ".". I feel like we had security problems in Chrome Extensions when we allowed "*" to be part of a host component, and it's not allowed anymore ( https://developer.chrome.com/extensions/match_patterns), but I don't remember exactly what the problems were. - In https://rawgit.com/zolkis/web-nfc/gh-pages/index.html#the-pushmessage-method : - You don't need to say "If the options parameter is undefined, then let options be a new NFCPushOptions object with the default values." WebIDL does that for you. - "On any termination of this algorithm, output must be cleared." is odd. `output` is a local variable, so of course it gets a fresh value on any execution of this algorithm. - A record.kind of "empty" silently throws away all the other records? That seems surprising. - The "When an NFC device comes within communication range" steps don't actually say to transfer any data. - "the sending MAY only happen if an NFC tag is tapped" means the sending can happen if an NFC peer is tapped, at the discretion of the UA. Is that what you mean? - Steps "If timer exists" through "If the top-level browsing context loses focus" should be phrased as "Wait until one of the following events happens: <sublist>" since they don't happen in sequence. On Tue, Sep 15, 2015 at 8:53 AM, Anssi Kostiainen via GitHub < sysbot+gh@w3.org> wrote: > @jyasskin The editors believe [your feedback][1] has now been > addressed. Could you please review this PR, and let @zolkis and > @kenchris know if you'd like the editors to do further changes before > merging. This version of the spec does not need to be perfect, but it > should be a good base for the next iteration of the experimental > implementation. > > For convenience, here's the [RawGit HTML preview of the head of this > PR][2]. > > Thank you again for your extensive and detailed feedback. > > [1]: > https://lists.w3.org/Archives/Public/public-web-nfc/2015Sep/0033.html > [2]: https://rawgit.com/zolkis/web-nfc/gh-pages/index.html > > -- > GitHub Notif of comment by anssiko > See https://github.com/w3c/web-nfc/pull/46#issuecomment-140439333 > >
Received on Wednesday, 16 September 2015 19:54:48 UTC