Closed Bug 879821 Opened 7 years ago Closed 6 years ago

NFC B2G IPC Implementation Using UnixSockets

Categories

(Core :: DOM: Device Interfaces, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: psiddh, Assigned: psiddh)

References

Details

Attachments

(1 file, 4 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.31 (KHTML, like Gecko) Chrome/26.0.1410.64 Safari/537.31

Steps to reproduce:

Added IPC gecko support for NFC


Actual results:

No IPC support for NFC - Refer to bug# 
https://bugzilla.mozilla.org/show_bug.cgi?id=674741 , Comment# 121


Expected results:

NFC Gecko IPC should use underlying UnixSocket implementation on the lines of bluetooth and RIL
Blocks: b2g-nfc
OS: Windows 7 → Linux
Attachment #758622 - Flags: review?(kyle)
Assignee: nobody → psiddh
Comment on attachment 758622 [details] [diff] [review]
Patch 1 (v1) - NFC B2G IPC Implementation

Review of attachment 758622 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome! r=me, just go ahead and fix up nits.

BTW, have you tested the network stuff? Last I had checked, RIL network testing was broken, and I know you were using it as your template.

::: ipc/nfc/Nfc.cpp
@@ +31,5 @@
> +const char* NFC_SOCKET_NAME = "/dev/socket/nfcd";
> +
> +// Network port to connect to for adb forwarded sockets when doing
> +// desktop development.
> +const uint32_t NFCD_TEST_PORT = 6300;

Nit: Might want to pick a different port than RIL.

@@ +36,5 @@
> +
> +class DispatchNfcEvent : public WorkerTask
> +{
> +public:
> +  DispatchNfcEvent(UnixSocketRawData* aMessage)

Nit: Looks like there's some indentation issues here?

@@ +46,5 @@
> +};
> +
> +bool
> +DispatchNfcEvent::RunTask(JSContext *aCx)
> +{

Nit: Might want to assert your context is valid.

@@ +61,5 @@
> +
> +class NfcConnector : public mozilla::ipc::UnixSocketConnector
> +{
> +public:
> +  virtual ~NfcConnector()

Nit: Looks like there's some indentation issues here?

@@ +127,5 @@
> +    struct sockaddr_in addr_in;
> +
> +    hp = gethostbyname("localhost");
> +    if (!hp) {
> +    memset(&addr_in, 0, sizeof(addr_in));

Nit: Indentation issues.

@@ +175,5 @@
> +void
> +NfcConsumer::ReceiveSocketData(nsAutoPtr<UnixSocketRawData>& aMessage)
> +{
> +    MOZ_ASSERT(NS_IsMainThread());
> +    LOG("ReceiveSocketData\n");

Nit: This should probably only run during debug? Here and LOG commands below.
Attachment #758622 - Flags: review?(kyle) → review+
v2 patch takes care of Kyle's comments + Re-based changes to mozillaorg code base as on 06/04
Attachment #758622 - Attachment is obsolete: true
v3 patch = v2 + Indentation in Nfc.h file
Attachment #759475 - Attachment is obsolete: true
Attachment #759480 - Flags: review?(kyle)
Comment on attachment 759480 [details] [diff] [review]
Patch 1 (v3) - NFC B2G IPC Implementation

Review of attachment 759480 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/nfc/Nfc.cpp
@@ +36,5 @@
> +
> +class DispatchNfcEvent : public WorkerTask
> +{
> +public:
> +    DispatchRILEvent(UnixSocketRawData* aMessage)

Um, don't you mean DisplayNfcEvent?

@@ +48,5 @@
> +};
> +
> +bool
> +DispatchNfcEvent::RunTask(JSContext *aCx)
> +{

Which thread should this run on? Can you assert for it?
Attachment #759480 - Flags: review?(kyle) → review-
Comment on attachment 759480 [details] [diff] [review]
Patch 1 (v3) - NFC B2G IPC Implementation

Review of attachment 759480 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/nfc/Nfc.cpp
@@ +36,5 @@
> +
> +class DispatchNfcEvent : public WorkerTask
> +{
> +public:
> +    DispatchRILEvent(UnixSocketRawData* aMessage)

Oops. DispatchNfcEvent.

@@ +48,5 @@
> +};
> +
> +bool
> +DispatchNfcEvent::RunTask(JSContext *aCx)
> +{

Which thread should this run on? Can you assert for it?
v4 = v3 + latest Kyle's comments
Attachment #759480 - Attachment is obsolete: true
Attachment #763036 - Flags: review?(kyle)
Comment on attachment 763036 [details] [diff] [review]
Patch 1 (v4) - NFC B2G IPC Implementation

Malformed patch. You've got an extra newline in there.
Attachment #763036 - Flags: review?(kyle) → review-
Oops sorry for the earlier patch, attaching the new patch for review
v5 = Corrected malformed v4 patch
Attachment #763036 - Attachment is obsolete: true
Attachment #763669 - Flags: review?(kyle)
Comment on attachment 763669 [details] [diff] [review]
Patch 1 (v5) - Nfc Gecko IPC Implementation

Review of attachment 763669 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I'm going to run it through a cross platform try with MOZ_B2G_NFC defaulted to on just to make sure we don't get any surprises when that becomes the true case, then I'll land.
Attachment #763669 - Flags: review?(kyle) → review+
try: -b do -p all -u none -t none
https://hg.mozilla.org/mozilla-central/rev/80ebe45d4652
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
I see this patch introduced JSAPI usage without proper review. Please find someone to rectify that.
You need to log in before you can comment on or make changes to this bug.