[Bug 584632]

[Ehsan-mozilla]

Comment on attachment 8582360
Patch to fix all three problems: Click, "end" key, left arrow from start of 
previous line

Review of attachment 8582360:
-----------------------------------------------------------------

Some feedback on your patch so far.

::: layout/generic/nsFrame.cpp
@@ +3555,3 @@
>    for (int32_t n = aLine->GetChildCount(); n;
>         --n, frame = frame->GetNextSibling()) {
> +    // Bug 756984: Skip brFrames. Can only skip if the line contains at 
> least one selectable and non-empty frame before

Nit: you don't need to mention bug numbers in comments.  This
information is available through hg/git blame.

@@ +3555,4 @@
>    for (int32_t n = aLine->GetChildCount(); n;
>         --n, frame = frame->GetNextSibling()) {
> +    // Bug 756984: Skip brFrames. Can only skip if the line contains at 
> least one selectable and non-empty frame before
> +    if (!SelfIsSelectable(frame, aFlags) || frame->IsEmpty() || (canSkipBr 
> && frame->GetType() == nsGkAtoms::brFrame))

Nit: please wrap lines at 80 characters.  For example, according to our
coding style, this condition should be written as:

  if (!SelfIsSelectable(frame, aFlags) || frame->IsEmpty() ||
      (canSkipBr && frame->GetType() == nsGkAtoms::brFrame))

@@ +6801,5 @@
>          for (int32_t count = lineFrameCount; count;
>               --count, frame = frame->GetNextSibling()) {
>            if (!frame->IsGeneratedContentFrame()) {
> +            // Bug 756984: When jumping to the end of the line with the 
> "end" key, skip over brFrames
> +            if (endOfLine && lineFrameCount > 1 && frame->GetType() == 
> nsGkAtoms::brFrame) continue;

Nit: again, please adjust the whitespace here.  Also, please wrap if
bodies in braces.

@@ +7090,5 @@
> +      nsIFrame *currentBlockFrame, *currentFirstFrame;
> +      nsRect usedRect;
> +      int32_t currentLine = nsFrame::GetLineNumber(traversedFrame, 
> aScrollViewStop, &currentBlockFrame);
> +      nsAutoLineIterator it = currentBlockFrame->GetLineIterator();
> +      it->GetLine(currentLine, &currentFirstFrame, &lineFrameCount, 
> usedRect);

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.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/584632

Title:
  composer changes font mid email

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs