1010733 - Capability for partner's app to implement WISPr function

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Henry Chang [:hchang]]

Per http://goo.gl/AVHUwF, B2G should provide the capability for partner's app to be able to implement WISPr in phase 1.

Comment 1

[Henry Chang [:hchang]]

Per discussion with vhcang and chucklee, we would like to provide WISPr capability for partner with the following minimum modification:

1) Add a new system message 'wispr-login' which binds certain certified permission.
2) While sending 'captive-portal-login' event, broadcast 'wispr-login' system message as well if the response is WISPr xml format.

An WISPr app could implement such function like the following:
1) Use MozWifiManager.associate/forget to add the ssid it supports.
2) Use setMessageHandler('wispr-login', handleWisprLogin) to handle WISPr login.

Also, if we are going to support "auto connect" (Bug 866718), partner could have
their own default SSID list for "auto connect".

Comment 2

[Henry Chang [:hchang]]

Attachment: WISPr WIP v1

1) Add system message 'wispr-login' for system app to bring up partner's WISPr app.
2) Send mozChromeEvent 'captive-portal-login-success' to system app on login success. System app could use this event to dismiss captive portal notification.
3) Add a new property 'isWispr' to the detail carried by 'captive-portal-login' event.

Comment 3

[Henry Chang [:hchang]]

Attachment: WISPr WIP v2

Broadcat system message 'captive-portal' whenever detected. Partner's app could listen to this message to proceed the following task.

Comment 4

[Henry Chang [:hchang]]

Attachment: WISPr WIP v3

Comment 5

[Henry Chang [:hchang]]

Comment on attachment 8428552 [details] [diff] [review]
WISPr WIP v3

Hi schien, since you are the first author of captivedetect.js, I'd like to "route" the review request to you. If you have a hard time reviewing this, please feel free to route to the next hop :p Thanks!

I am briefing what this patch is trying to do:

In order to provide a platform for our partner to develop WIPSr (an extension of captive portal), what we choose to support is to broadcast a system message to notify apps who are interested in the need of captive portal log-in event. 

For now, the mozChromeEvent 'captive-portal-login' is only used to let the system app know to deal with captive portal log-in. For partner's app to handle captive portal with knowledge of WISPr, we will use system message 'captive-portal' to bring up partner's app.

'captive-portal' is bound to 'wifi-manage' which is only allowed by certified app so that there won't be a pile of apps launched when captive-portal is detected.

Partner's WISPr app could be implemented as the following:
1) Add 'captive-portal' to its manifest.webapp
2) Set message handler for 'captive-portal'
3) Use MozWifiManager to add networks which this WISPr app could handle.
4) When the app is launched by 'captive-portal', handle all the WISPr things.
5) Close the app when finished.

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Hi Henry, should we also launch WISPr app when user click the notification on system tray? In your current implementation, both browser app and WISPr app will be launched when user selecting a login-required access point in settings app.
I'll suggest to move the logic into system app so that only one app (browser app or WISPr app) will be launched while connecting to a login-required access point.

Comment 7

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

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #6)
> Hi Henry, should we also launch WISPr app when user click the notification
> on system tray? In your current implementation, both browser app and WISPr
> app will be launched when user selecting a login-required access point in
> settings app.
> I'll suggest to move the logic into system app so that only one app (browser
> app or WISPr app) will be launched while connecting to a login-required
> access point.

Well.. if system-message for wispr and captive-portal is sent to system app at the same, we cannot decide which to launch since the launch path is not the same.

Browser app is invoked by mozActivity which is triggered by mozChromeEvent of captive portal.
WISPr is invoked by open-app from system message internal.

Gecko should tell us there's a captive-portal app installed otherwise we need to display browser app.
But I wonder this is something hacky...

Also note there's no browser app in the future.

Comment 8

[Henry Chang [:hchang]]

:schien, :alive,

Thanks for your suggestions. Actually, I couldn't find the perfect solution for this bug. What I propose is the least hacky approach.

Since captive-portal/WISPr is an application level function, we think gecko shouldn't be aware of it too much. So we decide to let gecko do nothing specific for WISPr other than notifying the system "log-in is required", that is, broadcasting a system message 'captive-portal'. This is the idea of my patch.

For current system app implementation, there are different ways of handling mozChromeEvent 'captive-portal-login':

(Please correct me if anything wrong)

1) During FTU: 

this.entrySheet = new EntrySheet(document.getElementById('screen'),
                                 url,
                                 new BrowserFrame({url: url}));
this.entrySheet.open();

2) Auto connecting to AP: Display notification (pop up browser via mozActivity)
3) Manually connecting to AP: Pop up browser via mozActivity. 

For case 2), if the partner's WISPr app successfully logs us in at background, everything is going fine. No browser popped up and no user intervention. One might argue there is an useless notification on the system tray. This is an existing issue because we can reproduce this state by the following step:

a) Auto connect to a log-in-required AP (notification shown on the system tray)
b) Open browser and log in on their own without clicking the notification.
c) Now the notification on the system tray becomes useless.

For case 1), since the WISPr app is never launched before and the username/password is not set, the WISPr app couldn't handle the login process. The user experience is as before.

For case 3), the browser will pop up automatically and the WISPr app will also be launched by system message and handle the log-in process at background. If the WISPr app succeeds in log-in, the user will see the log-in page without any 'already logged in' notice (depends on how the WISPr app implements). I think this state may be a unacceptable issue (even though it's fine to me).

Other approaches may be hacky or require unsupported feature on B2G such as application background service.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Henry,

You are right, and we indeed need a new UI flow to properly support both captive portal and WISPr.
Let's find a time to discuss this offline. Do you know who is responsible of UX of both items?

Comment 10

[Henry Chang [:hchang]]

Just had a quick discussion with :schien and came out some issues for my current design:

1) All the installed apps which are interested in WISPr would race to handle WISPr.
2) The pop-up browser or 'log-in-required' notification will bother users while the WISPr app(s) already logged them in.

In fact, only when we consider WISPr a part of B2G could (1) and (2) be avoided. 

To avoid (1), we must define "What is a WISPr app" and "What WISPr app should be chosen to handle WISPr".
To avoid (2), B2G needs the prior knowledge that "if any WISPr app is installed" and "if the installed WISPr app could successfully log us in" to prevent from bothering the user.

Omega should be in charge of the UX according to http://goo.gl/AVHUwF if everything unchanged.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Henry Chang [:henry] from comment #10)
> Just had a quick discussion with :schien and came out some issues for my
> current design:
> 
> 1) All the installed apps which are interested in WISPr would race to handle
> WISPr.

We already know this is by design for the first iteration since Gecko does not plan to target specific app with a given identifier.

> 2) The pop-up browser or 'log-in-required' notification will bother users
> while the WISPr app(s) already logged them in.
> 
> In fact, only when we consider WISPr a part of B2G could (1) and (2) be
> avoided. 
> 
> To avoid (1), we must define "What is a WISPr app" and "What WISPr app
> should be chosen to handle WISPr".
> To avoid (2), B2G needs the prior knowledge that "if any WISPr app is
> installed" and "if the installed WISPr app could successfully log us in" to
> prevent from bothering the user.

I think the solution of (2) should be
-- Always use entry sheet for captive portal, instead of launching the browser.
-- If WISPr logs the user in (Gaia to find out that with captive-portal-login-success from Gecko), or it bring itself to foreground by mozApps.getSelf() ...launch(), we dismiss the entry sheet.

> Omega should be in charge of the UX according to http://goo.gl/AVHUwF if
> everything unchanged.

Comment 12

[Omega Feng [:Omega] [:馮於懋]]

ni? new UX owner Jenny.

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8428552 [details] [diff] [review]
WISPr WIP v3

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

Per our discussion today, this patch needs more modification. Cancel the review for now.

Comment 14

[Henry Chang [:hchang]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #13)
> Comment on attachment 8428552 [details] [diff] [review]
> WISPr WIP v3
> 
> Review of attachment 8428552 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Per our discussion today, this patch needs more modification. Cancel the
> review for now.

The modification will be on gaia side. I will also add the mozChromeEvent 'captive-portal-login-success' to V4 for system app to be able to dismiss the notification whenever needed. Thanks!

Comment 15

[Henry Chang [:hchang]]

Attachment: WISPr WIP v4

Comment 16

[Henry Chang [:hchang]]

Comment on attachment 8430506 [details] [diff] [review]
WISPr WIP v4

Hi :schien,

I added the notification of 'captive-portal-login-success' to this patch as compared to the last one. The test case is also added. However, it looks the captivedetect xpcshell tests are not run on the try server. We might need to run the tests locally and file a follow-up bug to add all the test cases to try if needed. 

Thanks!

Comment 17

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8430506 [details] [diff] [review]
WISPr WIP v4

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

Looks good but need some more refactory.

::: toolkit/components/captivedetect/captivedetect.js
@@ +342,5 @@
> +    this._broadcastCaptivePortalSystemMessage();
> +  },
> +
> +  _broadcastCaptivePortalSystemMessage: function _broadcastCaptivePortalSystemMessage() {
> +    gSysMsgr.broadcastMessage(kCaptivePortalSystemMessage, {});

You don't need a function to for a single-line code only used in one place. Use line #346 in line #342 directly.

@@ +363,5 @@
>        }
>  
> +      if (success) {
> +        this._notifyLoginSuccess(this._runningRequest);
> +      }

I'll suggest to move these code to _removeRequest because that's where we notify login-abort. http://dxr.mozilla.org/mozilla-central/source/toolkit/components/captivedetect/captivedetect.js?from=captivedetect.js&case=true#403

@@ +376,5 @@
> +    let details = {
> +      type: kCaptivePortalLoginSuccessEvent,
> +      id: runningRequest['eventId'],
> +    };
> +    // Send a event to shell.js to dispatch a mozChromeEvent to gaia.

You don't need this comment because the code is clear enough. In addition, you might want to generalize this function to |notifyLoginFinished(type, id)| after moving code above to _removeRequest.

Comment 18

[Henry Chang [:hchang]]

Hi,

Thanks for your review! I have some questions regarding your comments.
Please correct me if anything I ignored or misunderstood. Thanks!

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #17)
> Comment on attachment 8430506 [details] [diff] [review]
> WISPr WIP v4
> 
> Review of attachment 8430506 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good but need some more refactory.
> 
> @@ +363,5 @@
> >        }
> >  
> > +      if (success) {
> > +        this._notifyLoginSuccess(this._runningRequest);
> > +      }
> 
> I'll suggest to move these code to _removeRequest because that's where we
> notify login-abort.
> http://dxr.mozilla.org/mozilla-central/source/toolkit/components/
> captivedetect/captivedetect.js?from=captivedetect.js&case=true#403
> 

What is the reason that login-abort and login-success has to be 
in one function? Besides, _remvoeRequest already does too many things
other than removing request. I am worried about the logic of _removeRequest
becomes too complicated >"<

> @@ +376,5 @@
> > +    let details = {
> > +      type: kCaptivePortalLoginSuccessEvent,
> > +      id: runningRequest['eventId'],
> > +    };
> > +    // Send a event to shell.js to dispatch a mozChromeEvent to gaia.
> 
> You don't need this comment because the code is clear enough. In addition,
> you might want to generalize this function to |notifyLoginFinished(type,
> id)| after moving code above to _removeRequest.

I am wondering if login-abort is a kind of login-finish. The semantics of 
|abort| and |success| is hard to be generalized to one.

Comment 19

[Henry Chang [:hchang]]

Attachment: WISPr WIP v5

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Quick note about the offline discussion with Henry:
1. Sending login-success event only when we consider the captive portal log-in process is success. The |complete| attribute is not the correct flag to distinguish it so we cannot move the code to |_removeRequest|
2. We should only send login-success event if login event has been sent during this transaction.

Comment 21

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8430693 [details] [diff] [review]
WISPr WIP v5

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

Please add the test step for login-success in test_captive_portal_found_303.js and test_multiple_requests.js.

::: toolkit/components/captivedetect/captivedetect.js
@@ +359,5 @@
>        }
>  
> +      // Only when the request has a event id and |success| is true
> +      // do we need to notify the login-success event.
> +      if (_runningRequest['eventId'] && success) {

Please use |this._runningRequest.hasOwnProperty('eventId')| and inline the |_notifyLoginSuccess| since it's only used one time.

Comment 22

[Henry Chang [:hchang]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #21)
> Comment on attachment 8430693 [details] [diff] [review]
> WISPr WIP v5
> 
> Review of attachment 8430693 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please add the test step for login-success in
> test_captive_portal_found_303.js and test_multiple_requests.js.
> 
> ::: toolkit/components/captivedetect/captivedetect.js
> @@ +359,5 @@
> >        }
> >  
> > +      // Only when the request has a event id and |success| is true
> > +      // do we need to notify the login-success event.
> > +      if (_runningRequest['eventId'] && success) {
> 
> Please use |this._runningRequest.hasOwnProperty('eventId')| and inline the
> |_notifyLoginSuccess| since it's only used one time.

1) My argument is it's more readable and make each function as short and simple as possible.
   But I am also good to inline this function! Will address it in the next patch.

2) I also found this one "this._runningRequest.hasOwnProperty('eventId')" and am attaching a new one :p

Besides, I am going to add all the test cases to xpcshell_b2g.js. Probably do it in this bug or in a separate bug I just filed: Bug 1017513. What do you think?

Thanks!

Comment 23

[Henry Chang [:hchang]]

Attachment: Part 1: Add exiting test cases to xpcshell_b2g.ini

Comment 24

[Henry Chang [:hchang]]

Attachment: Part 2: Implementation and new test cases

Comment 25

[Henry Chang [:hchang]]

Attached two patches and not ready for review yet because I am still waiting for test result.

Comment 26

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Henry Chang [:henry] from comment #22)
> Besides, I am going to add all the test cases to xpcshell_b2g.js. Probably
> do it in this bug or in a separate bug I just filed: Bug 1017513. What do
> you think?
> 
> Thanks!
I suppose these test cases are ran on b2g-desktop but all the xpcshell-test are disabled on this platform for a while. I'm happy to see people enable these test cases on b2g emulator again. I'm ok with doing it in a separate bug.

Comment 27

[Henry Chang [:hchang]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #26)
> (In reply to Henry Chang [:henry] from comment #22)
> > Besides, I am going to add all the test cases to xpcshell_b2g.js. Probably
> > do it in this bug or in a separate bug I just filed: Bug 1017513. What do
> > you think?
> > 
> > Thanks!
> I suppose these test cases are ran on b2g-desktop but all the xpcshell-test
> are disabled on this platform for a while. I'm happy to see people enable
> these test cases on b2g emulator again. I'm ok with doing it in a separate
> bug.

From this try server result [1], it seems captivedetect xpcshell test is 
not running on any platform. (I intentionally add a failure test case but none failed.
Emulator ran it because I added it.)

I am finally able to run xpcshell test locally and unfortunately the existing 
tests are broken. It requires some fixes to enable it. So, do you mind adding
the test cases for new functions and fixing the existing test cases in a separate
bug and land this feature-b2g bug first?

Thanks!



[1] https://tbpl.mozilla.org/?tree=Try&rev=e38b76d47246

Comment 28

[Henry Chang [:hchang]]

Attachment: Part 2: Implementation and new test cases

Comment 29

[Henry Chang [:hchang]]

Comment on attachment 8431352 [details] [diff] [review]
Part 2: Implementation and new test cases

This patch addresses the comments for the previous patch:

1) Add 'captive-portal-login-success' to 

test_captive_portal_found.js
test_captive_portal_found_303.js
test_multiple_requests.js

2) Inline _notifyLoginSuccess

I am trying to add test cases to xpcshell_b2g.ini but it requires a fix for test_captive_portal_found_303.js regarding the multiple interface request. It's the existing failure. So I prefer moving "Part 1" to another bug and only ask for review this patch. Thanks!

Comment 30

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8431352 [details] [diff] [review]
Part 2: Implementation and new test cases

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

lgtm!

Comment 31

[Henry Chang [:hchang]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #30)
> Comment on attachment 8431352 [details] [diff] [review]
> Part 2: Implementation and new test cases
> 
> Review of attachment 8431352 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm!

Thanks! I will modify the comment and obsolete Part 1. Thanks!

Comment 32

[Henry Chang [:hchang]]

Attachment: Patch for checkin-needed (r+'d)

Comment 33

[Jenny Lee]

Attachment: WISPr v1.0.pdf

Please refer to the document for captive portal/WISPr flow. Tks.

Comment 34

[Henry Chang [:hchang]]

(In reply to Jenny Lee from comment #33)
> Created attachment 8431415 [details]
> WISPr v1.0.pdf
> 
> Please refer to the document for captive portal/WISPr flow. Tks.

This bug is going to be resolved and we can track the flow improvement in Bug 1017472.
Thanks!

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Can we please get a Try run to confirm that xpcshell is fixed now?

Comment 36

[Henry Chang [:hchang]]

The existing xpcshell ini (toolkit/components/captivedetect/test/unit/xpcshell.ini) is neither added to xpcshell_b2g.ini nor any other ini. Also, the current tests are broken and will be fixed by Bug 1017513. 
I get a try run with Bug 1017513 followed by this bug:

https://tbpl.mozilla.org/?tree=Try&rev=418cd8ee252d

Local xpcshell test is passed.

Comment 37

[Henry Chang [:hchang]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #35)
> Can we please get a Try run to confirm that xpcshell is fixed now?

The try run passed. Flag checkin-needed again. Thanks!

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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

Comment 39

[Carsten Book [:Tomcat]]

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