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

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
5 years ago
a month ago

People

(Reporter: allstars.chh, Assigned: dimi)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
2.2 S4 (23jan)
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment, 16 obsolete attachments)

22.55 KB, patch
dimi
: review+
Details | Diff | Splinter Review
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)
(Assignee)

Updated

4 years ago
Assignee: nobody → dlee
(Assignee)

Comment 1

4 years ago
Attachment #8536479 - Flags: review?(kchen)
(Assignee)

Comment 2

4 years ago
Attachment #8536480 - Flags: review?(kchen)
(Assignee)

Updated

4 years ago
Attachment #8536479 - Attachment description: [Part1] Add setNfcFocusApp BrowserAPI v1 → Part1. Add setNfcFocusApp BrowserAPI v1
(Assignee)

Comment 3

4 years ago
Posted patch Part3. NFC implemenetation (obsolete) — Splinter Review
This patch doesn't include use browserId to notify correct foreground app
Attachment #8536482 - Flags: review?(allstars.chh)
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
Attachment #8536482 - Flags: review?(allstars.chh)
nominate feature2.2? for it's blocking bug 1082440
feature-b2g: --- → 2.2?
feature-b2g: 2.2? → 2.2+

Updated

4 years ago
Status: NEW → ASSIGNED
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);|
Attachment #8536479 - Flags: review?(kchen) → feedback+
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();
Attachment #8536480 - Flags: review?(kchen) → review+
Setting target milestone to sprint3 which is before branch date.
Feel free to revisit if there's concern.
Target Milestone: --- → 2.2 S3 (9jan)
(Assignee)

Updated

4 years ago
Blocks: 1116449
(Assignee)

Comment 9

4 years ago
(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.
(Assignee)

Comment 10

4 years ago
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
Attachment #8536479 - Attachment is obsolete: true
Attachment #8542869 - Flags: review?(kchen)
(Assignee)

Comment 11

4 years ago
Change in this patch
- Address comment 7
Attachment #8536480 - Attachment is obsolete: true
Attachment #8542870 - Flags: review+
(Assignee)

Comment 12

4 years ago
Posted patch Part3. NFC implemenetation v2 (obsolete) — Splinter Review
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)
Attachment #8536482 - Attachment is obsolete: true
Attachment #8542871 - Flags: review?(allstars.chh)
Comment on attachment 8542869 [details] [diff] [review]
Part1. Add setNfcFocusApp BrowserAPI v2

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

wrong patch.
Attachment #8542869 - Flags: review?(kchen)
(Assignee)

Comment 14

4 years ago
I upload load wring patch, update it.
Please see Comment 10 for patch change
Attachment #8542869 - Attachment is obsolete: true
Attachment #8543732 - Flags: review?(kchen)
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.
Attachment #8542871 - Flags: review?(allstars.chh)
(Assignee)

Updated

4 years ago
Blocks: 1117633
(Assignee)

Comment 16

4 years ago
(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 ?
Flags: needinfo?(allstars.chh)
(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.
Flags: needinfo?(allstars.chh)
(Assignee)

Comment 18

4 years ago
(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
(Assignee)

Updated

4 years ago
Attachment #8542871 - Flags: review?(allstars.chh)
(Assignee)

Comment 19

4 years ago
Attachment #8543906 - Flags: review?(kchen)
(Assignee)

Updated

4 years ago
Attachment #8543732 - Attachment is obsolete: true
Attachment #8543732 - Flags: review?(kchen)
(Assignee)

Comment 20

4 years ago
(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
Attachment #8543906 - Flags: review?(kchen) → review+
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.
Attachment #8542871 - Flags: review?(allstars.chh) → review+
(Assignee)

Comment 22

4 years ago
Posted patch Part3. NFC implemenetation v3 (obsolete) — Splinter Review
Fix nits
Attachment #8542871 - Attachment is obsolete: true
Attachment #8543938 - Flags: review+
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.
Attachment #8543906 - Flags: review+ → review-
(Assignee)

Comment 24

4 years ago
Attachment #8543944 - Flags: review?(kchen)
(Assignee)

Updated

4 years ago
Attachment #8543906 - Attachment is obsolete: true
(Assignee)

Comment 25

4 years ago
(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
(Assignee)

Updated

4 years ago
Attachment #8543944 - Flags: review?(bugs)
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)
Attachment #8543944 - Flags: review?(bugs) → review+
Attachment #8543944 - Flags: review?(kchen) → review+
(Assignee)

Comment 27

4 years ago
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.
(Assignee)

Comment 28

4 years ago
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
Attachment #8543944 - Attachment is obsolete: true
Attachment #8544430 - Flags: review?(kchen)
Attachment #8544430 - Flags: review?(bugs)
(Assignee)

Comment 29

4 years ago
Posted patch Part3. NFC implemenetation v4 (obsolete) — Splinter Review
Hi Yoshi,
According to comment 28, I also update the NFC gecko implementation with addtional isFocus parameter, please help review again, thanks!
Attachment #8543938 - Attachment is obsolete: true
Attachment #8544433 - Flags: review?(allstars.chh)
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);
Attachment #8544433 - Flags: review?(allstars.chh)
(Assignee)

Comment 31

4 years ago
(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 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
Attachment #8544430 - Flags: review?(bugs) → review+
(Assignee)

Comment 33

4 years ago
Posted patch Part3. NFC implemenetation v5 (obsolete) — Splinter Review
Change order of tabid and isFocus
Attachment #8544433 - Attachment is obsolete: true
Attachment #8545060 - Flags: review?(allstars.chh)
Duplicate of this bug: 1065307
Attachment #8545060 - Flags: review?(allstars.chh) → review+
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.
(Assignee)

Comment 36

4 years ago
(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'.
Deferring to current sprint.
Target Milestone: 2.2 S3 (9jan) → 2.2 S4 (23jan)
Comment on attachment 8544430 [details] [diff] [review]
Part1. Add setNfcFocusApp BrowserAPI v6

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

Clear review per comment 36
Attachment #8544430 - Flags: review?(kchen)
(Assignee)

Comment 39

4 years ago
- Change naming from SetNFCFocusApp to SetNFCFocus
Attachment #8544430 - Attachment is obsolete: true
Attachment #8548009 - Flags: review?(kchen)
(Assignee)

Comment 40

4 years ago
Posted patch Part3. NFC implemenetation v6 (obsolete) — Splinter Review
Checked with yoshi, remove redundant check function |setFocusApp| in Nfc.js
Attachment #8545060 - Attachment is obsolete: true
Attachment #8548012 - Flags: review+
Attachment #8548009 - Flags: review?(kchen) → review+
(Assignee)

Comment 41

4 years ago
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 !
The root cause is that you're including <windows.h> (via TabParent.h) into a bunch of code that can't deal with that.
(Assignee)

Comment 43

4 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 !!!
(Assignee)

Comment 44

4 years ago
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
Attachment #8548009 - Attachment is obsolete: true
Attachment #8552393 - Flags: review?(kchen)
Attachment #8552393 - Flags: review?(kchen) → review+
(Assignee)

Comment 45

4 years ago
Attachment #8542870 - Attachment is obsolete: true
Attachment #8548012 - Attachment is obsolete: true
Attachment #8552393 - Attachment is obsolete: true
Attachment #8552986 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bccb6cedee61
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 49

4 years ago
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)
Attachment #8552986 - Flags: approval-mozilla-b2g37?
Attachment #8552986 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
(Assignee)

Updated

4 years ago
Blocks: 1131964
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.