577309 - Stop using indices into content child lists in frame construction

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We'd like to move away from the index-based access to content child lists we have now.  This bug is about switching frame construction to the new GetFirstChild/GetLastChild/GetNextSibling/GetPreviousSibling APIs.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1. Fix assert in IsRootOfAnonymousSubtree

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2. Pass the end child to ContentRangeInserted

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3. Change ClearLazyBitsInChildren

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4. Change MaybeRecreateForFrameset

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5. Change MaybeConstructLazily

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 6. Change ChildIterator usage in nsListBoxBodyFrame to not use indices

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 7. Stop using indices for whitespace-suppression optimizations

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 8. Stop using indices in ChildIterator

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 9. Stop using indices in GetInsertionPrevSibling

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 10. Stop using indices in GetRangeInsertionPoint

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 11. Stop using indices in AddTextItemIfNeeded.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 12. Mostly stop using indices in ContentRangeInserted

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 13. Stop using indices in ContentInserted

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 14. Stop using indices in ContentAppended

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 15. Stop using indices in ContentRemoved

Comment 16

[Timothy Nikkel (:tnikkel)]

Comment on attachment 456296 [details] [diff] [review]
Part 2.  Pass the end child to ContentRangeInserted

+  // child at aIndexInContainer, and must be non-null.  aEndChild is the node at
+  // aEndIndexInContainer (which may be null).  If aAllowLazyConstruction is

Please indicate that aEndChild may only be null when the range is at the end of the child list.

Comment 17

[Timothy Nikkel (:tnikkel)]

Comment on attachment 456297 [details] [diff] [review]
Part 3.  Change ClearLazyBitsInChildren

Maybe ClearLazyBitsInSiblings or ClearLazyBits instead of ClearLazyBitsInChildren since we don't look at the children of the passed in nodes anymore?

Anyways, that's a nice improvement to that function.

Comment 18

[Timothy Nikkel (:tnikkel)]

Comment on attachment 456298 [details] [diff] [review]
Part 4. Change MaybeRecreateForFrameset

That's a nice improvement too.

Comment 19

[Timothy Nikkel (:tnikkel)]

Comment on attachment 456299 [details] [diff] [review]
Part 5.  Change MaybeConstructLazily

>-   *  -(for inserts) the child is not at the passed in index (this should only
>-   *   be happening because the editor is broken and passes in native anonymous
>-   *   content with index -1)
>+   *  -(for inserts) the child is anonymous

When we had indices it was pretty clear that passing content that was not in the container's principal child list to ContentAppended would just not work. Without indices it is not clear why this only applies to inserts. Maybe mention why this only applies to inserts and add an assertion in the append case in MaybeConstructLazily that the children are not anonymous.

Comment 20

[Timothy Nikkel (:tnikkel)]

Comment on attachment 456306 [details] [diff] [review]
Part 9.  Stop using indices in GetInsertionPrevSibling

This patch appears to be broken. The function signature for GetInsertionPrevSibling doesn't match in the .h and .cpp and aStartSkipIndexInContainer is still used in GetInsertionPrevSibling.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Please indicate that aEndChild may only be null when the range is at the end

Done.

> Maybe ClearLazyBitsInSiblings or ClearLazyBits 

ClearLazyBits it is!

> Maybe mention why this only applies to inserts and add an assertion in the
> append case

Done.  New comment:

   *  -(for inserts) the child is anonymous.  In the append case this function
   *   must not be called with anonymous children.

and I'm asserting for each child in the CONTENTAPPEND loop.

> The function signature for GetInsertionPrevSibling doesn't match in the .h
> and .cpp

Oops.  Indeed.  Needed to remove the aIndexInContainer arg in the .cpp (I had accidentally put that in a later patch in the queue).

> and aStartSkipIndexInContainer is still used in GetInsertionPrevSibling.

Where?  With the aIndexInContainer removal things compile fine, and I see no uses in the code.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 9 updated to comments

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 13 with saner comments

Comment 24

[Timothy Nikkel (:tnikkel)]

Comment on attachment 456397 [details] [diff] [review]
Part 9 updated to comments

This looks the same as the previous version.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 9 updated to comments for real

> This looks the same as the previous version.

I could have sworn I ran qref before attaching.... in any case, _this_ version is different!

Comment 26

[Timothy Nikkel (:tnikkel)]

(In reply to comment #21)
> > and aStartSkipIndexInContainer is still used in GetInsertionPrevSibling.
> 
> Where?  With the aIndexInContainer removal things compile fine, and I see no
> uses in the code.

My mistake, I didn't realize that this function was also changed in a patch you didn't ask for my review on.

Comment 27

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

Comment on attachment 456301 [details] [diff] [review]
Part 7.  Stop using indices for whitespace-suppression optimizations

Looks fine, however I think instead of boolean parameters it's much more readable (and extensible) to use a flags word and a meaningful flag #define or enum.

Comment 28

[Timothy Nikkel (:tnikkel)]

Comment on attachment 456302 [details] [diff] [review]
Part 8.  Stop using indices in ChildIterator

>@@ -6042,40 +6043,31 @@ nsCSSFrameConstructor::GetInsertionPrevS
>+  if (xblCase || !aChild->IsRootOfAnonymousSubtree()) {

Wouldn't we want the condition on this if to be just "!aChild->IsRootOfAnonymousSubtree()"?

Also, the only other place you use xblCase is in a comment, and you delete that comment in the next patch.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> Wouldn't we want the condition on this if to be just
> "!aChild->IsRootOfAnonymousSubtree()"?

I was trying to avoid changing behavior...  In particular, I'm not sure what the deal with |container != aContainer| is.  It might be worth a followup bug on this.

Comment 30

[Timothy Nikkel (:tnikkel)]

(In reply to comment #29)
> I was trying to avoid changing behavior...  In particular, I'm not sure what
> the deal with |container != aContainer| is.  It might be worth a followup bug
> on this.

That makes sense. I would be happy with a followup to do just what I suggested.

Comment 31

[Timothy Nikkel (:tnikkel)]

Comment on attachment 456307 [details] [diff] [review]
Part 10.  Stop using indices in GetRangeInsertionPoint

>@@ -6010,17 +6010,16 @@ GetAdjustedParentFrame(nsIFrame*       a
>-                                               PRInt32 aIndexInContainer,

Looks like you have a stray fragment in here from part 9. I assume you fixed this when you fixed part 9.

Comment 32

[Boris Zbarsky [:bzbarsky]]

> I assume you fixed this when you fixed part 9.

Yes, I did.

Comment 33

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

Comment on attachment 456302 [details] [diff] [review]
Part 8.  Stop using indices in ChildIterator

+  aFirst->mChild   = aLast->mChild   = 0;

nsnull

Comment 34

[Boris Zbarsky [:bzbarsky]]

Made that change and checked in:

  http://hg.mozilla.org/mozilla-central/rev/6870012f82dd
  http://hg.mozilla.org/mozilla-central/rev/d5fe749035ca
  http://hg.mozilla.org/mozilla-central/rev/67ed11b6c280
  http://hg.mozilla.org/mozilla-central/rev/56dc9c85d416
  http://hg.mozilla.org/mozilla-central/rev/4aa932b494ec
  http://hg.mozilla.org/mozilla-central/rev/bbc1189fce68
  http://hg.mozilla.org/mozilla-central/rev/c371e7a283a6
  http://hg.mozilla.org/mozilla-central/rev/aa1f92ff7a0f
  http://hg.mozilla.org/mozilla-central/rev/0e3e71b12b77
  http://hg.mozilla.org/mozilla-central/rev/16f50b6267cc
  http://hg.mozilla.org/mozilla-central/rev/2fb5e533c18f
  http://hg.mozilla.org/mozilla-central/rev/5d26e1919b83
  http://hg.mozilla.org/mozilla-central/rev/646ba1458dbd
  http://hg.mozilla.org/mozilla-central/rev/c879c3795adb
  http://hg.mozilla.org/mozilla-central/rev/f12a52c2b589

Filed bug 578966 on comment 30.