516742 - Make RFindLineContaining walk the kids of the lines backwards, not just the lines

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This should prevent issues when all the content is on one line (e.g. placeholders) and we're appending.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 2

[fantasai]

+    NS_ASSERTION(!aEnd->mFirstChild || aEnd->IsLastChild(curFrame),
+                 "Unexpected curFrame");

I thought we had an invariant that lines must never be empty... so asserting !aEnd->mFirstChild seems wrong to me.

+    PRInt32 n = aEnd->GetChildCount();
+    // i is the index of curFrame in aEnd
+    PRInt32 i = n - 1;

I don't think we need to define n here, just
  PRInt32 i = aEnd->GetChildCount() -1;
should be sufficient.

Otherwise it looks ok to me. It just seems a bit odd to be coding this up when you're planning to remove the need for an index anyway.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I thought we had an invariant that lines must never be empty..

If we do, that makes me very happy.  I thought we did too, until I found all sorts of places null-checking mFirstChild on lines...  Please let me know, since this might well affect other code I write in the near future!  ;)

> you're planning to remove the need for an index anyway.

This fixed an existing bug, and the other patch I'm still not completely sure about (e.g. not sure how to actually make it work).  It's not exactly a huge amount of duplicated effort here.... ;)

I nixed the |n| local.  Good catch.

Comment 4

[fantasai]

I'm pretty sure we do. The deletion code is very careful to remove line boxes whenever they become empty afaict. But you probably want roc to confirm. r=fantasai if you change the assertion to not imply that null mFirstChilds are OK, assuming roc confirms this is the case. :)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, line boxes should never be empty.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/0629dc7b3e5e with that assertion change