Move diff expand/collapse support into the new DiffReviewableView.

 






 

 


  Review Request #4257
  —
  Created June 24, 2013 and submitted 

 


















 

  

   Information

  





 
 

  chipx86
 









 
 

  Review Board
 









 
 

  master
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Move diff expand/collapse support into the new DiffReviewableView.

The resulting code is actually cleaner than the old code, because we
have more context to work from, instead of having to embed a bunch of
stuff inside the links.

There's now unit tests that ensure our expand/collapse logic works
correctly.

It also fixes a regression in collapsing that was introduced in the
original DiffReviewable model that was pulled out early on in the
JavaScript refactoring.

This is the last work needed before we can move the DiffComment
support over.

 








  


 
 

  
Tested expanding in every possible way, and collapsing each expanded
state.

Unit tests pass.

 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
You need to define 'var i' above.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
So I recall when I was optimizing rbpdf that jquery's scrollTop and height methods were much, much slower than the …

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Same here re: profiling with .offset() and .height()

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Can you move the && to the next line to avoid the visual confusion between the conditional and the block?

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
I don't see where this is ever assigned.

      		
      
       daviddavid
      
      
       
      
     


    
   

  

 







 






  

 








 














 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    You need to define 'var i' above.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    So I recall when I was optimizing rbpdf that jquery's scrollTop and height methods were much, much slower than the DOM versions. Can you profile scrolling around and see what sort of impact these have?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      So I performance-checked all this. Here's what I found:

Accessing the native attributes are, of course, faster. Much, much faster. Noticeably... if you're testing thousands of these a second, which, fortunately, we're not.

I switched the code around to using some of the native accessors. I didn't perceive a difference, and even without switching, 97% of the time spent on the page was idle time, according to the profiler.

After a lot of back-and-forth, and reading through jQuery code, I decided not to change any of this. The reason is that, while it's technically faster, I'd have to duplicate a bunch of cross-browser code that jQuery already does, and then it just becomes closer to jQuery, with more of a maintenance burden.

Breaking this down:

1) For scrollTop, window.pageYOffset works most of the time, but not in IE in quirks mode, at which point we now have to go through a bunch of checks.
2) $(window).height() is a bit easier. It's just more wordy to get to the native value (document.documentElement.clientHeight). Easier to replace, certainly. Might as well do that in one of these cases. It's 8x faster than calling $window.height().
3) $(el).height() checks a bunch of things that we don't want to duplicate, so that's not worth it at all.
4) $(el).offset() also does some cross-browser compatibility checks that I don't want to duplicate.

Actually, the biggest speed improvement we can make is to cache $(window) whenever possible. That's crazy slow.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      By the way, without changing anything else, caching $(window) on the view made the collapse button's position updating go from sorta choppy (it's clear we're updating it) to absolutely fixed in place, no stutter at all.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Also, sounds like $(window).height() may end up changing to be more accurate in some cases in the future, as they're trying to figure out issues surrounding some mobile browsers. So I'm choosing to go with that so we don't hit any future issues.
      
 
      

      4. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Same here re: profiling with .offset() and .height()
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Can you move the && to the next line to avoid the visual confusion between the conditional and the block?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    I don't see where this is ever assigned.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Huh. It's never defined in the old code either. Guess we don't need it!
      
 
      

      2. 


 
    


    

 
    

    6. 




  6. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 















 



 

  

   




chipx86




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...