Closed Bug 1313568 Opened 8 years ago Closed 8 years ago

Handle captive portal notification bar per-window

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Iteration:
53.5 - Jan 23
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox52 + verified
firefox53 --- verified

People

(Reporter: nhnt11, Assigned: nhnt11)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxprivacy])

Attachments

(4 files, 1 obsolete file)

Bug 1313568 - Handle captive portal UI in per-window script.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1313568 - Update CaptivePortalWatcher tests to work with per-window handling of captive portal UI.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1313568 - Remove CaptivePortalWatcher.jsm and move tests to browser/base/content/tests/captivePortal.
59 bytes, text/x-review-board-request
MattN
: review+
jcristau
: approval-mozilla-beta+
Details
Folded patch, rebased for beta
35.92 KB, patch
Details | Diff | Splinter Review 
Currently automatic captive portal detection shows a notification bar and opens a tab with the login page in just one window - the currently focused one, or if no browser window is focused, the first one to receive focus.

The notification bar at least should be shown in all windows, and should be handled in a per-window script.
[Tracking Requested - why for this release]: Consistent captive portal experience
tracking-firefox52: --- → ?
Tracking captive portal related issue for 52
tracking-firefox52: ? → +
Figured I'd upload this WIP while bug 989197 goes through review cycles. It has been working fairly well for me. The patch doesn't include tests yet.

Most of the code is reused from CaptivePortalWatcher.jsm, with just a few modifications since this new script runs in each window. I'll document more details in a future comment, along with a more polished patch.
This will work nicely with the ssl error page once it lands; browser.js will simply need to call CaptivePortalWatcher._ensureCaptivePortalTab() when the "Open Login Page" button is clicked. (btw, maybe this file should be renamed to browser-CaptivePortalWatcher.js, hmm)
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Comment on attachment 8816955 [details]
Bug 1313568 - Handle captive portal notification bar per-window.

https://reviewboard.mozilla.org/r/97438/#review100058

I understand a new patch is incoming. Sorry for not noticing this in my queue.

::: browser/base/content/browser-captivePortal.js:22
(Diff revision 2)
> +  // This holds a weak reference to the captive portal tab so that we
> +  // don't leak it if the user closes it.
> +  _captivePortalTab: null,

This patch is different than expected… in my mind we were just moving the notification bar handling code but you're moving everything… I see now that most of the code is UI related so I kinda understand.

One difference is that now we will have a captive portal tab per-window instead of one total. Is that desired? I guess we would stil close all of the opened ones…
Attachment #8816955 - Flags: review?(MattN+bmo)
Blocks: 1325224
Attachment #8816955 - Attachment is obsolete: true
Meh, my brain hurt trying to figure out the approach in the first patch. This patch just moves the UI handling code to a per-window script. Watcher still does the logic to decide when to actually show stuff, but the actual showing is done in a per-window script. All windows will now show the notification. It was easy to account for this in the existing tests.
Comment on attachment 8824747 [details]
Bug 1313568 - Handle captive portal UI in per-window script.

https://reviewboard.mozilla.org/r/103066/#review103698

::: browser/base/content/browser.js:2981
(Diff revision 2)
>  
>      }
>    },
>  
>    onOpenCaptivePortalPage() {
> -    // Open a new tab with the canonical URL that we use to check for a captive portal.
> +    CaptivePortalUIHandler._ensureCaptivePortalTab();

Calling an underscore prefixed method?

::: browser/base/content/global-scripts.inc:13
(Diff revision 2)
>  <script type="application/javascript" src="chrome://browser/content/browser-addons.js"/>
>  <script type="application/javascript" src="chrome://browser/content/browser-ctrlTab.js"/>
> +<script type="application/javascript" src="chrome://browser/content/browser-captivePortal.js"/>
>  <script type="application/javascript" src="chrome://browser/content/browser-customization.js"/>

Please keep these sorted alphabetically
Comment on attachment 8824747 [details]
Bug 1313568 - Handle captive portal UI in per-window script.

https://reviewboard.mozilla.org/r/103066/#review103706

::: browser/base/content/test/captivePortal/browser_captivePortal_certErrorUI.js:55
(Diff revision 4)
>    });
>  
>    let portalTab = yield portalTabPromise;
>    is(gBrowser.selectedTab, portalTab, "Login page should be open in a new foreground tab.");
>  
> +  // Make sure clicking the "Open Login Page" button again focuses the existing portal tab.

New patch tests that the "Open Login Page" button in the error page re-focuses an existing portal tab instead of opening a new one.
Comment on attachment 8824747 [details]
Bug 1313568 - Handle captive portal UI in per-window script.

https://reviewboard.mozilla.org/r/103066/#review104792

::: browser/modules/CaptivePortalWatcher.jsm
(Diff revision 4)
> -  // This holds a weak reference to the captive portal tab so that we
> -  // don't leak it if the user closes it.
> -  _captivePortalTab: null,

I'm confused… I was thinking we were going to do one portal tab globally instead of one per window?

::: browser/modules/CaptivePortalWatcher.jsm:150
(Diff revision 4)
>      if (this._delayedCaptivePortalDetectedInProgress) {
>        Services.obs.removeObserver(this, "xul-window-visible");
>        this._delayedCaptivePortalDetectedInProgress = false;
>      }
>  
> -    this._removeNotification();
> +    Services.obs.notifyObservers(null, "captive-portal-remove-notification", null);

Why can't the front-end use "captive-portal-login-abort" and "captive-portal-login-success" directly?
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #15)
> I'm confused… I was thinking we were going to do one portal tab globally
> instead of one per window?

Really? That wouldn't be a straightforward change at all - what happens when there's already a portal tab in one window and the user clicks the notification bar button? Do we close the tab in the other window? Do we focus the other window? Neither of these options sounds optimal to me.

> Why can't the front-end use "captive-portal-login-abort" and
> "captive-portal-login-success" directly?

OK, I made it work. This latest patchset removes the CaptivePortalWatcher module and handles everything in the per-window script. All the tests still pass! :)
Comment on attachment 8824747 [details]
Bug 1313568 - Handle captive portal UI in per-window script.

https://reviewboard.mozilla.org/r/103066/#review105392

::: browser/base/content/browser-captivePortal.js:17
(Diff revision 5)
> +  /**
> +   * This constant is chosen to be large enough for a portal recheck to complete,
> +   * and small enough that the delay in opening a tab isn't too noticeable.
> +   * Please see comments for _delayedCaptivePortalDetected for more details.
> +   */
> +  PORTAL_RECHECK_DELAY_MS: Preferences.get("captivedetect.portalRecheckDelayMS", 500),

I had to make this pref-able so that the tests could modify it *before* a browser window exists. I also took the opportunity to up the value to 500ms, I think it's still low enough but also allows rechecks to complete much more often.

::: browser/base/content/browser-captivePortal.js:61
(Diff revision 5)
> +      // A captive portal has already been detected.
> +      this._captivePortalDetected();
> +
> +      // Automatically open a captive portal tab if there's no other browser window.
> +      let windows = Services.wm.getEnumerator("navigator:browser");
> +      if (windows.getNext() == window && !windows.hasMoreElements()) {

Is there an easier way to check if there's no other browser window?

::: browser/base/content/browser-captivePortal.js:125
(Diff revision 5)
> +    if (win != Services.ww.activeWindow) {
> +      // The window that got focused was not a browser window.
> +      return;
> +    }
> +    Services.obs.removeObserver(this, "xul-window-visible");
> +    this._delayedCaptivePortalDetectedInProgress = false;

This used to be on line 142, but considering we remove the xul-window-visible observer if this boolean is true in captivePortalGone(), it should live here, since we have an early return on line 129 now.
Comment on attachment 8826778 [details]
Bug 1313568 - Update CaptivePortalWatcher tests to work with per-window handling of captive portal UI.

https://reviewboard.mozilla.org/r/104656/#review105394

::: browser/modules/test/browser_CaptivePortalWatcher.js:22
(Diff revision 1)
>  add_task(function* setup() {
>    yield SpecialPowers.pushPrefEnv({
>      set: [["captivedetect.canonicalURL", CANONICAL_URL],
>            ["captivedetect.canonicalContent", CANONICAL_CONTENT]],
>    });
> -});
> +  RecentWindow._getMostRecentBrowserWindowCopy = RecentWindow.getMostRecentBrowserWindow;

Hacky hacky!

::: browser/modules/test/browser_CaptivePortalWatcher.js:316
(Diff revision 1)
>  
>  for (let testcase of singleRunTestCases) {
>    add_task(testcase);
>  }
> +
> +add_task(function* cleanUp() {

Unhacky unhacky! :P
Iteration: --- → 53.5 - Jan 23
Comment on attachment 8824747 [details]
Bug 1313568 - Handle captive portal UI in per-window script.

https://reviewboard.mozilla.org/r/103066/#review106576

::: browser/base/content/browser.js:2975
(Diff revision 7)
>    onOpenCaptivePortalPage() {
> -    // Open a new tab with the canonical URL that we use to check for a captive portal.
> +    CaptivePortalWatcher.ensureCaptivePortalTab();

I'm not sure why we would keep this stub

::: browser/base/content/browser-captivePortal.js
(Diff revision 7)
> -  // This is the value used to identify the captive portal notification.
> -  PORTAL_NOTIFICATION_VALUE: "captive-portal-detected",

Why move this and delete the comment?

::: browser/base/content/browser-captivePortal.js:31
(Diff revision 7)
> +  // This holds a weak reference to the captive portal tab so that we
> +  // don't leak it if the user closes it.
> +  _captivePortalTab: null,

Why move this?

::: browser/base/content/browser-captivePortal.js:100
(Diff revision 7)
> -    if (!win || win != Services.ww.activeWindow) {
> +    if (win != Services.ww.activeWindow) {
>        this._delayedCaptivePortalDetectedInProgress = true;
>        Services.obs.addObserver(this, "xul-window-visible", false);

Doesn't this mean that every window that isn't the active window will add a xul-window-visible observer? Perhaps we should only do this in the most recent window?
Attachment #8824747 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8826778 [details]
Bug 1313568 - Update CaptivePortalWatcher tests to work with per-window handling of captive portal UI.

https://reviewboard.mozilla.org/r/104656/#review106578
Attachment #8826778 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8826779 [details]
Bug 1313568 - Remove CaptivePortalWatcher.jsm and move tests to browser/base/content/tests/captivePortal.

https://reviewboard.mozilla.org/r/104658/#review106580
Attachment #8826779 - Flags: review?(MattN+bmo) → review+
(In reply to Nihanth Subramanya [:nhnt11] from comment #19)
> (In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from
> comment #15)
> > I'm confused… I was thinking we were going to do one portal tab globally
> > instead of one per window?
> 
> Really? That wouldn't be a straightforward change at all - what happens when
> there's already a portal tab in one window and the user clicks the
> notification bar button? Do we close the tab in the other window? Do we
> focus the other window? Neither of these options sounds optimal to me.

We would focus the other window and existing portal tab. That's how switch-to-tab works with multiple windows already.

I'm find with what you have for now as I can also see concerns about switching windows.
Comment on attachment 8826778 [details]
Bug 1313568 - Update CaptivePortalWatcher tests to work with per-window handling of captive portal UI.

https://reviewboard.mozilla.org/r/104656/#review107264
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7957edd71a11
Handle captive portal UI in per-window script. r=MattN
https://hg.mozilla.org/integration/autoland/rev/e8a4e520dcb7
Update CaptivePortalWatcher tests to work with per-window handling of captive portal UI. r=MattN
https://hg.mozilla.org/integration/autoland/rev/67abe927efae
Remove CaptivePortalWatcher.jsm and move tests to browser/base/content/tests/captivePortal. r=MattN
Comment on attachment 8826779 [details]
Bug 1313568 - Remove CaptivePortalWatcher.jsm and move tests to browser/base/content/tests/captivePortal.

Approval Request Comment
[Feature/Bug causing the regression]: Captive Portal
[User impact if declined]: Without the patches in this bug, the captive portal notification will only be shown in one window (the most recent one).
[Is this code covered by automated tests?]: yes.
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: Not very.
[Why is the change risky/not risky?]: It mainly moves code to a different location and makes a few modifications to functionality. Covered by automated tests.
[String changes made/needed]: none
Attachment #8826779 - Flags: approval-mozilla-aurora?
Nihanth, excluding the negative scenario from bug 1332271, I can see two behaviors:

1. If I have multiple windows opened at the time of connecting the CP wireless, the CP notification bar will be still shown only in the first window focused and not in the others.
2. If I have 1 single window opened and after the wireless CP connection I start opening new windows, each new window shall present the CP notification bar.

Is this the expected functionality for this change? I find it a bit confusing.
Flags: needinfo?(nhnt11)
Comment on attachment 8826779 [details]
Bug 1313568 - Remove CaptivePortalWatcher.jsm and move tests to browser/base/content/tests/captivePortal.

52 is now beta, updating uplift request.
Attachment #8826779 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
(In reply to Adrian Florinescu [:AdrianSV] from comment #56)
> Nihanth, excluding the negative scenario from bug 1332271, I can see two
> behaviors:
> 
> 1. If I have multiple windows opened at the time of connecting the CP
> wireless, the CP notification bar will be still shown only in the first
> window focused and not in the others.
> 2. If I have 1 single window opened and after the wireless CP connection I
> start opening new windows, each new window shall present the CP notification
> bar.
> 
> Is this the expected functionality for this change? I find it a bit
> confusing.

Taking care of this in bug 1333075 as we discussed.
Flags: needinfo?(nhnt11)
Comment on attachment 8826779 [details]
Bug 1313568 - Remove CaptivePortalWatcher.jsm and move tests to browser/base/content/tests/captivePortal.

per-window captive portal notification, beta52+ (all three patches in this bug)
Attachment #8826779 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs rebasing for Beta uplift.
Flags: needinfo?(nhnt11)
Flags: needinfo?(nhnt11)

    
    

    
  
Verified fixed on Windows 10 x64, Mac OS X 10.11 and Ubuntu 14.04 x64 using Firefox 52 Beta 9 (buildID: 20170223185858) and latest Aurora 53.0a2 (2017-02-24).
Status: RESOLVED → VERIFIED

          status-firefox52:
          fixed → verified

          status-firefox53:
          fixed → verified

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: