345898 - Add error handling to getClosedTabCount

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Simon Bünzli]

... and at least undoCloseTab is missing a check for the existence of aWindow.__SSi as well.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: fix

Adds more error handling to the areas originally reported, as well as others.

With SS disabled the behavior should be:

- open history menu - expected behavior: RCT menu disabled, no errors in console
- view tab context menu - expected behavior: no "undo close tab" menuitem, no errors in console
- cmd|ctl+shift+t to undo-close-tab: expected behavior: most recently closed tab is not re-opened, no errors in console

Comment 2

[Simon Bünzli]

(In reply to comment #1)
> - open history menu - expected behavior: RCT menu disabled, no errors in
> console

Why not completely hide it in this case, as we do with the item in the tab context menu?

Furthermore, you could just remove the method closedTabNameAt. It's not part of the API (we use getClosedTabData instead) and never used internally.

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: fix + comment #2 suggestions

(In reply to comment #2)
> (In reply to comment #1)
> > - open history menu - expected behavior: RCT menu disabled, no errors in
> > console
> 
> Why not completely hide it in this case, as we do with the item in the tab
> context menu?
> 
> Furthermore, you could just remove the method closedTabNameAt. It's not part of
> the API (we use getClosedTabData instead) and never used internally.
> 

Thanks Simon, both suggestions are implemented in the new patch.

Comment 4

[Simon Bünzli]

(In reply to comment #3)
> Created an attachment (id=239693) [edit]

This patch consists only of the browser.js changes. Anyway, I'd just check once for browser.sessionstore.enabled per window, since that pref only has an effect at startup, so changing it in between won't enable/disable SessionStore.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 239693 [details] [diff] [review]
fix + comment #2 suggestions

Gah, forgot a file in the diff.

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: fix v3 - addresses comments #2, #4

(In reply to comment #4)
> (In reply to comment #3)
> > Created an attachment (id=239693) [edit]
> 
> This patch consists only of the browser.js changes. Anyway, I'd just check once
> for browser.sessionstore.enabled per window, since that pref only has an effect
> at startup, so changing it in between won't enable/disable SessionStore.
> 

good suggestion, i added this to the patch.

Comment 7

[Asaf Romano (gone)]

Comment on attachment 240161 [details] [diff] [review]
fix v3 - addresses comments #2, #4

I really don't like this true|false|null modes thing, just initialize the global variable when the window opens (FWIW, I don't think this is worth an observer).

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: fix v4 - addresses mano review

(In reply to comment #7)
> (From update of attachment 240161 [details] [diff] [review] [edit])
> I really don't like this true|false|null modes thing, just initialize the
> global variable when the window opens 

ok, now just using a global var.

> (FWIW, I don't think this is worth an
> observer).
> 

what doesn't need an observer?

Comment 9

[Dietrich Ayala (:dietrich)]

Attachment: v5 - proper separator handling

Comment 10

[Asaf Romano (gone)]

Comment on attachment 240997 [details] [diff] [review]
v5 - proper separator handling

Could you simplify this a bit by using a shared broadcaster for all those items and set the hidden attribute on it?

http://www.xulplanet.com/tutorials/xultu/broadob.html

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: using broadcater

(In reply to comment #10)
> (From update of attachment 240997 [details] [diff] [review] [edit])
> Could you simplify this a bit by using a shared broadcaster for all those items
> and set the hidden attribute on it?
> 
> http://www.xulplanet.com/tutorials/xultu/broadob.html
> 

New patch, using a broadcaster. This works fine when hiding elements, but not when disabling. I tried using a broadcaster and a command (which should be functionally equivalent from what i can tell), and setting @disable=true didn't flow to the menus in either case. Is @disable a special case, or is this user-error? or a bug?

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: using broadcaster v2

Updated patch with the nonsense line fixed. However, I'm still not able to disable the menu via the broadcaster :(

Comment 15

[Simon Bünzli]

Attachment: updated patch

Throwing the error code really seems like the only way of reaching JavaScript callers which AFAICT so far have been the majority of our users.

Comment 16

[Simon Bünzli]

Attachment: patch and tests

Comment 17

[(no longer active)]

This patch is bitrottted now.  Need a new patch here.

Comment 18

[Simon Bünzli]

Attachment: for check-in [Checkin: Comment 19]

Unbitrotted and with a cleaned-up version of undoCloseTab (harmonized in style with the rest of the API methods).

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 342742 [details] [diff] [review]
for check-in
[Checkin: Comment 19]

http://hg.mozilla.org/mozilla-central/rev/5c8163ad17aa

Comment 20

[tabmix.onemen]

>  duplicateTab: function sss_duplicateTab(aWindow, aTab) {
>    if (!aTab.ownerDocument || !aTab.ownerDocument.defaultView.__SSi ||
>        !aWindow.getBrowser)
>      throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG);


there is aWindow.getBrowser in duplicateTab function
it should be aWindow.getBrowser() or aWindow.gBrowser

Comment 21

[(no longer active)]

(In reply to comment #20)
> >  duplicateTab: function sss_duplicateTab(aWindow, aTab) {
> >    if (!aTab.ownerDocument || !aTab.ownerDocument.defaultView.__SSi ||
> >        !aWindow.getBrowser)
> >      throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG);
> 
> 
> there is aWindow.getBrowser in duplicateTab function
> it should be aWindow.getBrowser() or aWindow.gBrowser

That code is simply checking to see if the function exists in the aWindow argument.