Integrate Shrinking UI with NfcManager (System App)

RESOLVED FIXED

Status

Firefox OS
NFC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: psiddh, Assigned: psiddh)

Tracking

unspecified
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/30.0.1599.114 Chrome/30.0.1599.114 Safari/537.36

Steps to reproduce:

Integrate with Shrinking UI at the system application level
(Assignee)

Updated

4 years ago
Depends on: 860910
(Assignee)

Comment 1

4 years ago
Created attachment 8338229 [details] [diff] [review]
(v1) Bug 943179 : Integrate P2P Shrink UI in nfc_manager. r=alive

Integrate Shrinking UI functionality to NfcManager
Attachment #8338229 - Flags: review?(alive)
Comment on attachment 8338229 [details] [diff] [review]
(v1) Bug 943179 : Integrate P2P Shrink UI in nfc_manager. r=alive

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

::: apps/system/js/nfc_manager.js
@@ +167,5 @@
>        this.handleTechLost.bind(this));
>      window.addEventListener('screenchange', this);
>      window.addEventListener('lock', this);
>      window.addEventListener('unlock', this);
> +    window.addEventListener('shrinking-sent', handleP2PUserFeedback);

window.addEventListener('shrinking-sent', this);

@@ +172,2 @@
>      var self = this;
> +    function handleP2PUserFeedback(e) {

Put this inside NFCManager instead of a inner function here.

@@ +336,5 @@
>      var nfcdom = window.navigator.mozNfc;
>  
> +    // Get the top application's manifest Url and check for valid P2P
> +    // registration. If registered, shrink the UI
> +    var currentActiveApp = WindowManager.getCurrentActiveAppWindow();

I dislike this. Do not call WindowManager directly.
Send an event and let WindowManager do the followup.

@@ +361,5 @@
> +  dispatchP2PUIStart: function nm_dispatchP2PUIStart() {
> +    var currentAppManifestUrl =
> +      WindowManager.getCurrentActiveAppWindow().manifestURL;
> +    var detail = { manifestUrl: currentAppManifestUrl };
> +    var evt = new CustomEvent('nfc-p2p-user-accept', {

Who is consumer of this event?
Attachment #8338229 - Flags: review?(alive)

Updated

4 years ago
Blocks: 903305
(Assignee)

Comment 3

4 years ago
Created attachment 8338293 [details] [diff] [review]
(v2) Bug 943179 : Integrate P2P Shrink UI in nfc_manager. r=alive
Attachment #8338229 - Attachment is obsolete: true
Attachment #8338293 - Flags: review?(alive)
(Assignee)

Comment 4

4 years ago
Hi Alive, Thanks for your comments

> window.addEventListener('shrinking-sent', this);
     and
> Put this inside NFCManager instead of a inner function here.
Fixed this in v2 of the patch

> +    var currentActiveApp = WindowManager.getCurrentActiveAppWindow();
> I dislike this. Do not call WindowManager directly.
> Send an event and let WindowManager do the followup.
Ok. I actually copied this from 'activity_window_factory.js'.
So do you think WindowManager should send the event to nfc_manager.js during Shrink UI? Please suggest
Do we already have support this event in 'WindowManager' ? Otherwise should we raise a follow up bug ?

> +    var evt = new CustomEvent('nfc-p2p-user-accept', {
> Who is consumer of this event?

Consumer is actually the Nfc DOM code. Please look at this bugzilla changes as well
https://bug933136.bugzilla.mozilla.org/attachment.cgi?id=8338067
'this._window.addEventListener("nfc-p2p-user-accept", function (event) { .... '
(In reply to Siddartha P from comment #4)
> Ok. I actually copied this from 'activity_window_factory.js'.
> So do you think WindowManager should send the event to nfc_manager.js during
> Shrink UI? Please suggest
> Do we already have support this event in 'WindowManager' ? Otherwise should
> we raise a follow up bug ?

Proposal:
NFCManager = {
  checkPermissionThenStartNFC: function nm_check(manifestURL) {
     var status = nfcdom.checkP2PRegistration(manifestURL);
     var self = this;
     status.onsuccess = function() {
       // Top visible application's manifest Url is registered;
       // Do P2P UI and wait for user to accept P2P event
       self.dispatchP2PUIAction('shrinking-start');
     };
     status.onerror = function() {
       // Do nothing!
       self._debug('Top visible application manifest Url is NOT registered');
     };
  }
}
window.dispatchEvent(new CustomEvent('request-nfc-permission-check-to-active-app', { detail: this }));

// WINDOW MANAGER
window.addEventListener('request-nfc-permission-check-to-active-app', function(e) {
  e.detail.checkPermissionThenStartNFC(runningApps[displayedApp].manifestURL);
});
Comment on attachment 8338293 [details] [diff] [review]
(v2) Bug 943179 : Integrate P2P Shrink UI in nfc_manager. r=alive

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

Please use github PR.
Attachment #8338293 - Flags: review?(alive)
Maybe I should not write the codes directly but describe the idea ;)

1. Let NFCManager sends a custom event accompanying with itself or the callback function(need to rebind this)
2. WindowManager OR ShrinkUI who knows who is the active App would consume the event and call the function call on the active appWindow with its manifestURL.

Comment 8

4 years ago
I think this one should be a 1.3+ bug. Ken, what do you think?
blocking-b2g: --- → 1.3?
Flags: needinfo?(kchang)

Comment 9

4 years ago
(In reply to Kevin Hu [:khu] from comment #8)
> I think this one should be a 1.3+ bug. Ken, what do you think?
It definitely is....
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(kchang)
(Assignee)

Comment 10

4 years ago
https://github.com/mozilla-b2g/gaia/pull/14152 - Here is the update
(Assignee)

Comment 11

4 years ago
Created attachment 8339659 [details] [review]
Shrink UI integration into NFC Manager r=alive
Attachment #8338293 - Attachment is obsolete: true
Attachment #8339659 - Flags: review?(alive)
Comment on attachment 8339659 [details] [review]
Shrink UI integration into NFC Manager r=alive

See github.
Attachment #8339659 - Flags: review?(alive)
(Assignee)

Updated

4 years ago
Attachment #8339659 - Flags: review?(alive)
(Assignee)

Comment 13

4 years ago
In the latest the revision,
- renamed the earlier custom event to 'check-p2p-registration-for-active-app'.
- Add new custom event as per your latest comment 'dispatch-p2p-user-response-on-active-app'
- Add / Remove 'shrinking-sent' at more appropriate places
Comment on attachment 8339659 [details] [review]
Shrink UI integration into NFC Manager r=alive

r+ if you move the event handler from window-manager to shrinkUI
Attachment #8339659 - Flags: review?(alive) → review+
BTW please pay attention to travis status.
(Assignee)

Comment 16

4 years ago
Created attachment 8340662 [details] [review]
Shrink UI integration into NFC Manager r=alive
Attachment #8339659 - Attachment is obsolete: true
Attachment #8340662 - Flags: review?(alive)
(Assignee)

Comment 17

4 years ago
As suggested moved the logic of handling events from window_manager to shrinking_ui.js
(Assignee)

Comment 18

4 years ago
Travis CI keeps failing at this point all the time. Any pointers ?

module.js:340
    throw err;
          ^
Error: Cannot find module 'mocha/lib/reporters/Spec'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:280:25)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at getOpts (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-js-runner/bin/marionette-mocha:26:16)
    at main (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-js-runner/bin/marionette-mocha:145:14)
    at Object.<anonymous> (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-js-runner/bin/marionette-mocha:151:3)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
make: *** [test-integration] Error 8
Comment on attachment 8340662 [details] [review]
Shrink UI integration into NFC Manager r=alive

r=me
I'd re-run the test. Wait and see..
Attachment #8340662 - Flags: review?(alive) → review+
NFC is not a committed feature for 1.3, so this should not block the release.
blocking-b2g: 1.3+ → 1.3?
(Assignee)

Comment 21

4 years ago
https://travis-ci.org/mozilla-b2g/gaia/builds/14773455 - All green. CI needed
Flags: needinfo?(alive)
master
https://github.com/mozilla-b2g/gaia/commit/943825614405b539091d53d9dc4c94cffd4992dd
Assignee: nobody → psiddh
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(alive)
Resolution: --- → FIXED
NFC isn't a committed feature, so clearing nom.
blocking-b2g: 1.3? → ---
You need to log in before you can comment on or make changes to this bug.