1047233 - B2G NFC: handler the errorMsg in NfcService and list errorMsg in WebIDL.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Apr. 10]

Right now the error message are all listed in nfc_consts.js, we should list the possible error message in the WebIDL.

Comment 1

[Dimi Lee [:dimi]]

Attachment: WIP Patch

Comment 2

[Dimi Lee [:dimi]]

Attachment: Add NFC Error code in WebIDL patch v1

This is based on modification of Bug 1083651

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Apr. 10]

Comment on attachment 8506762 [details] [diff] [review]
Add NFC Error code in WebIDL patch v1

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

use 'enum'.

Comment 4

[Dimi Lee [:dimi]]

Attachment: Patch v2

Change error code to enum

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Apr. 10]

Comment on attachment 8508534 [details] [diff] [review]
Patch v2

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

Should we also modify NfcService or NfcMessageHandler so it will report errorMsg to Nfc.js directly?
In that case Nfc.js doesn't have to const those error code nor error message.

::: dom/webidl/MozNFC.webidl
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>   /* Copyright © 2013 Deutsche Telekom, Inc. */
>  
> +enum NfcErrorCode {

NfcErrorMessage

@@ +8,5 @@
> +   "success",
> +   "ioerror",
> +   "timeout",
> +   "busy",
> +   "errorconnect",

use either
errorConnect 
or 
ErrorConnect 
will be easier to understand.

Comment 6

[Dimi Lee [:dimi]]

Attachment: patch v3

Comment 7

[Dimi Lee [:dimi]]

Comment on attachment 8508588 [details] [diff] [review]
patch v3

Remove review? for now because I will upload another patch with using WebIDL error message in gecko

Comment 8

[Dimi Lee [:dimi]]

Attachment: pull request to mozilla-b2g/platform_system_nfcd

1. Remove bad session id from error code because this is gecko error.
2. Also change SUCCESS to -1 because WebIDL enum error array only contain error string

Comment 9

[Dimi Lee [:dimi]]

Attachment: Part1. Support error code

Comment 10

[Dimi Lee [:dimi]]

Attachment: Part2. Error Code WEBIDL

Comment 11

[Dimi Lee [:dimi]]

Attachment: Part1. WebIDL change v2

Comment 12

[Dimi Lee [:dimi]]

Attachment: Part2. Support error code

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Apr. 10]

Comment on attachment 8513208 [details] [diff] [review]
Part1. WebIDL change v2

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

::: dom/webidl/MozNFC.webidl
@@ +4,5 @@
>  
>   /* Copyright © 2013 Deutsche Telekom, Inc. */
>  
> +enum NfcErrorMessage {
> +    "IOError",

fix indent to 2 spaces.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Apr. 10]

Comment on attachment 8513209 [details] [diff] [review]
Part2. Support error code

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

::: dom/nfc/gonk/NfcMessageHandler.cpp
@@ +36,5 @@
>  static const char* kTechLostNotification = "TechLostNotification";
>  static const char* kHCIEventTransactionNotification =
>                       "HCIEventTransactionNotification";
>  
> +static const int kErrorCodeToErrorStringOffset = 1;

could be just kErrorMsgOffset.

@@ +178,5 @@
>  bool
>  NfcMessageHandler::ReadNDEFResponse(const Parcel& aParcel, EventOptions& aOptions)
>  {
> +  int errorCode = aParcel.readInt32();
> +  int seesionId = aParcel.readInt32();

sessionId

::: dom/nfc/gonk/NfcMessageHandler.h
@@ +42,5 @@
>    bool ReadNDEFMessage(const android::Parcel& aParcel, EventOptions& aOptions);
>    bool WriteNDEFMessage(android::Parcel& aParcel, const CommandOptions& aOptions);
>  
>  private:
> +  nsString ConvertErrCodetoErrMsg(int aErrCode);

CovertToErrorMsg is enough

Comment 15

[Dimi Lee [:dimi]]

Comment on attachment 8513208 [details] [diff] [review]
Part1. WebIDL change v2

Add r? to smug for adding NFC error code to webIDL.

Indent nits mentioned by yoshi will be fixed in next patch.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8513208 [details] [diff] [review]
Part1. WebIDL change v2

So we add enum NfcErrorMessage but don't use it anywhere?

Should NfcEventOptions.errorMsg use type NfcErrorMessage ?
And if you need "" to indicate no error, just add it to the enum.

Comment 17

[Dimi Lee [:dimi]]

Attachment: Part1. WebIDL change v3

This patch address the following comment
1. use "" to indicate success
2. use type NfcErrorMessage for NfcEventOptions.errorMsg

Because we are moving NFC API from certified to privileged, so these error codes are for future use, no one is using it now.

Comment 18

[Dimi Lee [:dimi]]

Attachment: Part2. Support error code v2

Hi Yoshi,
The implementation is changed because of WebIDL according to Comment 16.
Could you help review again ? Thanks !

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Apr. 10]

Comment on attachment 8515797 [details] [diff] [review]
Part2. Support error code v2

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

Update the patch subject.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8515794 [details] [diff] [review]
Part1. WebIDL change v3

What is the status used for if we now just set errorMsg (based on part 2).

Comment 21

[Dimi Lee [:dimi]]

(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 8515794 [details] [diff] [review]
> Part1. WebIDL change v3
> 
> What is the status used for if we now just set errorMsg (based on part 2).

Originally |status| is used for both initialize notification callback from nfc daemon and error status for different request.
Now we move error status to errorMsg and keep using |status| for initialize notification.
http://dxr.mozilla.org/mozilla-central/source/dom/nfc/gonk/NfcMessageHandler.cpp#234

Comment 22

[Dimi Lee [:dimi]]

try result:
https://tbpl.mozilla.org/?tree=Try&rev=02d4d7544ae8

Comment 23

[Dimi Lee [:dimi]]

Attachment: Rebased patch v1

Comment 24

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Apr. 10]

https://hg.mozilla.org/integration/b2g-inbound/rev/6c9d06d89be1

Comment 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Apr. 10]

ni? myself to land the nfcd patch.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c9d06d89be1

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Apr. 10]

https://github.com/mozilla-b2g/platform_system_nfcd/commit/2cb1db1f6dccecbd4a458f8115c1fdc68dfb9365