989193 - Open captive portal automatically in a new tab when detected

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P3)

Description

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Captive portal login automatically opened (designed by vtsatskin)

Note: Implementation followup from design Bug 878566
Spec: https://github.com/vtsatskin/FX-Captive-Portals-Design/blob/master/design/spec.md

As part of Firefox's Captive Portal Design, this bug opens a tab automatically in the active Firefox window, to the right of all tabs.

Acceptance Criteria:

- If a captive portal is detected, a new tab is automatically created in the user's active Firefox window displaying the captive portal
- Tab opens to the right of all existing tabs
- If a captive portal is detected while the user is actively using a tab, the captive portal tab is not created until the user either a) switches to a new window, where the captive portal is opened in a new tab or b) leaves Firefox, and upon returning finds the captive portal is opened in their most recently active window

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: MozReview Request: Bug 989193 - Open captive portal automatically in a new tab when detected. r=MattN

Review commit: https://reviewboard.mozilla.org/r/49431/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49431/

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8746433 [details]
MozReview Request: Bug 989193 - Open captive portal automatically in a new tab when detected. r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49431/diff/1-2/

Comment 3

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/49431/#review46263

Awesome, excited to see progress. I have some drive-by comments:

::: browser/modules/CaptivePortalWatcher.jsm:7
(Diff revision 2)
> +var Cc = Components.classes;
> +var Ci = Components.interfaces;
> +var Cu = Components.utils;

Nit: use destructuring with the 4 common properties

::: browser/modules/CaptivePortalWatcher.jsm:24
(Diff revision 2)
> +
> +  _captivePortalTab: null,
> +

This should probably be a weak reference to handle the case of the user closing the tab manually before login. See Cu.getWeakReference

::: browser/modules/CaptivePortalWatcher.jsm:62
(Diff revision 2)
> +        this._captivePortalTab.gBrowser = win.gBrowser;
> +        if (!win.document.hasFocus()) {
> +          win.gBrowser.selectedTab = this._captivePortalTab;
> +        }

Could you explain what the hasFocus check is for in a comment as it's non-obvious to me and I suspect it's incorrect:

IIUC, this is currently saying: "if the browser window (browser.xul document) is not focused, then select the captive portal tab" but I don't see how the condition is related to the action.

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8746433 [details]
MozReview Request: Bug 989193 - Open captive portal automatically in a new tab when detected. r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49431/diff/2-3/

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8746433 [details]
MozReview Request: Bug 989193 - Open captive portal automatically in a new tab when detected. r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49431/diff/3-4/

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

OK, this automatically opens a new tab with the canonical URL when a captive portal is detected, and the tab is automatically closed when we detect that login is completed.

There are a couple of issues, but not front-end side I think.

1. captivedetect.js polls every 3 seconds (by default, pref can be changed) to check if login is complete. This results in "success" being shown in the captive portal tab for 3 seconds, if the captive portal redirects the user back to the originally requested page after login.

2. Currently, there's no real way to know *what* caused us to check for a captive portal. Was it a cert error? Did we check at startup? Was it after a network state change? I think the context is important for UX - if it was a cert error or at startup, we likely want to immediately focus the captive portal tab. The way things currently work, CaptivePortalService waits for something else to tell it to trigger a recheck, but the Service doesn't maintain state for what called it. I think this would be a lot simpler if the Service itself watched for network changes and cert errors instead of relying on external callers to trigger it (which, honestly, could be made part of the nsICaptivePortalDetector implementation directly instead of having a separate Service).

BUT I digress! These issues are likely not blockers for a first-step feature, but they are things I am thinking about for follow up bugs.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Hi Nihanth, are you currently working on this? Let us know if there's something we could help with.

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

Yes, I am. Assigning myself. I just saw Patrick's comment, bug 562917 comment #73. Can you let me know if you will be adding retest-on-cert-error soon? I would be happy to work on it myself if you'd like to mentor me (I suspect the already attached patch in bug 1048131 needs to be adapted?).

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8746433 [details]
MozReview Request: Bug 989193 - Open captive portal automatically in a new tab when detected. r=MattN

https://reviewboard.mozilla.org/r/49431/#review48963

Looks pretty good though please add some tests using mocked observer notifications.

::: browser/components/nsBrowserGlue.js:781
(Diff revision 4)
> +    CaptivePortalWatcher.init();
> +

It seems like you should just move this to `_onFirstWindowLoaded` in this file then you can have init add the `captive-portal-*` observers.

This avoids the complexity of adding your own "browser-delayed-startup-finished" listener in init.

::: browser/modules/CaptivePortalWatcher.jsm:32
(Diff revision 4)
> +
> +  init() {
> +    Services.obs.addObserver(this, "browser-delayed-startup-finished", false);
> +  },
> +
> +  destroy() {

This is never called AFAICS

::: browser/modules/CaptivePortalWatcher.jsm:41
(Diff revision 4)
> +    switch(topic) {
> +    case "browser-delayed-startup-finished":

Nit: indent the `switch` contents two spaces

::: browser/modules/CaptivePortalWatcher.jsm:98
(Diff revision 4)
> +    // Check parentNode in case the object hasn't been gc'd yet.
> +    if (!tab || !tab.parentNode) {

I think we should add `|| tab.closing` before `|| !tab.parentNode`

::: browser/modules/CaptivePortalWatcher.jsm:104
(Diff revision 4)
> +    // If after the login, the captive portal has redirected to some other page,
> +    // leave it open.
> +    if (tab.linkedBrowser.currentURI.spec != this._canonicalURL) {
> +      return;
> +    }

One case to consider is the interaction with OS-level portal detection. Consider the following on OS X:

1) OS X detects a captive portal and opens its window
2) Firefox detects a captive portal (and selects the portal login tab since the application isn't focused).
3) Users logs into the portal via the OS X dialog since it's on top.
4) The OS X dialog closes (manually or automatically)
5) The user now sees the login page in Firefox as we didn't close it since it's not displaying the canonical URL.

Can you remind me why we don't want to close in this case? If it's just "the captive portal has redirected to some other page" then why is that useful for the user?

Comment 10

[Valentin Gosu [:valentin] (he/him)]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #9)
> 3) Users logs into the portal via the OS X dialog since it's on top.
> 4) The OS X dialog closes (manually or automatically)
> 5) The user now sees the login page in Firefox as we didn't close it since
> it's not displaying the canonical URL.
> 
> Can you remind me why we don't want to close in this case? If it's just "the
> captive portal has redirected to some other page" then why is that useful
> for the user?

I agree, that this is indeed an issue. Users could log in using a different method/browser, leading to the captive-portal-login-success. The general idea is that we would close that window, and just return the user to his previous tab, but sometimes the CP page may display important info, such as how much time you have to browse or etc.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

(In reply to Nihanth Subramanya [:nhnt11] from comment #8)
> Can you let me know if you will be adding retest-on-cert-error
> soon? I would be happy to work on it myself if you'd like to mentor me (I
> suspect the already attached patch in bug 1048131 needs to be adapted?).

I just posted a patch in bug 816866 which triggers rechecks on cert errors.

Comment 12

[Dale Harvey (:daleharvey)]

Hi Nihanth, I got asked to take a look at this bug and it looks like you are on top of things. Are you planning to implement the full spec or do you want a hand? Happy to help out if you need.

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8746433 [details]
MozReview Request: Bug 989193 - Open captive portal automatically in a new tab when detected. r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49431/diff/4-5/

Comment 14

[(inactive) Nihanth Subramanya [:nhnt11]]

MattN and I discussed this on IRC and decided to try the following if implementation didn't turn out to be too difficult:

- If Firefox is focused when we detect the portal, add the tab immediately and don't focus it.
- If not, wait till a browser window regains focus. Trigger a portal recheck. Wait for a short delay, then:
  -- If the recheck completed and the portal is still locked, add the tab and focus it immediately.
  -- If the recheck hasn't completed, add the tab but don't focus it - once the recheck completes, if the portal is unlocked, the tab will be closed - we don't the tab contents to flicker.

This wasn't too hard to implement, I used a delay of 150ms - we could probably reduce this further, but I don't have any real world data to estimate how long these checks typically take. I'll see if I can pop into a Starbucks and get some stats. What do you think?

Comment 15

[(inactive) Nihanth Subramanya [:nhnt11]]

This would be much cleaner if we had a way to know when a portal recheck completes. I see that there's an interface for a callback (https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsICaptivePortalService.idl#8), but it doesn't seem to be used. I'll file a bug for this.

Comment 16

[Valentin Gosu [:valentin] (he/him)]

(In reply to Nihanth Subramanya [:nhnt11] from comment #15)
> This would be much cleaner if we had a way to know when a portal recheck
> completes. I see that there's an interface for a callback
> (https://dxr.mozilla.org/mozilla-central/source/netwerk/base/
> nsICaptivePortalService.idl#8), but it doesn't seem to be used. I'll file a
> bug for this.

We initially considered using callbacks for checking the status of captive portals, but that becomes extremely complex once you have a bunch of modules that need to query the status of the CP.
It is best to observe the notifications. If you really need to know when the recheck completes, you could instantiate a nsICaptivePortalDetector and perform the check yourself. That shouldn't interfere with the captive portal service.

Comment 17

[Valentin Gosu [:valentin] (he/him)]

(In reply to Nihanth Subramanya [:nhnt11] from comment #14)
> MattN and I discussed this on IRC and decided to try the following if
> implementation didn't turn out to be too difficult:
> 
> - If Firefox is focused when we detect the portal, add the tab immediately
> and don't focus it.

I assume in this case we would display the status bar (in a follow-up bug)
  https://github.com/vtsatskin/FX-Captive-Portals-Design/blob/master/design/spec.md#a-captive-portal-is-detected-but-not-logged-in

> - If not, wait till a browser window regains focus. Trigger a portal
> recheck. Wait for a short delay, then:
>   -- If the recheck completed and the portal is still locked, add the tab
> and focus it immediately.

If the window is not focused, when we get the "captive-portal-login" event is received, we don't need to recheck. We are certain that at this moment, we are definitely in a captive portal, so we can just add the tab and focus it.

>   -- If the recheck hasn't completed, add the tab but don't focus it - once
> the recheck completes, if the portal is unlocked, the tab will be closed -
> we don't the tab contents to flicker.

As I said, we are definitely in a captive portal and don't need to recheck. If we are still unfocused when we receive "captive-portal-login-success" we should probably close the opened window.

Comment 18

[(inactive) Nihanth Subramanya [:nhnt11]]

> > - If Firefox is focused when we detect the portal, add the tab immediately
> > and don't focus it.
> 
> I assume in this case we would display the status bar (in a follow-up bug)
>  
> https://github.com/vtsatskin/FX-Captive-Portals-Design/blob/master/design/
> spec.md#a-captive-portal-is-detected-but-not-logged-in

Yes, that's the plan.

> > - If not, wait till a browser window regains focus. Trigger a portal
> > recheck. Wait for a short delay, then:
> >   -- If the recheck completed and the portal is still locked, add the tab
> > and focus it immediately.
> 
> If the window is not focused, when we get the "captive-portal-login" event
> is received, we don't need to recheck. We are certain that at this moment,
> we are definitely in a captive portal, so we can just add the tab and focus
> it.
> 
> >   -- If the recheck hasn't completed, add the tab but don't focus it - once
> > the recheck completes, if the portal is unlocked, the tab will be closed -
> > we don't the tab contents to flicker.
> 
> As I said, we are definitely in a captive portal and don't need to recheck.
> If we are still unfocused when we receive "captive-portal-login-success" we
> should probably close the opened window.

captivedetect.js monitors HTTP activity to trigger rechecks. We can't rely on receiving the login-success notification when Firefox isn't focused. I tested this with the debug flag in captivedetect.js set to true, and logging into the portal from a different client definitely doesn't get picked up until there's some http activity in Firefox.

Comment 19

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Valentin Gosu [:valentin] from comment #16)
> (In reply to Nihanth Subramanya [:nhnt11] from comment #15)
> > This would be much cleaner if we had a way to know when a portal recheck
> > completes. I see that there's an interface for a callback
> > (https://dxr.mozilla.org/mozilla-central/source/netwerk/base/
> > nsICaptivePortalService.idl#8), but it doesn't seem to be used. I'll file a
> > bug for this.
> 
> We initially considered using callbacks for checking the status of captive
> portals, but that becomes extremely complex once you have a bunch of modules
> that need to query the status of the CP.
> It is best to observe the notifications. If you really need to know when the
> recheck completes, you could instantiate a nsICaptivePortalDetector and
> perform the check yourself. That shouldn't interfere with the captive portal
> service.

Ideally, the service should maintain a promise indicating whether a recheck is going on right now or not - it can return a reference to this promise to callers of recheckCaptivePortal, and the callers can wait on it. The promise should be resolved in the complete() callback. Having said that, I have no idea if we can use Promises between JS and C++ code, or even if we can use Promises at all in C++ code.

Comment 20

[(inactive) Nihanth Subramanya [:nhnt11]]

Another idea, likely very simple to implement: what do you think about firing an observer notification from the CaptivePortalService whenever a recheck is complete? The notification would either indicate a state change, or confirm the lack of one.

Comment 21

[Valentin Gosu [:valentin] (he/him)]

(In reply to Nihanth Subramanya [:nhnt11] from comment #20)
> Another idea, likely very simple to implement: what do you think about
> firing an observer notification from the CaptivePortalService whenever a
> recheck is complete? The notification would either indicate a state change,
> or confirm the lack of one.

I think that's definitely possible. I filed bug 1273950 and intend to fix this soon.

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8746433 [details]
MozReview Request: Bug 989193 - Open captive portal automatically in a new tab when detected. r=MattN

https://reviewboard.mozilla.org/r/49431/#review50514

This is still lacking a test.

::: browser/modules/CaptivePortalWatcher.jsm:23
(Diff revisions 4 - 5)
>                                     "nsICaptivePortalService");
>  
>  this.CaptivePortalWatcher = {
>    _firstWindowLoaded: false,
>  
> +  // This should be a weak reference to the tab so that we

s/This should be/Holds/

As-is I read it as a suggestion to improve the property in the future.

::: browser/modules/CaptivePortalWatcher.jsm:85
(Diff revisions 4 - 5)
> +    let observer = (subject, topic) => {
> +      let win = RecentWindow.getMostRecentBrowserWindow();
> -    if (!win.document.hasFocus()) {
> +      if (!win.document.hasFocus()) {
> +        // The document that got focused was not in a browser window.

This observer is complicated enough that I think it should go in its own function (from a quick skim I didn't see it using variables in the `_addCaptivePortalTab` scope) and you may as well also use the modules `observe` method instead of having multiple different nsIObserver implementers.

::: browser/modules/CaptivePortalWatcher.jsm:101
(Diff revisions 4 - 5)
> +      cps.recheckCaptivePortal();
> +
> +      // We wait for kDelayPeriod milliseconds after the trigger.
> +      // - If the portal is no longer locked, we don't need to add a tab.
> +      // - If it is, the delay is chosen to not be extremely noticeable.
> +      const kDelayPeriod = 150;

Move this to the top of the file on line 8 after the other const and use uppercase and underscore style which is what we use for new code in JS. I would also then make it a bit more descriptive so we know what the delay is for and we like to include units via the suffix e.g. `…_MS`

::: browser/modules/CaptivePortalWatcher.jsm:101
(Diff revisions 4 - 5)
> +      cps.recheckCaptivePortal();
> +
> +      // We wait for kDelayPeriod milliseconds after the trigger.
> +      // - If the portal is no longer locked, we don't need to add a tab.
> +      // - If it is, the delay is chosen to not be extremely noticeable.
> +      const kDelayPeriod = 150;

Move this to the top of the file on line 8 after the other const and use uppercase and underscore style which is what we use for new code in JS. I would also then make it a bit more descriptive so we know what the delay is for and we like to include units via the suffix e.g. `…_MS`

::: browser/modules/CaptivePortalWatcher.jsm:106
(Diff revisions 4 - 5)
> +      const kDelayPeriod = 150;
> +
> +      // Set _addingTab to true to avoid the rare case when something
> +      // else also triggers a recheck and we get a login notification
> +      // within the delay period, resulting in an extra tab.
> +      this._addingTab = true;

This should be defined beside `_captivePortalTab` at the top of the object.

::: browser/modules/CaptivePortalWatcher.jsm:109
(Diff revisions 4 - 5)
> +      // else also triggers a recheck and we get a login notification
> +      // within the delay period, resulting in an extra tab.
> +      this._addingTab = true;
> +
> +      setTimeout(() => {
> +        delete this._addingTab;

Just use a boolean as it's clearer (like I said elsewhere)

::: browser/modules/CaptivePortalWatcher.jsm:127
(Diff revisions 4 - 5)
> -    // Keep a weak reference to the tab so that we don't leak it if the
> -    // user closes it.
> -    this._captivePortalTab = Cu.getWeakReference(tab);
> +        this._captivePortalTab = Cu.getWeakReference(tab);
> +      }, kDelayPeriod);
> +    }
> +    Services.obs.addObserver(observer, "document-shown", false);

It seems like this won't get removed if the tab gets closed without it being shown. We could perhaps remove if this is ever called and `this._captivePortalTab` doesn't have a tab or watch for the tabclose.

Also, make sure this works with e10s. Your test should check this.

::: browser/modules/CaptivePortalWatcher.jsm:21
(Diff revision 5)
> +XPCOMUtils.defineLazyServiceGetter(this, "cps",
> +                                   "@mozilla.org/network/captive-portal-service;1",
> +                                   "nsICaptivePortalService");
> +
> +this.CaptivePortalWatcher = {
> +  _firstWindowLoaded: false,

This is now unused

::: browser/modules/CaptivePortalWatcher.jsm:27
(Diff revision 5)
> +
> +  // This should be a weak reference to the tab so that we
> +  // don't leak it if the user closes it.
> +  _captivePortalTab: null,
> +
> +  get _canonicalURL() {

Nit: this probably doesn't need to be private and that makes it ever so slightly nicer for your tests to use.

::: browser/modules/CaptivePortalWatcher.jsm:28
(Diff revision 5)
> +  // This should be a weak reference to the tab so that we
> +  // don't leak it if the user closes it.
> +  _captivePortalTab: null,
> +
> +  get _canonicalURL() {
> +    return Services.prefs.getCharPref("captivedetect.canonicalURL");

Note that this pref should be overwritten with a fake testing one in testing/profiles/prefs_general.js to that we don't touch the network during testing.

::: browser/modules/CaptivePortalWatcher.jsm:76
(Diff revision 5)
> +    // If the browser is in use, add the tab without focusing it.
> +    if (win.document.hasFocus()) {

Nit: s/focusing/selecting/ is slightly more clear and reduce confusion with the .hasFocus

::: browser/modules/CaptivePortalWatcher.jsm:83
(Diff revision 5)
> +    // If another application has focus, open and focus the new tab
> +    // when we regain focus.

Note that line 65 is specifically looking for a browser window but then you assume that another application is focused here but it's possible that the most recent window is a Firefox window that isn't a browser window (e.g. page info, Places manager, pwmgr, etc.). Please fix/clarify this. I think you should probably use getMostRecentBrowserWindow if you actually care about what application is focused.

::: browser/modules/CaptivePortalWatcher.jsm:103
(Diff revision 5)
> +      // Set _addingTab to true to avoid the rare case when something
> +      // else also triggers a recheck and we get a login notification
> +      // within the delay period, resulting in an extra tab.
> +      this._addingTab = true;

This seems like a misleading variable name since sometimes we have an early return without adding a tab.

::: browser/modules/CaptivePortalWatcher.jsm:127
(Diff revision 5)
> +        }
> +
> +        this._captivePortalTab = Cu.getWeakReference(tab);
> +      }, kDelayPeriod);
> +    }
> +    Services.obs.addObserver(observer, "document-shown", false);

I doubt this does what you want from skimming the implementation. It seems to be referring to a documents initial showing. Is that not the case?

Comment 23

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/49431/#review50780

::: browser/modules/CaptivePortalWatcher.jsm:67
(Diff revision 5)
> +      // Wait for a new window to be opened.
> +      let observer = (subject, topic) => {
> +        Services.obs.removeObserver(observer, topic);
> +        this._addCaptivePortalTab();
> +      }
> +      Services.obs.addObserver(observer, "browser-delayed-startup-finished", false);
> +      return;
> +    }

Um… where do we actually open the window? Please manually test this.

Comment 24

[(inactive) Nihanth Subramanya [:nhnt11]]

https://reviewboard.mozilla.org/r/49431/#review50514

> Note that line 65 is specifically looking for a browser window but then you assume that another application is focused here but it's possible that the most recent window is a Firefox window that isn't a browser window (e.g. page info, Places manager, pwmgr, etc.). Please fix/clarify this. I think you should probably use getMostRecentBrowserWindow if you actually care about what application is focused.

If the most recent window is a non-browser Firefox window, then we still wait for a browser window to be opened. At line 77 we are sure that |win| is definitely a browser window.

> This seems like a misleading variable name since sometimes we have an early return without adding a tab.

Would you like "_waitingToAddTab" or something? I don't feel too strongly.

> I doubt this does what you want from skimming the implementation. It seems to be referring to a documents initial showing. Is that not the case?

I found this notification by adding an observer for "*" and manually switching focus back and forth between Firefox windows and other applications. I noticed two notifications every time a browser window was focused: xul-window-visible and document-shown. I chose document-shown because I felt it would be too soon to do anything at xul-window-shown: what if we don't have access to gBrowser yet for example? I'm going to investigate this a little more to be sure it does what I want.

Comment 25

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #24)
> https://reviewboard.mozilla.org/r/49431/#review50514
> 
> > Note that line 65 is specifically looking for a browser window but then you assume that another application is focused here but it's possible that the most recent window is a Firefox window that isn't a browser window (e.g. page info, Places manager, pwmgr, etc.). Please fix/clarify this. I think you should probably use getMostRecentBrowserWindow if you actually care about what application is focused.
> 
> If the most recent window is a non-browser Firefox window, then we still
> wait for a browser window to be opened. At line 77 we are sure that |win| is
> definitely a browser window.
>

Never mind, I see what you mean. I'll change the comment, I think the functionality is still correct (wait for a browser window to get focus).

Comment 26

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8746433 [details]
MozReview Request: Bug 989193 - Open captive portal automatically in a new tab when detected. r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49431/diff/5-6/

Comment 27

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8746433 [details]
MozReview Request: Bug 989193 - Open captive portal automatically in a new tab when detected. r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49431/diff/6-7/

Comment 28

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8746433 [details]
MozReview Request: Bug 989193 - Open captive portal automatically in a new tab when detected. r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49431/diff/7-8/

Comment 29

[Matthew N. [:MattN]]

Comment on attachment 8746433 [details]
MozReview Request: Bug 989193 - Open captive portal automatically in a new tab when detected. r=MattN

https://reviewboard.mozilla.org/r/49431/#review52098

Other than the question about "captive-portal-login-abort", I think is fine for an initial landing. We will simplify this a bit once we have the new obs. notification and I suspect we may want to tweak the UX related to focus but that can be handled in follow-ups/

If you're ready for Nightly testing with this, please also flip the service pref on for desktop (perhaps in an ifdef for Nightly or non-release if you think it won't be ready to ride the trains in 49).

::: browser/modules/CaptivePortalWatcher.jsm:137
(Diff revisions 5 - 8)
> -      }, kDelayPeriod);
> +    }, PORTAL_RECHECK_DELAY_MS);
> -    }
> -    Services.obs.addObserver(observer, "document-shown", false);
>    },
>  
> -  _afterCaptivePortalLogin() {
> +  _afterCaptivePortalFreed() {

Nit: What about `_captivePortalGone`?

::: browser/modules/CaptivePortalWatcher.jsm:43
(Diff revision 8)
> +
> +  init() {
> +    Services.obs.addObserver(this, "captive-portal-login", false);
> +    Services.obs.addObserver(this, "captive-portal-login-abort", false);
> +    Services.obs.addObserver(this, "captive-portal-login-success", false);
> +    this._initialized = true;

Nit: declare this after `_captivePortalTab` above (so they're alphabetical).

::: browser/modules/CaptivePortalWatcher.jsm:45
(Diff revision 8)
> +    Services.obs.addObserver(this, "captive-portal-login", false);
> +    Services.obs.addObserver(this, "captive-portal-login-abort", false);
> +    Services.obs.addObserver(this, "captive-portal-login-success", false);
> +    this._initialized = true;
> +    if (cps.state == cps.LOCKED_PORTAL) {
> +      // A captive portal has already been detected after startup.

Nit: the "after startup" feels wrong to me here.

::: browser/modules/CaptivePortalWatcher.jsm:64
(Diff revision 8)
> +      case "captive-portal-login-abort":
> +      case "captive-portal-login-success":
> +        this._afterCaptivePortalFreed();

Can you remind me when a "captive-portal-login-abort" happens? I'm trying to figure out whether it makes sense to treat these the same.

::: browser/modules/CaptivePortalWatcher.jsm:80
(Diff revision 8)
> +    // If there's no browser window or none have focus, open and show the
> +    // tab when we regain focus.
> +    if (!win || !win.document.hasFocus()) {

Could you document the rationale for this as it's non-obvious. I really think we should have started with the notification bar as it's simpler… This module is getting more complex than I feel it should be due to these focus rules. Perhaps those weren't necessary for a v1.

::: browser/modules/CaptivePortalWatcher.jsm:94
(Diff revision 8)
> +  // Called after we regain focus if we detect a portal while a browser window
> +  // doesn't have focus. Triggers a portal recheck to reaffirm state, and adds
> +  // the tab if needed after a short delay to allow the recheck to complete.
> +  _delayedAddCaptivePortalTab() {

Nit: Use JSDoc style with `/**  */`

Comment 30

[Matthew N. [:MattN]]

Oh, but again, I would like tests before landing.

Comment 31

[(inactive) Nihanth Subramanya [:nhnt11]]

https://reviewboard.mozilla.org/r/49431/#review52098

> Can you remind me when a "captive-portal-login-abort" happens? I'm trying to figure out whether it makes sense to treat these the same.

This is fired when whatever requested captivedetect.js to do a portal recheck changes its mind and asks it to abort. I basically interpreted it as loss of state, so I figured we should treat it as a reset.

Comment 32

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: MozReview Request: Bug 989193 - Add tests for CaptivePortalWatcher module. r=MattN

Review commit: https://reviewboard.mozilla.org/r/56086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56086/

Comment 33

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8746433 [details]
MozReview Request: Bug 989193 - Open captive portal automatically in a new tab when detected. r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49431/diff/8-9/

Comment 34

[(inactive) Nihanth Subramanya [:nhnt11]]

https://reviewboard.mozilla.org/r/49431/#review50514

> Note that this pref should be overwritten with a fake testing one in testing/profiles/prefs_general.js to that we don't touch the network during testing.

Oh, interesting, I missed this - current patch doesn't use the file you mentioned.

Comment 35

[(inactive) Nihanth Subramanya [:nhnt11]]

https://reviewboard.mozilla.org/r/56086/#review52730

::: browser/app/profile/firefox.js:1419
(Diff revision 1)
>  // Enable browser frames for use on desktop.  Only exposed to chrome callers.
>  pref("dom.mozBrowserFramesEnabled", true);
>  
>  pref("extensions.pocket.enabled", true);
> +
> +pref("network.captive-portal-service.enabled", true);

This should have gone into the other patch, sorry :(

Comment 36

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/49431/#review53368

::: browser/modules/CaptivePortalWatcher.jsm:8
(Diff revisions 8 - 9)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> -// This constant is chosen to be large enough for a portal recheck to complete,
> +/* This constant is chosen to be large enough for a portal recheck to complete,

This should be `/**`

::: browser/modules/CaptivePortalWatcher.jsm:14
(Diff revisions 8 - 9)
> -// and small enough that the delay in opening a tab isn't too noticeable.
> -// Please see comments for _delayedAddCaptivePortalTab for more details.
> + * and small enough that the delay in opening a tab isn't too noticeable.
> + * Please see comments for _delayedAddCaptivePortalTab for more details. */
>  const PORTAL_RECHECK_DELAY_MS = 150;
> +// We listen for this observer notification to detect when a browser window
> +// gains focus.
> +const BROWSER_FOCUS_NOTIFICATION = "xul-window-visible";

Personally I think this indirection just makes it harder to find code using the notification. I would inline all of these usages.

Comment 37

[Matthew N. [:MattN]]

Comment on attachment 8757688 [details]
MozReview Request: Bug 989193 - Add tests for CaptivePortalWatcher module. r=MattN

https://reviewboard.mozilla.org/r/56086/#review53372

I didn't have time to review the whole test yet

::: browser/modules/test/browser_CaptivePortalWatcher.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Nit: tests are normally public domain, not MPL2 and they get that license by default without the header so you can omit this.

::: browser/modules/test/browser_CaptivePortalWatcher.js:11
(Diff revision 1)
> +Services.prefs.setCharPref("captivedetect.canonicalURL", CANONICAL_URL);
> +Services.prefs.setCharPref("captivedetect.canonicalContent", CANONICAL_CONTENT);

These aren't getting cleaned up at the end of the test file. Please use SpecialPowers.pushPrefEnv

::: browser/modules/test/browser_CaptivePortalWatcher.js:17
(Diff revision 1)
> +function* portalDetectedNoBrowserWindow() {
> +  let getMostRecentBrowserWindow = RecentWindow.getMostRecentBrowserWindow;
> +  RecentWindow.getMostRecentBrowserWindow = () => {};
> +  Services.obs.notifyObservers(null, "captive-portal-login", null);
> +  RecentWindow.getMostRecentBrowserWindow = getMostRecentBrowserWindow;
> +}

You're not restoring this function's body with registerCleanupFunction so this is going to break other tests run after it that use the function.

::: browser/modules/test/browser_CaptivePortalWatcher.js:39
(Diff revision 1)
> +    "captive-portal-login-" + (aSuccess ? "success" : "abort"), null);
> +}
> +
> +// Each of the test cases below is run twice: once for login-success and once
> +// for login-abort (aSuccess set to true and false respectively).
> +var testCasesForBothSuccessAndAbort = [

Nit: s/var/let/

::: browser/modules/test/browser_CaptivePortalWatcher.js:101
(Diff revision 1)
> +    is(win.gBrowser.tabs.length, 1,
> +      "The portal tab should have been closed.");
> +  },
> +];
> +
> +var singleRunTestCases = [

Nit: s/var/let/

::: browser/modules/test/browser_CaptivePortalWatcher.js:101
(Diff revision 1)
> +var singleRunTestCases = [
> +  /* A portal is detected when there's no browser window,
> +   * then a browser window is opened, and the portal is logged into
> +   * and redirects to a different page. The portal tab should be added
> +   * and focused when the window is opened, and left open after login
> +   * since it redirected. */
> +  function* () {

Please name all of the task functions and use the "test_" prefix so the log output will indicate which function is being run.

::: browser/modules/test/browser_CaptivePortalWatcher.js:162
(Diff revision 1)
> +    BrowserTestUtils.loadURI(browser, CANONICAL_URL_REDIRECTED);
> +    yield loadPromise;
> +    freePortal(true);
> +    is(win.gBrowser.tabs.length, 2,
> +      "The portal tab should not have been closed.");
> +    win.gBrowser.removeTab(tab);

You should use the BrowserTestUtils equivalent in new code as it properly handles e10s async closing.

::: browser/modules/test/browser_CaptivePortalWatcher.js:166
(Diff revision 1)
> +testCasesForBothSuccessAndAbort.forEach(aCase => {
> +  add_task(function* () {
> +    yield aCase(true);
> +    yield aCase(false);
> +  });
> +});
> +
> +singleRunTestCases.forEach(aCase => {
> +  add_task(aCase);
> +});

Prefer for…of loops as they're more natural to read.

::: browser/modules/test/browser_CaptivePortalWatcher.js:166
(Diff revision 1)
> +testCasesForBothSuccessAndAbort.forEach(aCase => {
> +  add_task(function* () {
> +    yield aCase(true);
> +    yield aCase(false);
> +  });
> +});

Name all of the methods in the testCasesForBothSuccessAndAbort array to describe them with the "test\_" prefix and then do the following so those messages are logged properly:

```
for (let case of testCasesForBothSuccessAndAbort) {
  add_task(case.bind(null, true));
  info("Running again with an abort");
  add_task(case.bind(null, false));
}
```

Comment 38

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/56086/#review55700

I've addressed all of the comments and will be pushing to try soon.

::: browser/modules/test/browser_CaptivePortalWatcher.js:8
(Diff revision 1)
> +const CANONICAL_URL = "data:text/plain;," + CANONICAL_CONTENT;
> +const CANONICAL_URL_REDIRECTED = "data:text/plain;,redirected";

Adding a charset to these data URIs gets rid of console warning spew.

Comment 39

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/56086/#review53372

> You're not restoring this function's body with registerCleanupFunction so this is going to break other tests run after it that use the function.

I think I missed that you are restoring it after the notifyObservers call. That's good enough as there's not a trivial registerCleanupFunction solution how you have the code structured.

Comment 40

[Matthew N. [:MattN]]

Attachment: Bug 989193 - Open captive portal automatically in a new tab when detected.

Review commit: https://reviewboard.mozilla.org/r/58838/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58838/

Comment 41

[Matthew N. [:MattN]]

Attachment: Bug 989193 - Add tests for CaptivePortalWatcher module.

Review commit: https://reviewboard.mozilla.org/r/58840/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58840/

Comment 42

[Matthew N. [:MattN]]

There are test failures that I haven't had a chance to look into yet: https://treeherder.mozilla.org/#/jobs?repo=try&revision=118fda85d38cd7589aa65484dbc23253804ae6f0

Comment 43

[Matthew N. [:MattN]]

I think the problem is related to hasFocus not being true on Linux when the xul-... notification arrives. rs=me to disable the test or feature on Linux for now.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/0e70610a35f6
Open captive portal automatically in a new tab when detected. r=MattN
https://hg.mozilla.org/integration/fx-team/rev/000f0d293210
Add tests for CaptivePortalWatcher module. r=MattN

Comment 45

[Panos Astithas (he/him) [:past] (please ni?)]

Pushed with the test disabled on Linux for now. I'll file a followup to fix that.

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e70610a35f6
https://hg.mozilla.org/mozilla-central/rev/000f0d293210

Comment 47

[Matthew N. [:MattN]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Helps users login to networks (e.g. coffee shops, airports, hotels, etc.) that redirect requests to a login page.
[Suggested wording]: Automatically open a login page for networks requiring authentication.
[Links (documentation, blog post, etc)]: None

Comment 48

[Marcia Knous [:marcia]]

Tracking 50+ for this authentication improvement.

Comment 49

[Nick Nguyen [:osunick]]

Adding pdol and myself to this bug. After two weeks in a hotel it is making me cranky. Let's make sure it gets into 50.

Comment 50

[Matthew N. [:MattN]]

Nick, have you seen this feature working on Nightly 50? I believe Nihanth is now working on bug 989194 which should also make 50 and then I think that will be an okay MVP for the feature. Do you agree?

Comment 51

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx50 (Aurora) release notes.

Comment 52

[Matthew N. [:MattN]]

This was made Nightly-only right above your comment (bug 1285265).

Comment 53

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Matthew N. [:MattN] from comment #52)
> This was made Nightly-only right above your comment (bug 1285265).

Got it. I'll remove it from Fx50 release notes. Sorry I missed that somehow.

Comment 54

[Julien Cristau [:jcristau]]

Updating status flags as this is disabled in 51, but should be in 52.