Summary

Update DiffCommentBlock to be an AbstractCommentBlock in DiffReviewable.

Review Request #4267 — Created June 26, 2013 and submitted July 1, 2013, 1:30 a.m.

Information

Owner

chipx86

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

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.

Issues

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	July 1, 2013, 1:30 a.m.
i is defined twice.	david	June 30, 2013, 10:56 p.m.
Is it really worth checking if the property value is already the same? Especially for html that seems like it …	david	June 30, 2013, 11:11 p.m.

reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js (Diff revision 1)

The issue has been resolved. 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?

chipx86 June 30, 2013, 5:30 p.m.

I was very tempted to rename it, but didn't know if it was worth breaking compatibility even more. But hey, why not. Renaming.

chipx86 June 30, 2013, 7:49 p.m.

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.

david June 30, 2013, 9:49 p.m.

OK. Let me do one more pass on this one.

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js (Diff revision 1)

The issue has been resolved. Show all issues

i is defined twice.

chipx86 June 30, 2013, 11:11 p.m.
```
Oops, merge issue.
```

reviewboard/static/rb/js/utils/propertyUtils.js (Diff revision 1)

The issue has been dropped. 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.

chipx86 June 30, 2013, 11:11 p.m.

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

david June 30, 2013, 11:18 p.m.
```
Awesome, thanks.
```

chipx86 June 30, 2013, 11:54 p.m.

Anything left after the 'i' change above?

david July 1, 2013, midnight
```
Nope! Ship it.
```

Status:: Completed