1349887 - Remove obsolete comment about non-existent _closedDuringPermitUnload flag

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement)

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1349887 - Remove obsolete comment about non-existent _closedDuringPermitUnload flag.

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

Comment 2

[:Gijs (he/him)]

Where did this flag go away?

Comment 3

[Dão Gottwald [:dao]]

I have no idea. The first time I heard about this flag was in this obsolete comment.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8850465 [details]
Bug 1349887 - Remove obsolete comment about non-existent _closedDuringPermitUnload flag.

https://reviewboard.mozilla.org/r/123070/#review125366

(In reply to Dão Gottwald [::dao] from comment #3)
> I have no idea. The first time I heard about this flag was in this obsolete
> comment.

It seems I neglected to update the comment when landing bug 1050638, see comments on that bug. The comment should probably reference aTab.closing, and in that case still makes sense and should be kept.

Comment 5

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #4)
> Comment on attachment 8850465 [details]
> Bug 1349887 - Remove obsolete comment about non-existent
> _closedDuringPermitUnload flag.
> 
> https://reviewboard.mozilla.org/r/123070/#review125366
> 
> (In reply to Dão Gottwald [::dao] from comment #3)
> > I have no idea. The first time I heard about this flag was in this obsolete
> > comment.
> 
> It seems I neglected to update the comment when landing bug 1050638, see
> comments on that bug. The comment should probably reference aTab.closing,
> and in that case still makes sense and should be kept.

There's no reason to think aTab.closing should be set to false here. It makes no sense at all. So we should still remove this comment, as it's more confusing than clarifying. This comment is way sufficient:

              // If we were closed during onbeforeunload, we return false now
              // so we don't (try to) close the same tab again.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8850465 [details]
Bug 1349887 - Remove obsolete comment about non-existent _closedDuringPermitUnload flag.

https://reviewboard.mozilla.org/r/123070/#review125384

(In reply to Dão Gottwald [::dao] from comment #5)
> There's no reason to think aTab.closing should be set to false here. It
> makes no sense at all.

Before bug 1162860 this would happen automatically because the XBL binding would be destroyed, and then bad things would happen (which unfortunately I missed after incorporating the feedback for bug 1050638 - it wouldn't have been the case if we used an extra property instead of overloading tab.closing, as the patch originally did). Also, the `return false;` essentially means "we're not closing the tab", and so in that sense it's odd, at first sight, to keep aTab.closing set to true. Perhaps it would make more sense to return true if `aTab.closing` is true, and only return false in the other case. Not sure if that breaks anything in practice.

Anyway, we have tests for some of those edgecases now, so if you think we need no comments, it's up to you.

Comment 7

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #6)
> Comment on attachment 8850465 [details]
> Bug 1349887 - Remove obsolete comment about non-existent
> _closedDuringPermitUnload flag.
> 
> https://reviewboard.mozilla.org/r/123070/#review125384
> 
> (In reply to Dão Gottwald [::dao] from comment #5)
> > There's no reason to think aTab.closing should be set to false here. It
> > makes no sense at all.
> 
> Before bug 1162860 this would happen automatically because the XBL binding
> would be destroyed, and then bad things would happen

And then the aTab.closing check this is guarded with wouldn't work. Doesn't seem like an argument to me for keeping this comment.

> Also, the `return false;`
> essentially means "we're not closing the tab", and so in that sense it's
> odd, at first sight, to keep aTab.closing set to true.

Then we should more fundamentally clarify what return false means, as it doesn't mean "this tab isn't closing" but "we're not closing the tab in this call."

> Perhaps it would make more sense to return true if `aTab.closing` is true

Not at all.

Comment 8

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 59b6dcc66a37 -d 2d27eb9b83cb: rebasing 383530:59b6dcc66a37 "Bug 1349887 - Remove obsolete comment about non-existent _closedDuringPermitUnload flag. r=Gijs" (tip)
merging browser/base/content/tabbrowser.xml
warning: conflicts while merging browser/base/content/tabbrowser.xml! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 9

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #7)
> (In reply to :Gijs from comment #6)
> > Also, the `return false;`
> > essentially means "we're not closing the tab", and so in that sense it's
> > odd, at first sight, to keep aTab.closing set to true.
> 
> Then we should more fundamentally clarify what return false means, as it
> doesn't mean "this tab isn't closing" but "we're not closing the tab in this
> call."

So should we file a followup bug to add comments to removeTab and _beginRemoveTab explaining what the return values are supposed to mean? It looks like _beginRemoveTab returns true, false and null right now, with null and false being treated the same way (I think?) and no comments.

Comment 10

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #9)
> (In reply to Dão Gottwald [::dao] from comment #7)
> > (In reply to :Gijs from comment #6)
> > > Also, the `return false;`
> > > essentially means "we're not closing the tab", and so in that sense it's
> > > odd, at first sight, to keep aTab.closing set to true.
> > 
> > Then we should more fundamentally clarify what return false means, as it
> > doesn't mean "this tab isn't closing" but "we're not closing the tab in this
> > call."
> 
> So should we file a followup bug to add comments to removeTab and
> _beginRemoveTab explaining what the return values are supposed to mean?

If you think this is currently not clear enough, then yes, please file a bug.

> It
> looks like _beginRemoveTab returns true, false and null right now, with null
> and false being treated the same way (I think?) and no comments.

null is handled the same way and a leftover from when _beginRemoveTab returned an object rather than a boolean. I filed bug 1349905 on this earlier.

Comment 11

[Dão Gottwald [:dao]]

(In reply to Mozilla Autoland from comment #8)
> We're sorry, Autoland could not rebase your commits for you automatically.
> Please manually rebase your commits and try again.
> 
> hg error in cmd: hg rebase -s 59b6dcc66a37 -d 2d27eb9b83cb: rebasing
> 383530:59b6dcc66a37 "Bug 1349887 - Remove obsolete comment about
> non-existent _closedDuringPermitUnload flag. r=Gijs" (tip)
> merging browser/base/content/tabbrowser.xml
> warning: conflicts while merging browser/base/content/tabbrowser.xml! (edit,
> then use 'hg resolve --mark')
> unresolved conflicts (see hg resolve, then hg rebase --continue)

Looks like both autoland and inbound are out of sync with central in different ways:

https://hg.mozilla.org/mozilla-central/log/tip/browser/base/content/tabbrowser.xml
https://hg.mozilla.org/integration/mozilla-inbound/log/tip/browser/base/content/tabbrowser.xml
https://hg.mozilla.org/integration/autoland/log/tip/browser/base/content/tabbrowser.xml

Not sure if there are merges due to resolve this or if it's broken for good. For now I'll just wait and try again later.

Comment 12

[Carsten Book [:Tomcat]]

(In reply to Dão Gottwald [::dao] from comment #11)

> Looks like both autoland and inbound are out of sync with central in
> different ways:
> 
> https://hg.mozilla.org/mozilla-central/log/tip/browser/base/content/
> tabbrowser.xml
> https://hg.mozilla.org/integration/mozilla-inbound/log/tip/browser/base/
> content/tabbrowser.xml
> https://hg.mozilla.org/integration/autoland/log/tip/browser/base/content/
> tabbrowser.xml
> 
> Not sure if there are merges due to resolve this or if it's broken for good.
> For now I'll just wait and try again later.

should be ok now and re-merged m-c to m-i and autoland after the latest merge now

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d490ade72af7
Remove obsolete comment about non-existent _closedDuringPermitUnload flag. r=Gijs

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d490ade72af7