780793 - Wifi modification for captive portal detection

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

split B2G Wifi modification for captive portal detection (bug 752982)

Comment 1

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

Attachment: Part 1 - mozChromeEvent/mozContentEvent for captive portal login

Using mozChromeEvent/mozContent event to notify the progress of captive portal login procedure.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 681430 [details] [diff] [review]
Part 1 - mozChromeEvent/mozContentEvent for captive portal login

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

Changes looks fine, small nits to fix though. It also needs some Gaia love :)

::: b2g/chrome/content/shell.js
@@ +743,5 @@
>  }
>  
> +var CaptivePortalLoginHelper = {
> +  init: function init() {
> +    dump('####Gecko####CaptivePortalLoginHelper init\n');

nit: remove all those dumps to not pollute the log.

::: b2g/confvars.sh
@@ +17,5 @@
>  # MOZ_APP_DISPLAYNAME is set by branding/configure.sh
>  
>  MOZ_SAFE_BROWSING=
>  MOZ_SERVICES_COMMON=1
> +MOZ_SERVICES_CAPTIVEDETECT=1

nit: Any reason to not use DETECTOR instead of DETECT?

Comment 3

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

(In reply to Vivien Nicolas (:vingtetun) from comment #2)
> ::: b2g/confvars.sh
> @@ +17,5 @@
> >  # MOZ_APP_DISPLAYNAME is set by branding/configure.sh
> >  
> >  MOZ_SAFE_BROWSING=
> >  MOZ_SERVICES_COMMON=1
> > +MOZ_SERVICES_CAPTIVEDETECT=1
> 
> nit: Any reason to not use DETECTOR instead of DETECT?
I was just thinking about saving 3 characters, DETECTOR is ok for me. :)

Comment 4

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

Attachment: WIP - perform captive protal detection while WIFI is connected.

Start/stop captive portal detection while WIFI interface is connected/disconnected.

NOTE: Still need to set |Service.io.offline| to true before sending XHR, we cannot do it in offline mode. I'm not sure if there is an easy way we can delay the network access from app before captive portal login completion.

Comment 5

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

Attachment: gaia prototype - using browser app to display the login page

This gaia prototype will launch browser app to display the captive portal login page. Cancel login is not supported in the version.

NOTE: I've tested this prototype with several open wifi networks in the metro station in Taipei.

Comment 6

[Al Tsai [:atsai]]

Add Askeing for QA support

Comment 7

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

Attachment: Part 1 - mozChromeEvent/mozContentEvent for captive portal login (rebase to m-c tip)

rebase to latest m-c tip, this patch is verified on my local environment and ready for checking in shira branch.

Comment 8

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

Attachment: Part 2 - perform captive protal detection while WIFI is connected. (rebase to m-c tip)

rebase to latest m-c tip, this patch is verified on my local environment and ready for checking in shira branch.

Comment 9

[Askeing Yen[:askeing][:fyen]]

Device: unagi
Build ID: 20121228040204
Wifi: 7Wifi, Wifly, TPE-Free

After test unagi with captive portal wifi, there is no notifications on screen.
However, if unagi connect to wifi and open the browser, then access any website, the browser still can redirect to log in page.

Comment 10

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

Hi mrbkap, have time review this patch?

Comment 11

[Joe Cheng [:jcheng] (please needinfo)]

:mrbkap, Shira build is being planned for another round of new feature testing with nightly builds from the v1-train/mozilla-b2g18 branches next week. It will be great if you can spare some time to do the review this week so testing progress won't be blocked. Or anyone else can do the review? Thanks

Comment 12

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

Comment on attachment 694186 [details] [diff] [review]
Part 2 - perform captive protal detection while WIFI is connected. (rebase to m-c tip)

Change reviewer to vchang.

Comment 13

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

Comment on attachment 694184 [details] [diff] [review]
Part 1 - mozChromeEvent/mozContentEvent for captive portal login (rebase to m-c tip)

Change reviewer to timdream.

Comment 14

[u459114]

Comment on attachment 694186 [details] [diff] [review]
Part 2 - perform captive protal detection while WIFI is connected. (rebase to m-c tip)

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

::: dom/system/gonk/NetworkManager.js
@@ +417,5 @@
> +      CaptivePortalDetectionHelper.performDetection(this.active.name, function () {
> +        // We can disconnect wifi in here if user abort the login procedure.
> +      });
> +    }
> +

A minor suggestion.
Move Line 413~421 into observe function, or move Line 219~223 into setAndConfigureActive function.

Comment 15

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

Comment on attachment 694184 [details] [diff] [review]
Part 1 - mozChromeEvent/mozContentEvent for captive portal login (rebase to m-c tip)

Vivien, could you help review the modification for adding new mozChromeEvent?

Comment 16

[Joe Cheng [:jcheng] (please needinfo)]

Want to make sure everyone knows that Shira is targeting 2/15 code freeze. 
Shira+ bugs needs more attention for unassigned, needinfo?,  review?
Full regression test is planned on 2/6. Need to have shira+ bugs landed by the end of 2/5 to be covered in full regression testing. Thanks

Comment 17

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 694186 [details] [diff] [review]
Part 2 - perform captive protal detection while WIFI is connected. (rebase to m-c tip)

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

::: dom/system/gonk/NetworkManager.js
@@ +219,5 @@
> +            // Abort ongoing captive portal detection on the wifi interface
> +            if (CaptivePortalDetectionHelper.available &&
> +                network.type == Ci.nsINetworkInterface.NETWORK_TYPE_WIFI) {
> +              CaptivePortalDetectionHelper.abort(network.name);
> +            }

Can we move these code to CaptivePortalDetectionHelper ?

@@ +414,5 @@
> +    // perform captive portal detection on wifi interface
> +    if (CaptivePortalDetectionHelper.available && this.active &&
> +        this.active.type == Ci.nsINetworkInterface.NETWORK_TYPE_WIFI) {
> +      CaptivePortalDetectionHelper.performDetection(this.active.name, function () {
> +        // We can disconnect wifi in here if user abort the login procedure.

Please file a new bug to address this comment.

@@ +417,5 @@
> +      CaptivePortalDetectionHelper.performDetection(this.active.name, function () {
> +        // We can disconnect wifi in here if user abort the login procedure.
> +      });
> +    }
> +

Can we move these code to CaptivePortalDetectionHelper ?

Comment 18

[Joe Cheng [:jcheng] (please needinfo)]

Comment on attachment 694184 [details] [diff] [review]
Part 1 - mozChromeEvent/mozContentEvent for captive portal login (rebase to m-c tip)

pinged vivien seperately, he is fine having Tim to review this.

Comment 19

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

Comment on attachment 694184 [details] [diff] [review]
Part 1 - mozChromeEvent/mozContentEvent for captive portal login (rebase to m-c tip)

This part is fine. Note that |shell.sendChromeEvent| will defer the events all the way till system app fires |load| event. It should be fine for the use case.

Comment 20

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

Attachment: Part 1 - mozChromeEvent/mozContentEvent for captive portal login (rebase to m-c tip)

rebase to m-c tip and update reviewer info in hg log.

Comment 21

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

Attachment: Part 2 - perform captive protal detection while WIFI is connected. v2 (rebase to m-c tip)

rebase to m-c tip and update patch according to comment 14 and comment 17.

Comment 22

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 709621 [details] [diff] [review]
Part 2 - perform captive protal detection while WIFI is connected. v2 (rebase to m-c tip)

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

The patch looks good. Thank you.

::: dom/system/gonk/NetworkManager.js
@@ +882,5 @@
> +          // perform captive portal detection on wifi interface
> +          if (_available && network &&
> +              network.type == Ci.nsINetworkInterface.NETWORK_TYPE_WIFI) {
> +            _performDetection(network.name, function () {
> +              // We can disconnect wifi in here if user abort the login procedure.

Please file a follow-up when user aborts, and mention the bug number in a // TODO comment.

Comment 23

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

Attachment: Part 2 - perform captive protal detection while WIFI is connected. v3 (rebase to m-c tip)

update patch according to comment 22, carry r+.

Comment 24

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

Attachment: Part 2 - perform captive protal detection while WIFI is connected. v3.1

update API name according to the modification in bug 752982. carry r+.

Comment 25

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

Attachment: (b2g18) Part 1 - mozChromeEvent/mozContentEvent for captive portal login

rebase attachment 709619 [details] [diff] [review] to mozilla-b2g18.

Comment 26

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

Attachment: (b2g18) Part 2 - perform captive protal detection while WIFI is connected.

rebase attachment 710123 [details] [diff] [review] to mozilla-b2g18.

Comment 27

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

Need check-in after Part 1 of bug 752982 landed.

Comment 28

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

Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=b31407d3263f

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33be341bb133
https://hg.mozilla.org/integration/mozilla-inbound/rev/470187171cf1

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/33be341bb133
https://hg.mozilla.org/mozilla-central/rev/470187171cf1

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/16bcb1300fdb
https://hg.mozilla.org/releases/mozilla-b2g18/rev/2910a66baeaf

Comment 32

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Several other bugs listed against Captive Portal.  Going to skip marking verified because of those bugs.