Make _beginRemoveTab consistently return a boolean

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dao, Assigned: geekodour08, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 55
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
The _beginRemoveTab method in browser/base/content/tabbrowser.xml normally returns true or false, but in one case it returns null. It should always return true or false.
(Assignee)

Comment 1

2 years ago
In the method,

>      <method name="_beginRemoveTab">
>        <parameter name="aTab"/>
>        <parameter name="aAdoptedByTab"/>
>        <parameter name="aCloseWindowWithLastTab"/>
>        <parameter name="aCloseWindowFastpath"/>
>        <parameter name="aSkipPermitUnload"/>
>        <body>
 and the calls,
>1 . remoteBrowser._beginRemoveTab(aOtherTab, aOurTab, true)
>2.  this._beginRemoveTab(aTab, null, null, true, skipPermitUnload)
 The third parameter is given null on the second call, that heppens to be aCloseWindowWithLastTab,
 but again,
> closeWindow = aCloseWindowWithLastTab != null ? aCloseWindowWithLastTab :
>                            !window.toolbar.visible ||
>                              Services.prefs.getBoolPref("browser.tabs.closeWindowWithLastTab");

 handles this, so is the case you mentioned one of these or it's some other, additionally there is no return statement before the last one return true at the bottom of the method, so this is most probably caused by some function call.
I'll try fixing this one, will update here. But can you tell me how will I be able to test it?
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 2

2 years ago
(In reply to Hrishikesh Barman[:geekodour08] from comment #1)
> In the method,
> 
> >      <method name="_beginRemoveTab">
> >        <parameter name="aTab"/>
> >        <parameter name="aAdoptedByTab"/>
> >        <parameter name="aCloseWindowWithLastTab"/>
> >        <parameter name="aCloseWindowFastpath"/>
> >        <parameter name="aSkipPermitUnload"/>
> >        <body>
>  and the calls,
> >1 . remoteBrowser._beginRemoveTab(aOtherTab, aOurTab, true)
> >2.  this._beginRemoveTab(aTab, null, null, true, skipPermitUnload)
>  The third parameter is given null on the second call, that heppens to be
> aCloseWindowWithLastTab,
>  but again,
> > closeWindow = aCloseWindowWithLastTab != null ? aCloseWindowWithLastTab :
> >                            !window.toolbar.visible ||
> >                              Services.prefs.getBoolPref("browser.tabs.closeWindowWithLastTab");
> 
>  handles this, so is the case you mentioned one of these or it's some other,

That's not what this bug is about, it's only about what _beginRemoveTab returns.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 3

2 years ago
Thanks for pointing that out, I was mistaken.

>if (closeWindow &&
>                  aCloseWindowFastpath &&
>                  this._removingTabs.length == 0) {
>                // This call actually closes the window, unless the user
>                // cancels the operation.  We are finished here in both cases.
>                this._windowIsClosing = window.closeWindow(true, window.warnAboutClosingWindow);
>                return null;
>              }
to 
>if (closeWindow &&
>                  aCloseWindowFastpath &&
>                  this._removingTabs.length == 0) {
>                // This call actually closes the window, unless the user
>                // cancels the operation.  We are finished here in both cases.
>                this._windowIsClosing = window.closeWindow(true, window.warnAboutClosingWindow);
>                return true;
>              }

It should return true because the window is not closed yet but the user has opted to, am I correct on that? If that's right I'll send the patch here. Let me know.
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 4

2 years ago
closeWindow is responsible for closing the whole window, so we don't need to separately close the tab anymore. This means we should return false here.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 5

2 years ago
I've changed it to return false now.
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 6

2 years ago
Comment on attachment 8850941 [details] [diff] [review]
fixes1349905.patch

looks good!
Flags: needinfo?(dao+bmo)
Attachment #8850941 - Flags: review+
(Reporter)

Updated

2 years ago
Assignee: nobody → hrishikeshbman

Comment 7

2 years ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/adb38e68486e
Make _beginRemoveTab consistently return a boolean. r=dao

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/adb38e68486e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.