1105666 - [BrowserAPI] Add an API to indicate this iframe could receive NFC event.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

See Bug 1082440 for the background.

When we tapping a NFC tag or a NFC device, an ontagfound/peerfound will be notified.

However we don't want all apps could receive these callbacks, for example, we don't want a background app could receive these callbacks, otherwise the app could share some data to the other tag/device without user perception.

So we'd like to add a Browser API to indicate whether the iframe should receive NFC event or not.
And this API shall be ONLY used by System app.

Currently the setVisible is going to be updated in Bug 1034001, and I've asked Kanru, he thinks adding another attribute 'nfc' may NOT be a good idea, so maybe a seperate API should be created, maybe setNFCFocus(bool)

Comment 1

[Dimi Lee [:dimi]]

Attachment: Part1. Add setNfcFocusApp BrowserAPI v1

Comment 2

[Dimi Lee [:dimi]]

Attachment: Part2. Add id attribute to nsITabChild interfac v1

Comment 3

[Dimi Lee [:dimi]]

Attachment: Part3. NFC implemenetation

This patch doesn't include use browserId to notify correct foreground app

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8536482 [details] [diff] [review]
Part3. NFC implemenetation

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

I am okay with most changes.
However we should try to avoid using 'in' as it could hurt our performace.
Please try not to use 'in' op in Nfc.js.
I guess for-in also have the similar situation.

See
http://jsperf.com/in-vs-not-undefined

Comment 5

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

nominate feature2.2? for it's blocking bug 1082440

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8536479 [details] [diff] [review]
Part1. Add setNfcFocusApp BrowserAPI v1

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

What will happen when <iframe> is in-process? Should NFC still work in this case?

::: dom/html/nsBrowserElement.cpp
@@ +554,5 @@
> +    return;
> +  }
> +
> +  PBrowserParent* remoteBrowser = frameLoader->GetRemoteBrowser();
> +  TabParent* remote = static_cast<TabParent*>(remoteBrowser);

Use |TabParent* remote = TabParent::GetFrom(frameLoader);|

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8536480 [details] [diff] [review]
Part2. Add id attribute to nsITabChild interfac v1

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

::: dom/interfaces/base/nsITabChild.idl
@@ +22,5 @@
>    [noscript, notxpcom] void enableDisableCommands(in AString action,
>                                                    in CommandsArrayRef enabledCommands,
>                                                    in CommandsArrayRef disabledCommands);
> +
> +  readonly attribute uint64_t id;

readonly attribute uint64_t tabId;

::: dom/ipc/TabChild.cpp
@@ +3298,5 @@
> +
> +NS_IMETHODIMP
> +TabChild::GetId(uint64_t* aId)
> +{
> +  *aId = mUniqueId;

*aId = GetTabId();

Comment 8

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Setting target milestone to sprint3 which is before branch date.
Feel free to revisit if there's concern.

Comment 9

[Dimi Lee [:dimi]]

(In reply to [VAC ~Jan 05] Kan-Ru Chen [:kanru] from comment #6)
> Comment on attachment 8536479 [details] [diff] [review]
> Part1. Add setNfcFocusApp BrowserAPI v1
> 
> Review of attachment 8536479 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What will happen when <iframe> is in-process? Should NFC still work in this
> case?
> 
NFC should still work no matter <iframe> is in-process mode or not.

According to offline discussion result with kanru about this issue. Because we
will not create app in in-process mode for b2g now, I will still use current
approach. Bug 1116449 is created to track this issue, also comments and warning
messages will be added in next patch.

Comment 10

[Dimi Lee [:dimi]]

Attachment: Part1. Add setNfcFocusApp BrowserAPI v2

Change in this patch
- Add comments, warning messages about issue mentioned in Comment 6, Comment 9
- Change naming from browserId to tabId
- Add error check for function setNFCFocusApp in BrowserElementParent.js

Comment 11

[Dimi Lee [:dimi]]

Attachment: Part2. Add tabId attribute to nsITabChild interfac v2

Change in this patch
- Address comment 7

Comment 12

[Dimi Lee [:dimi]]

Attachment: Part3. NFC implemenetation v2

Change in this patch
- Address comment 4
- Change naming from browserId to tabId
- Add comment and error checking for issue mentioned in Comment 6, Comment 9 (NfcContentHelper.js)

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8542869 [details] [diff] [review]
Part1. Add setNfcFocusApp BrowserAPI v2

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

wrong patch.

Comment 14

[Dimi Lee [:dimi]]

Attachment: Part1. Add setNfcFocusApp BrowserAPI v3

I upload load wring patch, update it.
Please see Comment 10 for patch change

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8542871 [details] [diff] [review]
Part3. NFC implemenetation v2

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

::: dom/nfc/NfcContentHelper.js
@@ +124,5 @@
> +    try {
> +      this._tabId = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                            .getInterface(Ci.nsITabChild)
> +                            .tabId;
> +    } catch(e) {

Don't use try-catch to check this.

If the object doesn't inherit that interface returning null should be a desired behavior.

::: dom/nfc/gonk/Nfc.js
@@ +76,5 @@
>      messages: ["NFC:CheckP2PRegistration",
>                 "NFC:NotifyUserAcceptedP2P",
>                 "NFC:NotifySendFileStatus",
> +               "NFC:ChangeRFState",
> +               "NFC:SetFocusApp"] }

I don't see WebIDL has check 'nfc-manager' permission for this.

Comment 16

[Dimi Lee [:dimi]]

(In reply to Yoshi Huang[:allstars.chh] from comment #15)
> Comment on attachment 8542871 [details] [diff] [review]
> Part3. NFC implemenetation v2
> 
> Review of attachment 8542871 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/nfc/NfcContentHelper.js
> @@ +124,5 @@
> > +    try {
> > +      this._tabId = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                            .getInterface(Ci.nsITabChild)
> > +                            .tabId;
> > +    } catch(e) {
> 
> Don't use try-catch to check this.
> 
> If the object doesn't inherit that interface returning null should be a
> desired behavior.

I use catch because if there is no interface, Ci.nsIInterfaceRequestor will throw NS_ERROR_NO_INTERFACE
instead of return null.

> 
> ::: dom/nfc/gonk/Nfc.js
> @@ +76,5 @@
> >      messages: ["NFC:CheckP2PRegistration",
> >                 "NFC:NotifyUserAcceptedP2P",
> >                 "NFC:NotifySendFileStatus",
> > +               "NFC:ChangeRFState",
> > +               "NFC:SetFocusApp"] }
> 
> I don't see WebIDL has check 'nfc-manager' permission for this.
I will add nfc-manager permission in Part1 WebIDL change.

Is that ok for you ?

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #16)
> (In reply to Yoshi Huang[:allstars.chh] from comment #15)
> > Comment on attachment 8542871 [details] [diff] [review]
> > Part3. NFC implemenetation v2
> > 
> > Review of attachment 8542871 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/nfc/NfcContentHelper.js
> > @@ +124,5 @@
> > > +    try {
> > > +      this._tabId = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> > > +                            .getInterface(Ci.nsITabChild)
> > > +                            .tabId;
> > > +    } catch(e) {
> > 
> > Don't use try-catch to check this.
> > 
> > If the object doesn't inherit that interface returning null should be a
> > desired behavior.
> 
> I use catch because if there is no interface, Ci.nsIInterfaceRequestor will
> throw NS_ERROR_NO_INTERFACE
> instead of return null.
> 
Ah, okay.
> > 
> > ::: dom/nfc/gonk/Nfc.js
> > @@ +76,5 @@
> > >      messages: ["NFC:CheckP2PRegistration",
> > >                 "NFC:NotifyUserAcceptedP2P",
> > >                 "NFC:NotifySendFileStatus",
> > > +               "NFC:ChangeRFState",
> > > +               "NFC:SetFocusApp"] }
> > 
> > I don't see WebIDL has check 'nfc-manager' permission for this.
> I will add nfc-manager permission in Part1 WebIDL change.
> 
> Is that ok for you ?
okay,
but ff you're not sure whether we should use nfc-manager permission for now then you shouldn't check it in parent side.

Comment 18

[Dimi Lee [:dimi]]

(In reply to Yoshi Huang[:allstars.chh] from comment #17)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #16)
> > (In reply to Yoshi Huang[:allstars.chh] from comment #15)
> > > 
> > > ::: dom/nfc/gonk/Nfc.js
> > > @@ +76,5 @@
> > > >      messages: ["NFC:CheckP2PRegistration",
> > > >                 "NFC:NotifyUserAcceptedP2P",
> > > >                 "NFC:NotifySendFileStatus",
> > > > +               "NFC:ChangeRFState",
> > > > +               "NFC:SetFocusApp"] }
> > > 
> > > I don't see WebIDL has check 'nfc-manager' permission for this.
> > I will add nfc-manager permission in Part1 WebIDL change.
> > 
> > Is that ok for you ?
> okay,
> but ff you're not sure whether we should use nfc-manager permission for now
> then you shouldn't check it in parent side.

I think we will need nfc-manager permission check, so i will add this in WebIDL part and remain check it in gecko implementation

Comment 19

[Dimi Lee [:dimi]]

Attachment: Part1. Add setNfcFocusApp BrowserAPI v4

Comment 20

[Dimi Lee [:dimi]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #19)
> Created attachment 8543906 [details] [diff] [review]
> Part1. Add setNfcFocusApp BrowserAPI v4

This patch add |nfc-manager| permission check for setNFCFocusApp browser API

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8542871 [details] [diff] [review]
Part3. NFC implemenetation v2

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

::: dom/nfc/NfcContentHelper.js
@@ +135,5 @@
> +      if (inParent) {
> +        this._tabId = -1;
> +      } else {
> +        throw Components.Exception("Can't get tab id in child process",
> +                                    Cr.NS_ERROR_UNEXPECTED);

nit, align with "

::: dom/nfc/gonk/Nfc.js
@@ +189,4 @@
>      },
>  
>      removeEventListener: function removeEventListener(target) {
> +      for (let key in this.eventListeners) {

let id in ...
same for below.

Comment 22

[Dimi Lee [:dimi]]

Attachment: Part3. NFC implemenetation v3

Fix nits

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8543906 [details] [diff] [review]
Part1. Add setNfcFocusApp BrowserAPI v4

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

Sorry, I looked too fast. The CheckPermissions="browser nfc-manager" does not mean what you wanted. It means "browser" OR "nfc-manager". If you want to check nfc-manager permission please check it in code.

Comment 24

[Dimi Lee [:dimi]]

Attachment: Part1. Add setNfcFocusApp BrowserAPI v5

Comment 25

[Dimi Lee [:dimi]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #24)
> Created attachment 8543944 [details] [diff] [review]
> Part1. Add setNfcFocusApp BrowserAPI v5

Fix comment mentioned in Comment 23

Comment 26

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

Comment on attachment 8543944 [details] [diff] [review]
Part1. Add setNfcFocusApp BrowserAPI v5

Sounds like we should have tabid for all the tabs, not only remote tabs.
I wonder why it is for remote only.

We really need to get bug 1116449 fixed too.
(I think tabid should be kept in nsFrameLoader so that in-process tabids would be easier to handle)

Comment 27

[Dimi Lee [:dimi]]

As discussed with alive in bug 1117633, GAIA will still need a way to clear current focus app, ex. When APP is in transition mode, GAIA could not determine which app is in foreground in this case.

So i will add a boolean parameter to let GAIA could clear current focus app.

Comment 28

[Dimi Lee [:dimi]]

Attachment: Part1. Add setNfcFocusApp BrowserAPI v6

Hi Kanru, smug,
Sorry because of after discussing with alive in bug 1117633, there are cases that there is no "foreground" app, ex. when app is in transition mode.

So we decide to add a boolean value for GAIA to clear focus, in this case NFC module will not notify event to any app except system app (bug 1082443).

Could you help review this again, thanks

Comment 29

[Dimi Lee [:dimi]]

Attachment: Part3. NFC implemenetation v4

Hi Yoshi,
According to comment 28, I also update the NFC gecko implementation with addtional isFocus parameter, please help review again, thanks!

Comment 30

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8544433 [details] [diff] [review]
Part3. NFC implemenetation v4

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

::: dom/nfc/nsINfcContentHelper.idl
@@ +90,5 @@
> +[scriptable, uuid(03292f6b-b931-4454-8718-ae339029c961)]
> +interface nsINfcBrowserAPI : nsISupports
> +{
> +  void setFocusApp(in boolean isFocus,
> +                   in uint64_t tabId);

I think tabId should come first,

setFocus(in uint64_t tabId, in boolean isFoucs);

Comment 31

[Dimi Lee [:dimi]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #28)
> Created attachment 8544430 [details] [diff] [review]
> Part1. Add setNfcFocusApp BrowserAPI v6
> 
> Hi Kanru, smug,
> Sorry because of after discussing with alive in bug 1117633, there are cases
> that there is no "foreground" app, ex. when app is in transition mode.
> 
> So we decide to add a boolean value for GAIA to clear focus, in this case
> NFC module will not notify event to any app except system app (bug 1082443).
> 
> Could you help review this again, thanks

According to Comment 30, i will swich the order of tabId and isFocus parameter defined in nsIBrowserElementAPI.idl

Comment 32

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

Comment on attachment 8544430 [details] [diff] [review]
Part1. Add setNfcFocusApp BrowserAPI v6

>+
>+    if (typeof isFocus !== 'boolean') {
>+      throw Components.Exception("Invalid argument",
>+                                 Cr.NS_ERROR_INVALID_ARG);
>+    }
>+
>+    if (typeof tabId !== 'number') {
>+      throw Components.Exception("Invalid argument",
>+                                 Cr.NS_ERROR_INVALID_ARG);
>+    }
Why you actually need these checks. I would just drop them and rely on xpconnect to do the right thing

>+nsBrowserElement::SetNFCFocusApp(bool aIsFocus,
>+                                 ErrorResult& aRv)
>+{
>+  NS_ENSURE_TRUE_VOID(IsBrowserElementOrThrow(aRv));
>+
>+  nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader();
>+  if (!frameLoader) {
>+    aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
More likely UNEXPECTED_STATE or such. OOM would have killed the process

Comment 33

[Dimi Lee [:dimi]]

Attachment: Part3. NFC implemenetation v5

Change order of tabid and isFocus

Comment 35

[(inactive after 6/18) Alive Kuo [:alive]]

Etienne and me both think the 'App' here is redundant. It's called on mozbrowser iframe instead of the whole app. I don't know if you have chance to change but just post feedback here.

Comment 36

[Dimi Lee [:dimi]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #35)
> Etienne and me both think the 'App' here is redundant. It's called on
> mozbrowser iframe instead of the whole app. I don't know if you have chance
> to change but just post feedback here.

I have already talked with Kanru about this, i will remove the 'App'.

Comment 37

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Deferring to current sprint.

Comment 38

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8544430 [details] [diff] [review]
Part1. Add setNfcFocusApp BrowserAPI v6

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

Clear review per comment 36

Comment 39

[Dimi Lee [:dimi]]

Attachment: Part1. Add setNfcFocusApp BrowserAPI v7

- Change naming from SetNFCFocusApp to SetNFCFocus

Comment 40

[Dimi Lee [:dimi]]

Attachment: Part3. NFC implemenetation v6

Checked with yoshi, remove redundant check function |setFocusApp| in Nfc.js

Comment 41

[Dimi Lee [:dimi]]

I found add 
#include "mozilla/dom/TabParent.h"  to dom/html/nsBrowserElement.cpp
will cause try server windows build fail

Below is try server test result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f7238f3241a

I am not sure the root cause but will keep checking it, if anyone know the reason please let me know. thanks !

Comment 42

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The root cause is that you're including <windows.h> (via TabParent.h) into a bunch of code that can't deal with that.

Comment 43

[Dimi Lee [:dimi]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #42)
> The root cause is that you're including <windows.h> (via TabParent.h) into a
> bunch of code that can't deal with that.

Thanks for help !!!

Comment 44

[Dimi Lee [:dimi]]

Attachment: Part1. Add setNfcFocusApp BrowserAPI v8

Hi Kanru,
Sorry I update patch again because of bug mentioned in Comment 41, and the root cause is what kyle addressed in Comment42.

Originally I tried to move those header files causing this problem from TabParent.h to TabParent.cpp. But there are too many of them(PBrowserParent.h, TabContext.h ... etc) and some header files have to be included in TabParent.h

So the solution is do not include TabParent.h in nsBrowserElement.cpp, so I move get tabId related code from nsBrowserElement.cpp to BrowserElementParent.js.

Following are new changes in this patch:
1. Remove code that get tabId in nsBrowserElement.cpp
2. Add tabId attribute to nsITabParent.idl and implementation in TabParent.cpp
3. Get tab id in BrowserElementParent.js
4. Update nsIBrowserElementAPI.idl to remove tabId parameter for setNFCFcous API

Comment 45

[Dimi Lee [:dimi]]

Attachment: SetNfcFocusApp BrowserAPI patch

Comment 46

[Dimi Lee [:dimi]]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90998f1e54cf

Comment 47

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bccb6cedee61

Comment 49

[Dimi Lee [:dimi]]

Comment on attachment 8552986 [details] [diff] [review]
SetNfcFocusApp BrowserAPI patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
NFC feature

User impact if declined: 
Third party app will get nfc notification even it is in background

Testing completed: 
Manually with patch provided in bug 1117633

Risk to taking this patch (and alternatives if risky): 
No.

String or UUID changes made by this patch:
1.[nsIBrowserElementAPI.idl] interface nsIBrowserElementAPI
uuid(abae4fb1-7d6f-4e3f-b435-6501f1d4c659) to
uuid(3811446f-90bb-42c1-b2b6-aae3603b61e1)

2.[nsITabChild.idl] interface nsITabChild
uuid(7227bac4-b6fe-4090-aeb4-bc288b790925) to
uuid(1fb79c27-e760-4088-b19c-1ce3673ec24e)

3.[nsITabParent.idl] interface nsITabParent
uuid(33e8571d-5e5d-4000-81cf-e01f5f85d424) to
uuid(30361a5b-a3b8-4dbc-b464-e08761abb123)

4.[nsINfcContentHelper.idl] interface nsINfcBrowserAPI
Add uuid(7ae46728-bc42-44bd-8093-bc7563abf52d)

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f8103a51ba3c

Comment 51

[Chris Mills [:cmills]]

I've added this method to MDN:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/setNfcFocus

It is also covered in the Firefox OS 2.2 release notes:

https://developer.mozilla.org/en-US/Firefox_OS/Releases/2.2#Web_API_changes