Closed Bug 1273950 Opened 6 years ago Closed 5 years ago

[CaptivePortal] Allow rechecks to be triggered to reaffirm portal state, fire a notification when a check completes

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(2 files, 2 obsolete files)

No description provided.
Whiteboard: [necko-active]
After a portal has been detected, captivedetect.js doesn't allow consumers to trigger a recheck until it thinks that the portal has been unlocked. This is a problem when the user logs into the portal using a different client:

- captivedetect.js does a recheck to see if the portal is unlocked only after there has been HTTP activity.
- When the user logs in using a different client, there is no HTTP activity in Firefox, so a recheck is never triggered.

This is probably because the detection code was written in the context of FxOS, in which there *is no other client*.

The goals of this bug are to allow rechecks to be triggered to reaffirm the current state, and to fire a notification when a recheck completes even if the state hasn't changed so that consumers know that we have up-to-date information.
Summary: [CaptivePortal] Send a notification whenever a CP check is performed → [CaptivePortal] Allow rechecks to be triggered to reaffirm portal state, fire a notification when a check completes
I forgot to mention, the use case in mind for the above is to trigger a recheck when Firefox regains focus from a different application, and act upon the results when we get them.
This seems like the best way to address the issue in comment 1, without breaking FxOS behaviour.
Attachment #8755256 - Flags: review?(MattN+bmo)
This is a separate notification, so it shouldn't affect FxOS at all. I hope this works well with Nihanth's patch. With this, whenever we are in a captive portal, a notification will be fired every 3 seconds.
Attachment #8755258 - Flags: review?(MattN+bmo)
Thanks! What do you think about reducing the polling time?
(In reply to Nihanth Subramanya [:nhnt11] from comment #5)
> Thanks! What do you think about reducing the polling time?

To clarify, I was thinking of overriding the polling time pref in firefox.js with a value of 1000ms or so.
(In reply to Nihanth Subramanya [:nhnt11] from comment #6)
> (In reply to Nihanth Subramanya [:nhnt11] from comment #5)
> > Thanks! What do you think about reducing the polling time?
> 
> To clarify, I was thinking of overriding the polling time pref in firefox.js
> with a value of 1000ms or so.

I guess that's possible, but is it really necessary? I don't know if 1s is much better than 3s.
Comment on attachment 8755256 [details] [diff] [review]
Add pref to check captive portal without waiting for network activity

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

I haven't had time to learn this code yet s if you would like a faster review please flag someone else (e.g. on the Necko team or perhaps mikedeboer is willing to learn this).

::: toolkit/components/captivedetect/captivedetect.js
@@ +185,2 @@
>                                   captivePortalDetector._pollingTime,
>                                   timer.TYPE_ONE_SHOT);

Indentation is off now.
Comment on attachment 8755258 [details] [diff] [review]
Fire a notification whenever a captive portal check is performed

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

A simple test would be good but I'm not sure if there are existing tests for this component to easily add to.
Attachment #8755258 - Flags: review?(MattN+bmo) → review+
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #8)
> I haven't had time to learn this code yet so if you would like a faster
> review please flag someone else.

It seems like there's enough other people in the history of this file to ask for review (fabrice, thinker, schien, etc.)
Attachment #8755256 - Flags: review?(MattN+bmo) → review?(schien)
Comment on attachment 8755256 [details] [diff] [review]
Add pref to check captive portal without waiting for network activity

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

::: toolkit/components/captivedetect/captivedetect.js
@@ +89,5 @@
>                                .getService(Ci.nsIHttpActivityDistributor);
>    let urlFetcher = null;
>  
> +  let waitForNetworkActivity =
> +    Services.prefs.getBoolPref('captivedetect.waitForNetworkActivity');

maybe you can use |Services.appinfo.widgetToolkit == "gonk"| without creating a new pref, but I'm fine with it.
Attachment #8755256 - Flags: review?(schien) → review+
Attachment #8755256 - Attachment is obsolete: true
Hey Valentin, is there any reason this didn't land yet? We were hoping to have Mozillians test the Captive Portal feature at and on the way to the All Hands.
Flags: needinfo?(valentin.gosu)
Status: NEW → ASSIGNED
Comment on attachment 8755258 [details] [diff] [review]
Fire a notification whenever a captive portal check is performed

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

::: toolkit/components/captivedetect/captivedetect.js
@@ +21,5 @@
>  
>  const kOpenCaptivePortalLoginEvent = 'captive-portal-login';
>  const kAbortCaptivePortalLoginEvent = 'captive-portal-login-abort';
>  const kCaptivePortalLoginSuccessEvent = 'captive-portal-login-success';
> +const kCaptivePortalStillCaptive = 'captive-portal-still-captive';

Nit: I think 'captive-portal-check-complete' aligns more with other observer notification names.
Attachment #8755258 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/19d9a93486ab7b7a8b9c4a5b7e78f35fea4f7504
Bug 1273950 - Add pref to check captive portal without waiting for network activity r=MattN

https://hg.mozilla.org/integration/mozilla-inbound/rev/54d08fd41bf0b1d77eb7699d94b07a9e9fd6b03a
Bug 1273950 - Fire a notification whenever a captive portal check is performed r=MattN
Thanks for pinging me. I meant to push it two days ago, but I forgot.
Flags: needinfo?(valentin.gosu)
https://hg.mozilla.org/mozilla-central/rev/19d9a93486ab
https://hg.mozilla.org/mozilla-central/rev/54d08fd41bf0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.