Closed Bug 996397 Opened 10 years ago Closed 9 years ago

B2G NFC: Replace DOMRequests with Promises

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
tracking-b2g backlog

People

(Reporter: allstars.chh, Assigned: dimi)

References

Details

(Whiteboard: [p=5])

Attachments

(3 files, 10 obsolete files)

5.24 KB, patch
Details | Diff | Splinter Review
8.50 KB, patch
Details | Diff | Splinter Review
36.27 KB, patch
dimi
: review+
Details | Diff | Splinter Review
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
blocking-b2g: --- → backlog
Summary: B2G NFC: Replace DOMRequest by Promise → B2G NFC: Investigate if DOMRequests should be replaced with Promises
Assignee: nobody → dlee
Attached patch Part1. WebIDL change v1 (obsolete) — Splinter Review
Attachment #8512496 - Flags: review?(allstars.chh)
Attached patch Part2. Gecko implementation v1 (obsolete) — Splinter Review
Attachment #8512497 - Flags: review?(allstars.chh)
Attached patch Part3. NFC testcase modification (obsolete) — Splinter Review
Attachment #8512496 - Flags: review?(allstars.chh)
Attachment #8512497 - Flags: review?(allstars.chh)
Attached patch Part1. WebIDL change v2 (obsolete) — Splinter Review
Attachment #8512496 - Attachment is obsolete: true
Attachment #8512497 - Attachment is obsolete: true
Attachment #8518017 - Flags: review?(allstars.chh)
Attached patch Part2. Gecko implementation v2 (obsolete) — Splinter Review
Attachment #8518020 - Flags: review?(allstars.chh)
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,
Attachment #8518017 - Flags: review?(allstars.chh) → review+
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
Summary: B2G NFC: Investigate if DOMRequests should be replaced with Promises → B2G NFC: Replace DOMRequests with Promises
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.
Attachment #8518020 - Flags: review?(allstars.chh) → review-
Attached patch Part2. Gecko implementation v3 (obsolete) — Splinter Review
Attachment #8518020 - Attachment is obsolete: true
Attachment #8519861 - Flags: review?(allstars.chh)
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.
Attachment #8519861 - Flags: review?(allstars.chh) → review+
Attachment #8518017 - Flags: review+ → review?(bugs)
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
Attachment #8518017 - Flags: review?(bugs)
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.
Attached patch Part1. WebIDL change v3 (obsolete) — Splinter Review
Add r? to smaug for webIDL change.
Attachment #8518017 - Attachment is obsolete: true
Attachment #8520548 - Flags: review?(bugs)
Attachment #8512498 - Flags: review?(allstars.chh)
Attachment #8520548 - Flags: review?(bugs) → review+
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.
Attachment #8512498 - Flags: review?(allstars.chh) → review+
Depends on: 1097564
Attached patch Part2. Gecko implementation v4 (obsolete) — Splinter Review
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
Attachment #8519861 - Attachment is obsolete: true
Attachment #8526584 - Flags: review?(allstars.chh)
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.
Attachment #8526584 - Flags: review?(allstars.chh)
Attached patch Part2. Gecko implementation v5 (obsolete) — Splinter Review
Thanks for finding this, update patch to fix this issue
Attachment #8526584 - Attachment is obsolete: true
Attachment #8526596 - Flags: review?(allstars.chh)
Attachment #8526596 - Attachment is obsolete: true
Attachment #8526596 - Flags: review?(allstars.chh)
Attached patch Part2. Gecko implementation v5 (obsolete) — Splinter Review
Fix issue
Attachment #8526600 - Flags: review?(allstars.chh)
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();
Attachment #8526600 - Flags: review?(allstars.chh) → review+
Attached patch bug 996397 patchSplinter Review
Attachment #8526634 - Flags: review+
Attachment #8520548 - Attachment is obsolete: true
Attachment #8526600 - Attachment is obsolete: true
Attachment #8512498 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [p=5]
https://hg.mozilla.org/mozilla-central/rev/5da3b5f8fffe
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.