Wifi modification for captive portal detection

VERIFIED FIXED in Firefox 21

Status

defect
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: schien, Assigned: schien)

Tracking

unspecified
B2G C4 (2jan on)
Other
Other
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:shira+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [mno11][triaged:1/18] QARegressExclude)

Attachments

(4 attachments, 7 obsolete attachments)

3.99 KB, patch
schien
: review+
Details | Diff | Splinter Review
4.62 KB, patch
schien
: review+
Details | Diff | Splinter Review
3.98 KB, patch
schien
: review+
Details | Diff | Splinter Review
4.65 KB, patch
schien
: review+
Details | Diff | Splinter Review
split B2G Wifi modification for captive portal detection (bug 752982)
Using mozChromeEvent/mozContent event to notify the progress of captive portal login procedure.
Attachment #681430 - Flags: review?(mrbkap)
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?
(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. :)
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.
Attachment #686919 - Flags: feedback?(mrbkap)
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.

Updated

7 years ago
Whiteboard: mno11

Comment 6

7 years ago
Add Askeing for QA support
QA Contact: fyen
blocking-b2g: --- → shira+
Whiteboard: mno11
rebase to latest m-c tip, this patch is verified on my local environment and ready for checking in shira branch.
Attachment #681430 - Attachment is obsolete: true
Attachment #681430 - Flags: review?(mrbkap)
Attachment #694184 - Flags: review?(mrbkap)
rebase to latest m-c tip, this patch is verified on my local environment and ready for checking in shira branch.
Attachment #686919 - Attachment is obsolete: true
Attachment #686919 - Flags: feedback?(mrbkap)
Attachment #694186 - Flags: review?(mrbkap)
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.

Updated

7 years ago
Whiteboard: [mno11]
Whiteboard: [mno11] → [mno11][triaged:1/18]
Hi mrbkap, have time review this patch?
Flags: needinfo?(mrbkap)
: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
Flags: needinfo?(mrbkap)
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.
Attachment #694186 - Flags: review?(vchang)
Attachment #694186 - Flags: review?(mrbkap)
Attachment #694186 - Flags: review?(bsmith)
Comment on attachment 694184 [details] [diff] [review]
Part 1 - mozChromeEvent/mozContentEvent for captive portal login (rebase to m-c tip)

Change reviewer to timdream.
Attachment #694184 - Flags: review?(timdream)
Attachment #694184 - Flags: review?(mrbkap)
Attachment #694184 - Flags: review?(bsmith)

Comment 14

7 years ago
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 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?
Attachment #694184 - Flags: review?(timdream) → review?(21)
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 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 ?
Attachment #694186 - Flags: review?(vchang)
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.
Attachment #694184 - Flags: review?(21) → review?(timdream)
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.
Attachment #694184 - Flags: review?(timdream) → review+
rebase to m-c tip and update reviewer info in hg log.
Attachment #694184 - Attachment is obsolete: true
Attachment #709619 - Flags: review+
rebase to m-c tip and update patch according to comment 14 and comment 17.
Attachment #694186 - Attachment is obsolete: true
Attachment #709621 - Flags: review?(vchang)
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.
Attachment #709621 - Flags: review?(vchang) → review+
update patch according to comment 22, carry r+.
Attachment #709621 - Attachment is obsolete: true
Attachment #709641 - Flags: review+
update API name according to the modification in bug 752982. carry r+.
Attachment #709641 - Attachment is obsolete: true
Attachment #710123 - Flags: review+
Attachment #686923 - Attachment is obsolete: true
Need check-in after Part 1 of bug 752982 landed.
Keywords: checkin-needed
Several other bugs listed against Captive Portal.  Going to skip marking verified because of those bugs.

Updated

6 years ago
Whiteboard: [mno11][triaged:1/18] → [mno11][triaged:1/18] QARegressExclude
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.