Closed Bug 516742 Opened 11 years ago Closed 11 years ago
Line Containing walk the kids of the lines backwards, not just the lines
This should prevent issues when all the content is on one line (e.g. placeholders) and we're appending.
+ 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.
> 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.
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. :)
Yes, line boxes should never be empty.
Attachment #400825 - Flags: review?(roc) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/0629dc7b3e5e with that assertion change
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.