943179 - Integrate Shrinking UI with NfcManager (System App)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[psiddh]

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

Comment 1

[psiddh]

Attachment: (v1) Bug 943179 : Integrate P2P Shrink UI in nfc_manager. r=alive

Integrate Shrinking UI functionality to NfcManager

Comment 2

[(inactive after 6/18) Alive Kuo [: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?

Comment 3

[psiddh]

Attachment: (v2) Bug 943179 : Integrate P2P Shrink UI in nfc_manager. r=alive

Comment 4

[psiddh]

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) { .... '

Comment 5

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

(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 6

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

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.

Comment 7

[(inactive after 6/18) Alive Kuo [: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

[Kai-Chih Hu [:khu]]

I think this one should be a 1.3+ bug. Ken, what do you think?

Comment 9

[Ken Chang[:kenkai]]

(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....

Comment 10

[psiddh]

https://github.com/mozilla-b2g/gaia/pull/14152 - Here is the update

Comment 11

[psiddh]

Attachment: Shrink UI integration into NFC Manager r=alive

Comment 12

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

Comment on attachment 8339659 [details] [review]
Shrink UI integration into NFC Manager r=alive

See github.

Comment 13

[psiddh]

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 14

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

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

Comment 15

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

BTW please pay attention to travis status.

Comment 16

[psiddh]

Attachment: Shrink UI integration into NFC Manager r=alive

Comment 17

[psiddh]

As suggested moved the logic of handling events from window_manager to shrinking_ui.js

Comment 18

[psiddh]

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 19

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

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..

Comment 20

[Jason Smith [:jsmith]]

NFC is not a committed feature for 1.3, so this should not block the release.

Comment 21

[psiddh]

https://travis-ci.org/mozilla-b2g/gaia/builds/14773455 - All green. CI needed

Comment 22

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

master
https://github.com/mozilla-b2g/gaia/commit/943825614405b539091d53d9dc4c94cffd4992dd

Comment 23

[Jason Smith [:jsmith]]

NFC isn't a committed feature, so clearing nom.

Comment 24

[Walter Chen[:ypwalter][:wachen]]

Shrinking UI test cases added.
https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=50&sortfield=created_on&sortdirection=desc&filter-suite=419