Comment 177 for bug 584632

(In reply to Jorg K from comment #90)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #88)
> > Nit: you don't need to mention bug numbers in comments. This information is
> > available through hg/git blame.
> There are many bug numbers in the code (including some that you entered for
> bug 596506), so this policy must have changed recently.

Well you're repeating it multiple times in the same file. The bug number is really not adding any useful information in this case.

> > Also, please wrap if bodies in braces.
> Again, the policy seems to have changed here since there are many conditions
> of this style:
> if (!range.content)
> return NS_ERROR_FAILURE;

We unfortunately don't consistently adhere to the coding style, but we should for new code that we're adding.

> > There is already code which gets the line frame count earlier in this
> > function. Please refactor it out of the else block it currently lives in
> > and just use lineFrameCount from that code here. That already takes care of
> > checking the return value of GetLine for errors too. Also, I think you want
> > to check atLineEdge instead of aJumpLines here.
> I disagree. I did what you suggest, but it didn't work. The
> it->GetLine(thisLine, &firstFrame, &lineFrameCount, nonUsedRect);
> ealier on gets the details for the previous line where the left arrow was
> pressed.
> We then move on to the line before. The next
> it->GetLine(currentLine, &currentFirstFrame, &lineFrameCount, usedRect);
> is operating on the line we've just moved to, the one where we want to skip
> the brFrame.
> The only thing which would be a bit cleaner is not to re-use the "it"
> variable but to declare another one.

Oh yeah, good point. So please make sure you check the return value of GetLine like above.