Add Bookmarking feature to Diff Viewer.

Review Request #7658 — Created Sept. 25, 2015 and updated

david
Review Board
master
reviewboard, students

Add bookmarking feature in diff viewer so users can create and quickly jump
between bookmarks. Intended to help users moving back and forth between
different parts in the diff viewer when doing code reviews.

A "ghost" bookmark will follow the cursor on the right margin of the diff
viewer, which a user can click on to create new bookmarks on the hovered line
of the diff file. Clicking on the created bookmark will scroll the window
to where the bookmarked line is. Bookmarks are saved per diff using browser
Cookie, and will persist for one week.

Usability features include assigning different colors to bookmarks for easy
differentiation, positioning bookmarks on where the target line is relative
to the whole page, and animating the position at bookmark creation so users
can see where the newly created bookmark will reside.

Automated JS test cases:

Ghost bookmark
- should move along with the cursor on the diff
- becomes invisible when the cursor is not on diff
- expands when the cursor approaches
- shrinks when the cursor moves away
- creates a new bookmark when clicked on it
- no ghost bookmark when there already is a bookmark on the hovered line

Bookmarks
- appears in front of ghost bookmark
- expands when the cursor approaches
- shrinks when the cursor moves away
- will animate on creation
- on file header should only display the filename
- on a line in a file should display the filename and the line number
- on an expanded chunk will hide when the chunk is collapsed
- on a collapsed chunk will appear when the chunk is expanded
- should have different colors (for first 10 bookmarks)
- will be removed when clicked on [x]
- will scroll the page when clicked
- can be created from cookies
- will trigger cookie save event on creation or removal

Following features are tested manually on local devserver:

Bookmarks
- on an empty line should display nicely
- on a line that wraps on browser as multiple lines should display nicely
- added bookmark will appear again after refreshing on the page
- removed bookmarks will not appear again after refreshing on the page
- should be positioned where the main bookmark is relative to the whole page
- are repositioned when window is resized
- are repositioned when a chunk is expanded/collapsed
- are repositioned when a file is loaded


Description From Last Updated

Space after selector.

brenniebrennie

Alphabetize these.

brenniebrennie

We should set a constant in defs.less for this. e.g. @ghost-bookmark-background-color: rgba(173, 173, 173, 0.5);

brenniebrennie

Can we call this RB.MookmarkModel?

brenniebrennie

There should be a defaults object that contains safe default values (although fileName would probably be null in that case)

brenniebrennie

diffID

brenniebrennie

interdiffID

brenniebrennie

We really don't want to have models stored in models. Backbone doesn't support it that well. Instead, you can pass …

brenniebrennie

This should be in collections/bookmarkCollection.js

brenniebrennie

Likewise, RB.BookmarkCollection ?

brenniebrennie

Missing semicolon.

brenniebrennie

RB.BookmarkContainerView Also, this should be in its own separate file.

brenniebrennie

We use lowerCamelCase for JS names and hyphen-case for HTML names, so this should be bookmark-container.

brenniebrennie

Is this supposed to return something? If not, we do not need the return statement.

brenniebrennie

Please format this like: if (!noRender) { bookmarkerView.render(); }

brenniebrennie

We use one var statement as the first statement in any function.

brenniebrennie

You are likely going to want to use _.throttle here. Listening for mousemoves is expensive.

brenniebrennie

We put a space between the ) and the {. Please fix this throughout.

daviddavid

Should be bookmark (not Bookmark)

daviddavid

This line isn't necessary.

daviddavid

Variables should start with a lower-case letter.

daviddavid

You don't need the !! here since it's just a truthy-test. All conditionals should also have {}, regardless of whether …

daviddavid

It's probably better to format this with one call per line: this.$ghostBookmark .removeClass('open') .find('.bookmark-fold') .css('width', 0);

daviddavid

This stuff should probably happen in render rather than initialize.

daviddavid

Make sure you're indenting 4 spaces in JavaScript code.

daviddavid

Make sure you're indenting 4 spaces in JavaScript code.

daviddavid

The ?: operator will already convert to a bool, so you don't need !!

daviddavid

You can use _.bind here, too.

daviddavid

This should be the last line in the file. This is a vim modeline which gives the editor settings for …

brenniebrennie

You can nest this in the .close rule with &:hover.

brenniebrennie

You should use .transition(). It does a prefixed version of transition.

brenniebrennie

"the diffviewer" Needs a period.

brenniebrennie

I'm not sure if the cookie data should be passed in via options or not. You should maybe ping Christian …

brenniebrennie

Put these on seperate lines.

brenniebrennie

Remove this comment.

brenniebrennie

We use jquery.cookie. You can use it for reading/writing cookies. Check out the docs

brenniebrennie

Can we call this cookie ?

brenniebrennie

Blank line between these.

brenniebrennie

Remove this comment.

brenniebrennie

Replace this with a for loop.

brenniebrennie

Since we're not actually transmuting the models or options, I feel like we should do this in a signal handler. …

brenniebrennie

Needs a doc comment.

brenniebrennie

Put these on seperate lines, with defined variables first.

brenniebrennie

Can we call this bookmark ?

brenniebrennie

remove this comment.

brenniebrennie

This overwrites all of the cookie that is set, doesn't it? We really should use $.cookie here.

brenniebrennie

Needs a period.

brenniebrennie

Needs a doc comment.

brenniebrennie

Just call it template. That is the convention we use.

brenniebrennie

Space after , between items.

brenniebrennie

Remove this comment.

brenniebrennie

This should use sentence casing, e.g., begin with a capital and end with a period. "from a clone of the"

brenniebrennie

Missing a var statement. e isn't a descriptive variable name; we should use bookmark instead.

brenniebrennie

Since you're doing a filter & map on the values and only using them once, we can just do a …

brenniebrennie

Backbone.Model.get only takes one key at a time, so this should be: e.get('fileDiffID'), e.get('interFileDiffID')

brenniebrennie

If this is private it should be prefixed with an underscore. Also, this needs a doccomment.

brenniebrennie

Remove this line.

brenniebrennie

view is more readable than bview.

brenniebrennie

We format multi-line comments in JS as follows: /* * Line 1. * Line 2. * ... */

brenniebrennie

You can do view.remove().render() because remove should return the view.

brenniebrennie

Same comment as above regards to public/private-ness and doc comment.

brenniebrennie

Should be prefixed with underscore. Needs doc comment.

brenniebrennie

Should be prefixed with underscore. Needs doc comment.

brenniebrennie

Blank line between these.

brenniebrennie

Doc comment.

brenniebrennie

var $target = $(e.target), fileName, line, top;

brenniebrennie

Blank line between statement and block.

brenniebrennie

See above re: multi-line comments.

brenniebrennie

This is quite hard to read. I'm not sure what nodeType == 3 means? Is there a constnat we can …

brenniebrennie

Should be indented 4 spaces not 2.

brenniebrennie

What do you mean, without updating obj?

brenniebrennie

Sentence casing.

brenniebrennie

} else {

brenniebrennie

doc comment.

brenniebrennie

Doc comment.

brenniebrennie

We prefix jQuery objects, so this should be $diffViewElem. Also, you can shorten Elem to El.

brenniebrennie

Needs doc comment.

brenniebrennie

Should be indented 4.

brenniebrennie

var fileDiff = file.get('filediff'), interFileDiff = file.get('interfilediff');

brenniebrennie

We can write this more clearly as : return (filediff.id === fileDiffID && ((interFileDiff === null && interFileDiffID === null) …

brenniebrennie

===

brenniebrennie

Indent multiples of 4 spaces.

brenniebrennie

Missing a period.

brenniebrennie

Missing a period.

brenniebrennie

Col: 27 W601 .has_key() is deprecated, use 'in'

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Can we rename the cookie to "diff-bookmarks" ? Or maybe we should just have it be "bookmarks" (as we may …

chipx86chipx86

Just some tweaks to this. The argument name should include the type, and for request (a very common argument), we …

chipx86chipx86

This will be faster as: bookmark_cookie = request.COOKIES.get('diffbookmarks', '')

chipx86chipx86

We namespace these names, so these would be best as @bookmark-ghost-bg-color and @bookmark-default-bg-color, in alphabetical order.

chipx86chipx86

"Bookmarks"

chipx86chipx86

I know we don't have this in many places, but I'm trying to get in the habit: Can you add …

chipx86chipx86

We always use the long form: #FFFF00. Should also have a constant.

chipx86chipx86

All z-indexes must be registered in defs.less as constants.

chipx86chipx86

Not sure what e() is, but elsewhere, we do: .transition(padding-left 0.2s linear, top 0.2s ease-out;); (Note the inner ; near …

chipx86chipx86

We use the form of: 1px #333 solid This also needs a constant defined. Same with all other colors.

chipx86chipx86

All repeating sizes for padding, fonts, etc. should have a constant.

chipx86chipx86

Here and elsewhere, make sure there's a space before {.

chipx86chipx86

Selector groups are always in this order: & { ... } &#id { ... } &:pseudo-class { ... } &.class-name …

chipx86chipx86

No capitalization of "bookmarks" needed. Can you have this go into detail on what the class does? What it's tracking, …

chipx86chipx86

Multi-line comments must always be in this form: /* * line * line */ Same with other comments below.

chipx86chipx86

Function/method docs must be in this form: /* * One-line summary. * * Multi-line description. */ When there are arguments …

chipx86chipx86

Blank line between these.

chipx86chipx86

Blank line between these.

chipx86chipx86

Space before {.

chipx86chipx86

Let's pull out model.get('fileDiffID') just once.

chipx86chipx86

This would be better named as "updatedCookie". Make sure this is documented in the method docs.

chipx86chipx86

No blank line here.

chipx86chipx86

We only use one var statement per function, with one variable per line, like: var collection, listenerObj;

chipx86chipx86

I don't know that this is really needed. If you just need to check that the event emitted properly, you …

chipx86chipx86

Space before {. No need for return;, since that's implied.

chipx86chipx86

Ideally, you should use .toBe instead of .toEqual in tests. The reason is that .toBe is equivalent to strict JavaScript …

chipx86chipx86

Can you add docs for each of these?

chipx86chipx86

Just like methods, this should be in the form of: /* * One-line summary. * * Multi-line description. */ This …

chipx86chipx86

This is the default, so you don't need this here.

chipx86chipx86

When named template, it implies it's the template for the view itself. You probably want to call this ghostBookmarkTemplate.

chipx86chipx86

The strings must align. Indentation should be within the string, 1 space.

chipx86chipx86

Each method needs a docstring.

chipx86chipx86

Element-related initialization should go in render.

chipx86chipx86

Any member variables that don't need public access should be made private (prefix the name with a _).

chipx86chipx86

render must return this at the end.

chipx86chipx86

render shouldn't be responsible for deciding where its main element is put. That's the responsibility of the caller, only.

chipx86chipx86

These should instead go in events, like: events: { 'click .ghost': '_onGhostBookmarkClicked', ... }

chipx86chipx86

That sounds odd to me, but you should definitely be doing: return _super(this).remove.call(this); instead of removing it manually. The parent …

chipx86chipx86

Can you tell me what the body query is about? Is this about whether it's currently packed into the page …

chipx86chipx86

What's this for? Ideally, you wouldn't need to do this. I want to be sure I understand it well.

chipx86chipx86

The list of variables should begin with defined variables, so: var cookie = JSON.parse(cookieString), bookmark;

chipx86chipx86

i must be declared above, in the existing var list.

chipx86chipx86

All jQuery-wrapped elememnts should be named like $diffReviewable.

chipx86chipx86

This is a bit confusing. Can you instead just do this work when binding the element? If you're just looking …

chipx86chipx86

One var statement.

chipx86chipx86

.parents() is very expensive. Is there any way we can be smarter about what we're looing up? Can you explain …

chipx86chipx86

Blank line after this.

chipx86chipx86

This can be one statement, and you can probably use .width(). How about: var $bookmarkText = this._$ghostBookmark.find('.bookmark-text'); this._$ghostBookmark.find('.bookmark-fold') .width($bookmarkText.outerWidth()); Also, …

chipx86chipx86

Sentence capitalization and period.

chipx86chipx86

The .css should be indented one.

chipx86chipx86

These should go first, since they're defined.

chipx86chipx86

.parents() is very expensive, so we shoud minimize this however we can. Certainly, we should never call it more than …

chipx86chipx86

This is very awkwardly formatted. can we pull some of this out somehow?

chipx86chipx86

This has already been pulled out of diffModel.

chipx86chipx86

Only one blank line here.

chipx86chipx86

Space before {.

chipx86chipx86

filename +=.

chipx86chipx86

You can chain these.

chipx86chipx86

Blank line between these.

chipx86chipx86

Values should align.

chipx86chipx86

Blank line between these.

chipx86chipx86

Needs docs.

chipx86chipx86

Same comments as above with the formatting.

chipx86chipx86

This will cause problems. You need to instead do: this.$el.html(this.template()); Also, this must happen in render().

chipx86chipx86

Anything that isn't public API should have a _ in front of it.

chipx86chipx86

Blank line before comments.

chipx86chipx86

Would be best to pull these out into variables, since other things seem to use these.

chipx86chipx86

Blank lines before comments.

chipx86chipx86

If it's just one property, do: this.$el.css('top', topPosition);

chipx86chipx86

Blank line between these. vars and returns generally should be thought of as their own "paragraphs" of code.

chipx86chipx86

Parameters must align.

chipx86chipx86

Blank line between these.

chipx86chipx86

Need a space before {.

chipx86chipx86

Because of how JavaScript works, this can pick up data you don't expect. You need to use this form: for …

chipx86chipx86

Best to only pull this out once.

chipx86chipx86

Indentation is wrong. !== should be used instead of !=.

chipx86chipx86

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Should ideally be in alphabetical order.

chipx86chipx86

Here too.

chipx86chipx86

And these.

chipx86chipx86

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
  2. 
      
brennie
  1. 
      
  2. Space after selector.

  3. reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 1)
     
     
     
     
     
     
     
     
     
     

    Alphabetize these.

  4. We should set a constant in defs.less for this.

    e.g.

    @ghost-bookmark-background-color: rgba(173, 173, 173, 0.5);
    
  5. Can we call this RB.MookmarkModel?

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

    There should be a defaults object that contains safe default values (although fileName would probably be null in that case)

  7. We really don't want to have models stored in models. Backbone doesn't support it that well. Instead, you can pass diffModel in options, e.g.

    var bookmarkModel = new RB.BookmarkerModel({
        diffModel: diffModel
    });
    

    and then access it either through this.options or options (which is passed as the first argument to initialize).

  8. This should be in collections/bookmarkCollection.js

  9. Likewise, RB.BookmarkCollection ?

  10. RB.BookmarkContainerView

    Also, this should be in its own separate file.

  11. We use lowerCamelCase for JS names and hyphen-case for HTML names, so this should be bookmark-container.

  12. Is this supposed to return something? If not, we do not need the return statement.

  13. Please format this like:

    if (!noRender) {
        bookmarkerView.render();
    }
    
  14. We use one var statement as the first statement in any function.

  15. You are likely going to want to use _.throttle here. Listening for mousemoves is expensive.

  16. 
      
chipx86
  1. Can you upload screenshots showing how this works?

  2. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
YN
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
david
  1. So I don't have a lot of comments on the code yet, since it looks like you're still in early exploration.

    However, I do want to help guide you into our coding style now, before you write too much code. These comments are primarily about that.

  2. We put a space between the ) and the {. Please fix this throughout.

  3. Should be bookmark (not Bookmark)

  4. This line isn't necessary.

  5. Variables should start with a lower-case letter.

  6. You don't need the !! here since it's just a truthy-test.

    All conditionals should also have {}, regardless of whether the block therein is only one line.

  7. It's probably better to format this with one call per line:

    this.$ghostBookmark
        .removeClass('open')
        .find('.bookmark-fold')
        .css('width', 0);
    
  8. This stuff should probably happen in render rather than initialize.

  9. Make sure you're indenting 4 spaces in JavaScript code.

  10. Make sure you're indenting 4 spaces in JavaScript code.

  11. The ?: operator will already convert to a bool, so you don't need !!

  12. You can use _.bind here, too.

  13. 
      
YN
YN
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
YN
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
YN
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
        reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
        reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
brennie
  1. This is looking good and a lot of these issues are style nits. However, I have some general comments:

    1. You should use sentence casing for all comments. That means they should begin with a capital and end with a period.
    2. You should prefer === over == unless you are intentionally doing a type-coercing comparision. In that case, you should document why you are using coercion over expicit equality.
    3. Your functions need doc comments unless they're overriding functions like render, initialize, etc.
  2. This should be the last line in the file.

    This is a vim modeline which gives the editor settings for editing the file. Vim expects it to occur at the end of the file.

  3. You can nest this in the .close rule with &:hover.

  4. You should use .transition(). It does a prefixed version of transition.

  5. "the diffviewer"

    Needs a period.

  6. I'm not sure if the cookie data should be passed in via options or not. You should maybe ping Christian and ask him.

  7. Put these on seperate lines.

  8. We use jquery.cookie. You can use it for reading/writing cookies. Check out the docs

  9. Blank line between these.

  10. reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)
     
     
     
     
     
     
     
     
     

    Replace this with a for loop.

  11. reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     

    Since we're not actually transmuting the models or options, I feel like we should do this in a signal handler.

    In initialize we can do:

    this.listenTo(this, 'update', this.saveToCookie);
    

    That way, saveToCookie will be called whenever add/remove/set is called and we don't need to define these functions.

    (Just make sure this is in initialize after the cookie data is loaded).

  12. Put these on seperate lines, with defined variables first.

  13. This overwrites all of the cookie that is set, doesn't it? We really should use $.cookie here.

  14. Just call it template. That is the convention we use.

  15. Space after , between items.

  16. This should use sentence casing, e.g., begin with a capital and end with a period.

    "from a clone of the"

  17. Missing a var statement.

    e isn't a descriptive variable name; we should use bookmark instead.

  18. reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     

    Since you're doing a filter & map on the values and only using them once, we can just do a map and do the filtering inline as an if:

    this.collection.each(function(bookmark) {
        var fileDiffID = bookmark.get('fileDiffID'),
            interFileDiffID = bookmark.get('interFileDiffID');
    
        if (this.pageView.showsDiffFile(fileDiffID, interFileDiffID)) {
            this.createBookMarkView(bookmark, true);
        }
    }, this);
    

    This will be more efficient becuase there will be O(n) functions calls instead of ~2n.

  19. Backbone.Model.get only takes one key at a time, so this should be:

    e.get('fileDiffID'), e.get('interFileDiffID')
    
  20. If this is private it should be prefixed with an underscore.

    Also, this needs a doccomment.

  21. view is more readable than bview.

  22. We format multi-line comments in JS as follows:

    /*
     * Line 1.
     * Line 2.
     * ...
     */
    
  23. You can do view.remove().render() because remove should return the view.

  24. Same comment as above regards to public/private-ness and doc comment.

  25. Should be prefixed with underscore. Needs doc comment.

  26. Should be prefixed with underscore. Needs doc comment.

  27. Blank line between these.

  28. var $target = $(e.target),
        fileName,
        line,
        top;
    
  29. Blank line between statement and block.

  30. See above re: multi-line comments.

  31. This is quite hard to read.

    I'm not sure what nodeType == 3 means? Is there a constnat we can use.

    Really, I'm not sure what this is doing in general. Ping me in slack and we can discuss this.

  32. Should be indented 4 spaces not 2.

  33. What do you mean, without updating obj?

  34. We prefix jQuery objects, so this should be $diffViewElem.

    Also, you can shorten Elem to El.

  35. reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 6)
     
     
     
     
     
     
     
     
     

    Should be indented 4.

  36. var fileDiff = file.get('filediff'),
        interFileDiff = file.get('interfilediff');
    
  37. We can write this more clearly as :

    return (filediff.id === fileDiffID &&
            ((interFileDiff === null && interFileDiffID === null) ||
             interFileDiff.id === interFileDiffID));
    

    This should use === not ==.

  38. reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 6)
     
     
     
     
     
     
     

    Indent multiples of 4 spaces.

  39. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/diffviewer/views.py (Diff revision 8)
     
     
    Col: 27
     W601 .has_key() is deprecated, use 'in'
    
  3. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/staticbundles.py (Diff revision 9)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/staticbundles.py (Diff revision 10)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/staticbundles.py (Diff revision 11)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
chipx86
  1. First off, this feature is awesome and boy do I wish I had it while reviewing this change :)

    Second, I have a lot of comments here. The good news is, most of these are stylistic things that I repeat over and over. You'll be able to plow through most of these.

    However, I did not highlight every occurrence, so I'll want you to go through and find, for instance, cases where you have function(){ instead of function() {, and comments that need a blank line before them, etc.

    There also needs to be more documentation. We're working on fleshing these out, and we want them to follow the Python standards. I'm happy to answer questions on this.

    I also have some implementation comments, and comments about performance. For instance, I point out several cases where .parents() is used, which is an expensive function that should largely be avoided if at all possible, so I'd like to work with you to better understand these and see if we can come up with a better method.

  2. reviewboard/diffviewer/views.py (Diff revision 11)
     
     

    Can we rename the cookie to "diff-bookmarks" ? Or maybe we should just have it be "bookmarks" (as we may later want to port this to text-based file attachments, perhaps).

    It's also cheaper to fetch the data out, and set the header if that data is not None. Further cheaper if you assume it exists in a try/except, like:

    try:
        response['X-ReviewBoard-Bookmarks'] = request.COOKIES['bookmarks']
    except KeyError:
        # There aren't any bookmark cookies set. Ignore.
    
  3. reviewboard/diffviewer/views.py (Diff revision 11)
     
     
     
     

    Just some tweaks to this. The argument name should include the type, and for request (a very common argument), we don't need to go into details of how it'll be used. It just runs the risk of going stale as we use more of it down the road.

    request (django.http.HttpRequest):
        The HTTP request from the client.
    
  4. reviewboard/diffviewer/views.py (Diff revision 11)
     
     
     
     
     

    This will be faster as:

    bookmark_cookie = request.COOKIES.get('diffbookmarks', '')
    
  5. reviewboard/static/rb/css/defs.less (Diff revision 11)
     
     
     

    We namespace these names, so these would be best as @bookmark-ghost-bg-color and @bookmark-default-bg-color, in alphabetical order.

  6. I know we don't have this in many places, but I'm trying to get in the habit: Can you add a comment for each of these selectors documenting what part of the UI this is?

  7. We always use the long form: #FFFF00. Should also have a constant.

  8. All z-indexes must be registered in defs.less as constants.

  9. Not sure what e() is, but elsewhere, we do:

    .transition(padding-left 0.2s linear, top 0.2s ease-out;);
    

    (Note the inner ; near the end. This forces LessCSS to consider our commas as literal text, and not argument separators.)

  10. We use the form of:

    1px #333 solid

    This also needs a constant defined. Same with all other colors.

  11. All repeating sizes for padding, fonts, etc. should have a constant.

  12. Here and elsewhere, make sure there's a space before {.

  13. Selector groups are always in this order:

    & { ... }
    &#id { ... }
    &:pseudo-class { ... }
    &.class-name { ...}
    element { ... }
    #id { ... }
    .class-name { ... }
    

    Those should be in alphabetical order within a grouping.

    This applies to all the CSS in this change.

  14. No capitalization of "bookmarks" needed.

    Can you have this go into detail on what the class does? What it's tracking, how it's used, etc.

  15. Multi-line comments must always be in this form:

    /*
     * line
     * line
     */
    

    Same with other comments below.

  16. Function/method docs must be in this form:

    /*
     * One-line summary.
     *
     * Multi-line description.
     */
    

    When there are arguments or return values, add:

    /*
     * ...
     *
     * Args:
     *     bookmark (the type):
     *         The description.
     *
     *     next_argument (type):
     *         ...
     *
     * Returns:
     *     the return type:
     *     The description.
     */
    

    Same with all other methods.

  17. Let's pull out model.get('fileDiffID') just once.

  18. This would be better named as "updatedCookie".

    Make sure this is documented in the method docs.

  19. We only use one var statement per function, with one variable per line, like:

    var collection,
        listenerObj;
    
  20. I don't know that this is really needed. If you just need to check that the event emitted properly, you should spy on ollection.trigger and check that it was called with the expected parameters for emitting that event.

  21. Space before {.

    No need for return;, since that's implied.

  22. Ideally, you should use .toBe instead of .toEqual in tests. The reason is that .toBe is equivalent to strict JavaScript comparison (===), which means that 1 === 1 but 1 !== "1".

    If you use .toEqual, it's like ==, so 1 == "1".

    Unit tests should be strict, so we want to catch future issues like the possibility of an integer turning into a string.

  23. Can you add docs for each of these?

  24. Just like methods, this should be in the form of:

    /*
     * One-line summary.
     *
     * Multi-line description.
     */
    

    This should also go into more detail of what this class is in charge of doing.

  25. This is the default, so you don't need this here.

  26. When named template, it implies it's the template for the view itself. You probably want to call this ghostBookmarkTemplate.

  27. The strings must align. Indentation should be within the string, 1 space.

  28. Each method needs a docstring.

  29. Element-related initialization should go in render.

  30. Any member variables that don't need public access should be made private (prefix the name with a _).

  31. render must return this at the end.

  32. render shouldn't be responsible for deciding where its main element is put. That's the responsibility of the caller, only.

  33. These should instead go in events, like:

    events: {
        'click .ghost': '_onGhostBookmarkClicked',
        ...
    }
    
    1. $ghostBookmark is actually not a child of the container $el, so event listeners need to be registered manually.

  34. That sounds odd to me, but you should definitely be doing:

    return _super(this).remove.call(this);
    

    instead of removing it manually. The parent remove() does more than this.

  35. Can you tell me what the body query is about? Is this about whether it's currently packed into the page somewhere?

  36. What's this for? Ideally, you wouldn't need to do this. I want to be sure I understand it well.

  37. The list of variables should begin with defined variables, so:

    var cookie = JSON.parse(cookieString),
        bookmark;
    
  38. i must be declared above, in the existing var list.

  39. All jQuery-wrapped elememnts should be named like $diffReviewable.

  40. This is a bit confusing. Can you instead just do this work when binding the element? If you're just looking to bind additional arguments, you can pass them to _.bind().

  41. .parents() is very expensive. Is there any way we can be smarter about what we're looing up? Can you explain these checks in more detail to me?

    I'm wondering if hasClass() would be enough.

  42. This can be one statement, and you can probably use .width(). How about:

    var $bookmarkText = this._$ghostBookmark.find('.bookmark-text');
    
    this._$ghostBookmark.find('.bookmark-fold')
        .width($bookmarkText.outerWidth());
    

    Also, how about pulling out things like .bookmark-text in render() above, so that we don't have to keep re-fetching?

  43. Sentence capitalization and period.

  44. The .css should be indented one.

  45. These should go first, since they're defined.

  46. .parents() is very expensive, so we shoud minimize this however we can. Certainly, we should never call it more than once for the same element with the same selector.

  47. reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
     
     
     
     
     
     
     
     
     

    This is very awkwardly formatted. can we pull some of this out somehow?

  48. This has already been pulled out of diffModel.

  49. Only one blank line here.

  50. Blank line between these.

  51. Blank line between these.

  52. reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11)
     
     
     
     
     
     
     
     

    Same comments as above with the formatting.

  53. This will cause problems. You need to instead do:

    this.$el.html(this.template());
    

    Also, this must happen in render().

  54. Anything that isn't public API should have a _ in front of it.

  55. Blank line before comments.

  56. Would be best to pull these out into variables, since other things seem to use these.

  57. Blank lines before comments.

  58. If it's just one property, do:

    this.$el.css('top', topPosition);
    
  59. Blank line between these.

    vars and returns generally should be thought of as their own "paragraphs" of code.

  60. Parameters must align.

  61. Blank line between these.

  62. Need a space before {.

  63. Because of how JavaScript works, this can pick up data you don't expect. You need to use this form:

    for (var fileID in data) {
        if (data.hasOwnProperty(fileID)) {
            ...
        }
    }
    

    That assumes that data is an object and not an array.

    Also, space before {.

  64. Best to only pull this out once.

  65. Indentation is wrong.

    !== should be used instead of !=.

  66. reviewboard/staticbundles.py (Diff revision 11)
     
     
     

    Should ideally be in alphabetical order.

  67. reviewboard/staticbundles.py (Diff revision 11)
     
     

    Here too.

  68. reviewboard/staticbundles.py (Diff revision 11)
     
     
     

    And these.

  69. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/staticbundles.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/staticbundles.py (Diff revision 13)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/staticbundles.py (Diff revision 14)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
YN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
        reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
        reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
        reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/staticbundles.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
david
david
Review request changed

Status: Re-opened

Owner:

-ynkym
+david
Loading...