• 
      

    Update DiffCommentBlock to be an AbstractCommentBlock in DiffReviewable.

    Review Request #4267 — Created June 27, 2013 and submitted

    Information

    Review Board
    master

    Reviewers

    Update DiffCommentBlock to be an AbstractCommentBlock in DiffReviewable.
    
    DiffCommentBlock is now based on the AbstractCommentBlock{View}, which
    allows us to eliminate the majority of the code and take advantage of
    all the niceties that the Reviewable infrastructure gives us.
    
    Some of the AbstractCommentBlockView code had to be made more flexible
    with regard to positioning of tooltips and update bubbles, but otherwise
    it's basically unmodified.
    
    This also ends up fixing a long-standing bug where a comment in an
    expanded region would not show up again if the chunk is collapsed and
    then re-expanded.
    
    Most of the code from diffviewer.js is now gone with this change.
    Tested this pretty thoroughly with some diffs. Loaded in old comments
    (published and drafts). Created new ones. Tried different ranges.
    
    Tested making a comment in a collapsed area, then collapsed and re-expanded.
    Saw the comment.
    
    Repeated that test with a comment from a page load.
    
    Unit tests pass.
    Description From Last Updated

    Every time I see this I'm confused, and it takes me a minute to realize that this is for a …

    david david

    i is defined twice.

    david david

    Is it really worth checking if the property value is already the same? Especially for html that seems like it …

    david david
    david
    1. 
        
    2. Show all issues
      Every time I see this I'm confused, and it takes me a minute to realize that this is for a potential series of comments+replies for a single region (I always wonder why this isn't iterating over serializedComments).
      
      At a minimum, can we rename this to "addCommentBlock" (since it's only adding one), and document what "serializedComments" is?
      1. I was very tempted to rename it, but didn't know if it was worth breaking compatibility even more. But hey, why not. Renaming.
      2. Okay, I'm going to do this in a separate change, since it impacts too many lines. I'll have it up after this change goes in.
      3. OK. Let me do one more pass on this one.
    3. 
        
    david
    1. 
        
    2. Show all issues
      i is defined twice.
    3. reviewboard/static/rb/js/utils/propertyUtils.js (Diff revision 1)
       
       
       
       
       
       
      Show all issues
      Is it really worth checking if the property value is already the same? Especially for html that seems like it might be more expensive than just always calling the valued version.
      1. Calling html(...) is expensive. It destroys elements and creates new ones. Text is also expensive.
        
        I ran some tests on jsperfs. Turns out it's very expensive to set html and text. See http://jsperf.com/jquery-comparing-html-before-set-vs-just-setting
      2. Awesome, thanks.
      3. Anything left after the 'i' change above?
      4. Nope! Ship it.
    4. 
        
    chipx86
    Review request changed
    Status:
    Completed