Abdoulaye O. LY Assignee
	Description • 6 years ago

Actually with grouped closure of tabs (removeTabsToTheEndFrom/removeAllTabsBut...), the tabs to close are computed twice. This can be avoided by giving warnAboutClosingTabs the number of tabs to close instead of computing it.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 3 • 6 years ago
mozreview-review

Comment on attachment 8993082 [details]
Bug 1475427 - Refactor warnAboutClosingTabs function in tabbrowser.js so that it directly takes the number of tabs to close as param.

https://reviewboard.mozilla.org/r/257886/#review264824

::: browser/base/content/browser.js:6986
(Diff revision 2)
>  function warnAboutClosingWindow() {
>    // Popups aren't considered full browser windows; we also ignore private windows.
>    let isPBWindow = PrivateBrowsingUtils.isWindowPrivate(window) &&
>          !PrivateBrowsingUtils.permanentPrivateBrowsing;
> +
> +  let closingTabs = gBrowser.tabs.length - gBrowser._removingTabs.length;

Pinned tabs should be removed too.

::: browser/base/content/browser.js:7020
(Diff revision 2)
>                                   "last-pb-context-exiting");
>      if (exitingCanceled.data)
>        return false;
>    }
>  
> +

Please remove this blank line since there is already one above it.

::: browser/components/nsBrowserGlue.js:1407
(Diff revision 2)
>  
>      let win = BrowserWindowTracker.getTopWindow();
>  
>      // warnAboutClosingTabs checks browser.tabs.warnOnClose and returns if it's
>      // ok to close the window. It doesn't actually close the window.
> +    let closingTabs = win.gBrowser.tabs.length - win.gBrowser._removingTabs.length;

closingTabsEnum.ALL also subtracted the pinned tabs.

	Abdoulaye O. LY Assignee
	Comment 4 • 6 years ago
mozreview-review-reply

Comment on attachment 8993082 [details]
Bug 1475427 - Refactor warnAboutClosingTabs function in tabbrowser.js so that it directly takes the number of tabs to close as param.

https://reviewboard.mozilla.org/r/257886/#review264824

> Pinned tabs should be removed too.

See comment bellow.

> closingTabsEnum.ALL also subtracted the pinned tabs.

Yes, I saw it but I dont understand why pinned tabs are subtracted here since they will be removed too. For CloseOtherTabs, I did understand since they are not closed. Do you know why?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 5 • 6 years ago
mozreview-review-reply

Comment on attachment 8993082 [details]
Bug 1475427 - Refactor warnAboutClosingTabs function in tabbrowser.js so that it directly takes the number of tabs to close as param.

https://reviewboard.mozilla.org/r/257886/#review264824

> Yes, I saw it but I dont understand why pinned tabs are subtracted here since they will be removed too. For CloseOtherTabs, I did understand since they are not closed. Do you know why?

Yeah, this is certainly a bug. When a window is closing with 5 tabs (2 of them pinned) we should be warning about 5 tabs being closed instead of 3 tabs. This is the only case where we do close pinned tabs.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 7 • 6 years ago
mozreview-review

Comment on attachment 8993082 [details]
Bug 1475427 - Refactor warnAboutClosingTabs function in tabbrowser.js so that it directly takes the number of tabs to close as param.

https://reviewboard.mozilla.org/r/257886/#review265232

	Pulsebot
	Comment 8 • 6 years ago

Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b31d456f8cb
Refactor warnAboutClosingTabs function in tabbrowser.js so that it directly takes the number of tabs to close as param. r=jaws

	Cosmin Sabou [:CosminS]
	Comment 9 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8b31d456f8cb