Closed
Bug 858825
Opened 12 years ago
Closed 12 years ago
JavaScript error trying to use undo close tab menuitem in a private window
Categories
(SeaMonkey :: Tabbed Browser, defect)
SeaMonkey
Tabbed Browser
Tracking
(seamonkey2.18 fixed, seamonkey2.19 fixed, seamonkey2.20 fixed)
RESOLVED
FIXED
seamonkey2.20
People
(Reporter: neil, Assigned: neil)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
|
3.84 KB,
patch
|
Details | Diff | Splinter Review | |
|
6.53 KB,
patch
|
iannbugzilla
:
review+
philip.chee
:
feedback+
iannbugzilla
:
approval-comm-aurora+
iannbugzilla
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
Error: NS_ERROR_ILLEGAL_VALUE: 'Illegal value' when calling method: [nsISessionStore::getClosedTabData]
Source File: chrome://navigator/content/tabbrowser.xml
Line: 1513
|
Assignee | |
Comment 1•12 years ago
|
||
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #734118 -
Flags: review?(iann_bugzilla)
Attachment #734118 -
Flags: feedback?(philip.chee)
|
|
Assignee | |
Comment 2•12 years ago
|
||
Oh, I guess it would help to actually say something.
Session store doesn't activate for private windows, so it has no idea what is going on.
Therefore I have wrapped all the calls to session store in try/catch so if it fails we just use the old internal undo close tab mechanism.
I suppose the alternative would be for navigator.js to forward the privacy status to the tabbrowser so it could use if/else instead of try/catch.
Comment on attachment 734118 [details] [diff] [review]
Proposed patch
>+ <method name="undoCloseTabDisabled">
>+ <body>
>+ <![CDATA[
>+ try {
>+ this.mSessionStore.getClosedTabCount(window);
>+ if (this.mPrefs.getIntPref("browser.sessionstore.max_tabs_undo") > 0)
>+ return false;
>+ } catch (e) {
>+ // no session restore, just use saved browsers
Is this comment actually true in this case, surely it is a pref we are using here?
>+ }
>+ return this.mPrefs.getIntPref("browser.tabs.max_tabs_undo") <= 0;
>+ ]]>
>+ </body>
>+ </method>
(In reply to neil@parkwaycc.co.uk from comment #2)
> Oh, I guess it would help to actually say something.
>
> Session store doesn't activate for private windows, so it has no idea what
> is going on.
>
> Therefore I have wrapped all the calls to session store in try/catch so if
> it fails we just use the old internal undo close tab mechanism.
>
> I suppose the alternative would be for navigator.js to forward the privacy
> status to the tabbrowser so it could use if/else instead of try/catch.
Is using navigator.js any more efficient?
|
|
Assignee | |
Comment 5•12 years ago
|
||
So, after all that, I went for a slightly different alternative approach ;-)
Attachment #734140 -
Flags: review?(iann_bugzilla)
Attachment #734140 -
Flags: feedback?(philip.chee)
|
||
Comment 6•12 years ago
|
||
Comment on attachment 734140 [details] [diff] [review]
Alternative patch
I like this patch better. f=me
> + <field name="usePrivateBrowsing" readonly="true">
> + window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> + .getInterface(Components.interfaces.nsIWebNavigation)
> + .QueryInterface(Components.interfaces.nsILoadContext)
> + .usePrivateBrowsing
Shouldn't there be a semi-colon after .usePrivateBrowsing ?
Can we use the following instead?:
this.mCurrentBrowser.docShell.QueryInterface(Components.interfaces.nsILoadContext).usePrivateBrowsing;
Attachment #734140 -
Flags: feedback?(philip.chee) → feedback+
|
|
Assignee | |
Comment 7•12 years ago
|
||
(In reply to Philip Chee from comment #6)
> (From update of attachment 734140 [details] [diff] [review])
> > + <field name="usePrivateBrowsing" readonly="true">
> > + window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> > + .getInterface(Components.interfaces.nsIWebNavigation)
> > + .QueryInterface(Components.interfaces.nsILoadContext)
> > + .usePrivateBrowsing
> Shouldn't there be a semi-colon after .usePrivateBrowsing ?
We're using it as an expression, rather than a statement.
> Can we use the following instead?:
> this.mCurrentBrowser.docShell.QueryInterface(Components.interfaces.
> nsILoadContext).usePrivateBrowsing;
I don't want us to be confused by that private tab extension. (In theory we could check the window or the tab as appropriate for the call, but session store wants a window-level check anyway.)
|
|
||
Updated•12 years ago
|
Attachment #734118 -
Flags: feedback?(philip.chee)
Attachment #734140 -
Flags: review?(iann_bugzilla) → review+
|
|
Assignee | |
Comment 8•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.20
Attachment #734118 -
Flags: review?(iann_bugzilla)
|
|
Assignee | |
Comment 9•12 years ago
|
||
Comment on attachment 734140 [details] [diff] [review]
Alternative patch
[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Undo close menuitems don't work in private windows
Testing completed (on m-c, etc.): Baked on c-c
Risk to taking this patch (and alternatives if risky): Maybe, but not a lot
String changes made by this patch: None
Attachment #734140 -
Flags: approval-comm-beta?
Attachment #734140 -
Flags: approval-comm-aurora?
Attachment #734140 -
Flags: approval-comm-beta?
Attachment #734140 -
Flags: approval-comm-beta+
Attachment #734140 -
Flags: approval-comm-aurora?
Attachment #734140 -
Flags: approval-comm-aurora+
|
|
Assignee | |
Comment 10•12 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•