Closed Bug 1349905 Opened 7 years ago Closed 7 years ago

Make _beginRemoveTab consistently return a boolean

Categories

(Firefox :: Tabbed Browser, enhancement)

Product:
Firefox
Component:
Tabbed Browser
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: dao, Assigned: geekodour08, Mentored)

Details

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

Attachments

(1 file)

fixes1349905.patch
874 bytes, patch
dao
: review+
Details | Diff | Splinter Review 
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.
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)
(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)
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)
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)
I've changed it to return false now.
Flags: needinfo?(dao+bmo)
Comment on attachment 8850941 [details] [diff] [review]
fixes1349905.patch

looks good!
Flags: needinfo?(dao+bmo)
Attachment #8850941 - Flags: review+
Assignee: nobody → hrishikeshbman
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/adb38e68486e
Make _beginRemoveTab consistently return a boolean. r=dao
https://hg.mozilla.org/mozilla-central/rev/adb38e68486e
Status: NEW → RESOLVED
Closed: 7 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.

Attachment

General

Creator:
Created:
Updated:
Size: