Closed Bug 516742 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

This should prevent issues when all the content is on one line (e.g. placeholders) and we're appending.
Attached patch FixSplinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #400825 - Flags: review?(roc)
Attachment #400825 - Flags: review?(fantasai.bugs)
Blocks: 40988
+    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. :)
Blocks: 518665
Yes, line boxes should never be empty.
Pushed http://hg.mozilla.org/mozilla-central/rev/0629dc7b3e5e with that assertion change
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #400825 - Flags: review?(fantasai.bugs) → review+
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.