Closed Bug 943179 Opened 9 years ago Closed 9 years ago

Integrate Shrinking UI with NfcManager (System App)

Categories

(Firefox OS Graveyard :: NFC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: psiddh, Assigned: psiddh)

References

Details

Attachments

(1 file, 3 obsolete files)

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
Depends on: 860910
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)
Blocks: 903305
Attachment #8338229 - Attachment is obsolete: true
Attachment #8338293 - Flags: review?(alive)
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.
I think this one should be a 1.3+ bug. Ken, what do you think?
blocking-b2g: --- → 1.3?
Flags: needinfo?(kchang)
(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)
https://github.com/mozilla-b2g/gaia/pull/14152 - Here is the update
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)
Attachment #8339659 - Flags: review?(alive)
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.
Attachment #8339659 - Attachment is obsolete: true
Attachment #8340662 - Flags: review?(alive)
As suggested moved the logic of handling events from window_manager to shrinking_ui.js
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?
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
Closed: 9 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.