Right now NFC API still uses DOMRequest, but other Web API are start migrating to Promise now, so we should also try to see if we can replace DOMRequest by Promise. Also see comments from smuag, https://bugzilla.mozilla.org/show_bug.cgi?id=970251#c13

Comment on attachment 8518017 [details] [diff] [review] Part1. WebIDL change v2 Review of attachment 8518017 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/MozNFCTag.webidl @@ +24,5 @@ > [JSImplementation="@mozilla.org/nfc/NFCTag;1", AvailableIn="CertifiedApps"] > interface MozNFCTag { > + Promise<sequence<MozNDEFRecord>> readNDEF(); > + Promise<void> writeNDEF(sequence<MozNDEFRecord> records); > + Promise<void> makeReadOnlyNDEF(); It have been renamed to makeReadOnly. Remember to rebase your patch,

Comment on attachment 8518020 [details] [diff] [review] Part2. Gecko implementation v2 Review of attachment 8518020 [details] [diff] [review]: ----------------------------------------------------------------- just a quick glimse, I'll continue reviewing tomorrow. ::: dom/nfc/NfcContentHelper.js @@ +268,5 @@ > } > }, > > // nsIMessageListener > + fireRequestSuccess: function fireRequestSuccess(requestId, result, resultType) { fireRequestSuccess should be removed, using resultType to dispatch is bad. ::: dom/nfc/nsINfcContentHelper.idl @@ +56,5 @@ > void notifyPeerLost(in DOMString sessionToken); > }; > > +[scriptable, uuid(7f8472be-841a-4076-b6db-610deedff431)] > +interface nsINfcRequestCb : nsISupports For interface, we prefer full name, like nsINfcRequestCallback. ::: dom/nfc/nsNfc.js @@ +32,5 @@ > + * queryInterface method MUST implement Ci.nsISupportsWeakReference & > + * Ci.nsIObserver. > + */ > + __proto__: DOMRequestIpcHelper.prototype, > + Shouldn't this implement nsINfcRequestCallback? @@ +48,5 @@ > + aCallback(requestId); > + }); > + }, > + > + notifySuccess: function(aRequestId) { function name is missing, and below @@ +113,3 @@ > }, > writeNDEF: function writeNDEF(records) { > + let promise = getPromise(requestId => { this.getPromise

Sample patch to use callback. And this callback wraps up the DOMRequest.

Comment on attachment 8518020 [details] [diff] [review] Part2. Gecko implementation v2 Review of attachment 8518020 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/nsINfcContentHelper.idl @@ +111,5 @@ > + */ > + void writeNDEF(in nsIVariant records, > + in DOMString sessionToken, > + in DOMString requestId, > + in nsINfcRequestCb callback); Please see my patch for wrapping DOMRequest(or Promise), I'd prefer that the API takes only callback as parameters. Passing requestId and callback at that same time seems unneccessary.

Comment on attachment 8519861 [details] [diff] [review] Part2. Gecko implementation v3 Review of attachment 8519861 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/NfcContentHelper.js @@ +138,5 @@ > > // NFCTag interface > + readNDEF: function readNDEF(sessionToken, callback) { > + let requestId = callback.getCallbackId(); > + this._requestMap[requestId] = callback; Perhaps rename to callbackId and callbackMap. @@ +160,3 @@ > }, > > + makeReadOnlyNDEF: function makeReadOnlyNDEF(sessionToken, callback) { need to rebase this. @@ +282,5 @@ > receiveMessage: function receiveMessage(message) { > DEBUG && debug("Message received: " + JSON.stringify(message)); > let result = message.json; > > + if ("requestId" in result) { Since DOMEvent msg doesn't have this property, I'd suggest move this code into those handleXXXResponse code. ::: dom/nfc/nsNfc.js @@ +84,5 @@ > + resolver.reject(aErrorMsg); > + }, > + > + QueryInterface: XPCOMUtils.generateQI([Ci.nsISupportsWeakReference, > + Ci.nsIObserver]), nsINfcRequestCallback is missing? @@ +163,5 @@ > }; > + > + let callback = new NfcCallback(this._window); > + this._nfcContentHelper.sendFile(Cu.cloneInto(data, this._window), > + this.session, callback); line up with Cu.

Add r? to smaug for WebIDL change from DOMRequest to Promise

Comment on attachment 8518017 [details] [diff] [review] Part1. WebIDL change v2 Review of attachment 8518017 [details] [diff] [review]: ----------------------------------------------------------------- cancel review because we would like add some comments to WebIDL

Comment on attachment 8519861 [details] [diff] [review] Part2. Gecko implementation v3 Review of attachment 8519861 [details] [diff] [review]: ----------------------------------------------------------------- The line "#include "nsIDOMDOMRequest.idl"" in nsINfcContentHelper.idl should be removed, also.

Add r? to smaug for webIDL change.

Comment on attachment 8512498 [details] [diff] [review] Part3. NFC testcase modification Review of attachment 8512498 [details] [diff] [review]: ----------------------------------------------------------------- [PATCH 3/3] in Patch Subject should be removed.

Hi Yoshi, could you help review again because adding another function named handleGeneralResponse in Nfc.js to address Comment 12. Also included in this patch - Fix nits - Rebase to latest code

Comment on attachment 8526584 [details] [diff] [review] Part2. Gecko implementation v4 Review of attachment 8526584 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/NfcContentHelper.js @@ +371,3 @@ > } > + this._requestMap[requestId].notifySuccessWithNDEFRecords(ndefMsg); > + delete this._requestMap[requestId]; It seems the property is only deleted when success.

Thanks for finding this, update patch to fix this issue

Fix issue

Comment on attachment 8526600 [details] [diff] [review] Part2. Gecko implementation v5 Review of attachment 8526600 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/NfcContentHelper.js @@ +355,2 @@ > return; > } I think the original style is easier. let callback = this._requestMap[requestId]; if (!callback) { return; } delete this._requestMap[requestId]; callback.notifyXXX();

running try : https://tbpl.mozilla.org/?tree=Try&rev=db2979de79d8