Closed Bug 1353888 Opened 3 years ago Closed 3 years ago

Add documentation for CSSOrderAwareFrameIterator

Categories

(Core :: Layout, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files)

In bug 812687, I'm spinning an iterator class out of nsGridContainerFrame.cpp into its own .h/.cpp file.

I'm filing this followup on adding more thorough documentation for that class. Some things to mention, which mats brought up:
=====================
Here's the things we could point out:
* it's used for iterating frame children on a given child list sorted on either CSS 'order' or 'box-ordinal-group' (with DOM order secondary)
* it can optionally skip placeholders
* it's good for performance to reuse the same iterator, just call Reset() to start a new iteration
* it's good for performance to give a eKnownOrdered or eKnownUnordered state to the ctor if you know it
* the behavior is undefined if the child frame list changes while iterating - don't do that!
=====================
Assignee: nobody → dholbert
Flags: needinfo?(dholbert)
Comment on attachment 8855919 [details]
Bug 1353888 part 1: Add documentation for CSSOrderAwareFrameIterator.

https://reviewboard.mozilla.org/r/127782/#review130496

Looks good to me, thanks.  Perhaps we should add a warning at the end that if the given frame list changes it makes the iterator invalid and bad things will happen if it's used.
Attachment #8855919 - Flags: review?(mats) → review+
Comment on attachment 8855920 [details]
Bug 1353888 part 2: Assert that CSSOrderAwareFrameIterator is only used on flex/grid children.

https://reviewboard.mozilla.org/r/127784/#review130500
Attachment #8855920 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #3)
> Perhaps we should add a warning at the end that
> if the given frame list changes it makes the iterator invalid and bad things
> will happen if it's used.

Good call. Added a warning, using pretty much your exact language:
https://reviewboard.mozilla.org/r/127782/diff/1-2/
Flags: needinfo?(dholbert)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1a649a805099
part 1: Add documentation for CSSOrderAwareFrameIterator. r=mats
https://hg.mozilla.org/integration/autoland/rev/34095953c987
part 2: Assert that CSSOrderAwareFrameIterator is only used on flex/grid children. r=mats
You need to log in before you can comment on or make changes to this bug.