Closed
Bug 879821
Opened 11 years ago
Closed 11 years ago
NFC B2G IPC Implementation Using UnixSockets
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: psiddh, Assigned: psiddh)
References
Details
Attachments
(1 file, 4 obsolete files)
8.38 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
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
Attachment #758622 -
Flags: review?(kyle)
Updated•11 years ago
|
Assignee: nobody → psiddh
Comment 2•11 years ago
|
||
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 5•11 years ago
|
||
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 6•11 years ago
|
||
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 8•11 years ago
|
||
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
Assignee | ||
Comment 10•11 years ago
|
||
v5 = Corrected malformed v4 patch
Attachment #763036 -
Attachment is obsolete: true
Attachment #763669 -
Flags: review?(kyle)
Comment 11•11 years ago
|
||
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+
Comment 12•11 years ago
|
||
try: -b do -p all -u none -t none
Comment 13•11 years ago
|
||
Ok, pasted the wrong thing... https://tbpl.mozilla.org/?tree=Try&rev=184a42023573
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/80ebe45d4652
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 15•11 years ago
|
||
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.
Description
•