Closed Bug 1284947 Opened 8 years ago Closed 8 years ago

Update tab's permanentKey as part of _swapBrowserDocShells

Categories

(Firefox :: Tabbed Browser, enhancement, P3)

Product:
Firefox
Component:
Tabbed Browser
Platform:
x86
All
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: jryans, Assigned: u462496)

References

Details

Attachments

(1 file, 2 obsolete files)

Patch V3
1.25 KB, patch
dao
: review+
jryans
: feedback+
Details | Diff | Splinter Review 
In bug 1276514, `permanentKey` was made available on the tab as well as its browser.

tabbrowser.xml's `_swapBrowserDocShells` currently swaps the `permanentKey` fields between the two browsers.  Since this field is now also on the tab, it seems likely that we'd want to update the tab's `permanentKey` to match its browser during swapping.
Attached patch Patch (obsolete) — — Splinter Review 

        Attachment #8768519 -
        Flags: review?(dao+bmo)

    
  

        Attachment #8768519 -
        Attachment is patch: true

        Attachment #8768519 -
        Flags: review?(dao+bmo) → review+
Priority: -- → P3
Assignee: nobody → allassopraise

    
    

    
  
I wanted to land this but the patch failed to apply.
Flags: needinfo?(allassopraise)

    
    

    
  
Should I upload another patch?
Flags: needinfo?(allassopraise)

    
    

    
  
Yes, we need a patch that can be applied without conflicts.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch V2 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8768519 -
        Attachment is obsolete: true

        Attachment #8769165 -
        Flags: review?(dao+bmo)

        Attachment #8769165 -
        Flags: review?(dao+bmo) → review+

    
    

    
  
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/f6f33ab815a1
Update tab's permanentKey in _swapBrowserDocShells. r=dao

    
    

    
  
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10384506&repo=fx-team#L1905
Flags: needinfo?(allassopraise)

    
    

    
  
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/6cb3dc4bdbae
Backed out changeset f6f33ab815a1 for test bustages like in browser_bug477014.js

    
    

    
  
I think you need to use the gBrowser for other browser window when calling `getTabForBrowser(aOtherBrowser)`, instead of `this`.  The higher level method `swapBrowsersAndCloseOther` retrieves this as `var remoteBrowser = aOtherTab.ownerDocument.defaultView.gBrowser;`, so maybe it's simpler to update this field as part of `swapBrowsersAndCloseOther`.

    
    

    
  
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9)
> I think you need to use the gBrowser for other browser window when calling
> `getTabForBrowser(aOtherBrowser)`, instead of `this`.  The higher level
> method `swapBrowsersAndCloseOther` retrieves this as `var remoteBrowser =
> aOtherTab.ownerDocument.defaultView.gBrowser;`, so maybe it's simpler to
> update this field as part of `swapBrowsersAndCloseOther`.

Actually remoteBrowser is already defined on aOtherBrowser right in _swapBrowserDocShells.  I'm also going to test for `otherTab` because we don't always know there will be a tab associated with aOtherBrowser.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch V3
        — 
          — Splinter Review
      

    


  

        Attachment #8769165 -
        Attachment is obsolete: true
Flags: needinfo?(allassopraise)

        Attachment #8769295 -
        Flags: review?(jryans)

    
    

    
  
Comment on attachment 8769295 [details] [diff] [review]
Patch V3

Review of attachment 8769295 [details] [diff] [review]:
-----------------------------------------------------------------

It looks reasonable to me, but I am not a reviewer for /browser code, so let's check with :dao.

        Attachment #8769295 -
        Flags: review?(jryans)

        Attachment #8769295 -
        Flags: review?(dao+bmo)

        Attachment #8769295 -
        Flags: feedback+

        Attachment #8769295 -
        Flags: review?(dao+bmo) → review+

    
    

    
  
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/57d42a4e2ff1
Update tab's permanentKey in _swapBrowserDocShells. r=dao

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/57d42a4e2ff1
Status: NEW → RESOLVED
Closed: 8 years ago

          status-firefox50:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: