Closed Bug 858825 Opened 11 years ago Closed 11 years ago

JavaScript error trying to use undo close tab menuitem in a private window

Categories

(SeaMonkey :: Tabbed Browser, defect)

Product:
SeaMonkey
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(seamonkey2.18 fixed, seamonkey2.19 fixed, seamonkey2.20 fixed)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.20
Tracking Flags:
Tracking Status
seamonkey2.18 --- fixed
seamonkey2.19 --- fixed
seamonkey2.20 --- fixed

People

(Reporter: neil, Assigned: neil)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Proposed patch
3.84 KB, patch
Details | Diff | Splinter Review
Alternative patch
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
Attached patch Proposed patch — — Splinter Review 
Assignee: nobody → neil
Status: NEW → ASSIGNED

        Attachment #734118 -
        Flags: review?(iann_bugzilla)

        Attachment #734118 -
        Flags: feedback?(philip.chee)

    
    

    
  
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?

    
    

    
  


  
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 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+

    
    

    
  
(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.)

    
  

        Attachment #734118 -
        Flags: feedback?(philip.chee)

    
  

        Attachment #734140 -
        Flags: review?(iann_bugzilla) → review+

    
    

    
  
Pushed comm-central changeset 53106593a26c.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago

          status-seamonkey2.20:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.20

    
  

        Attachment #734118 -
        Flags: review?(iann_bugzilla)

    
    

    
  
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+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: