813358 - Move nsBoxFrame frame-sorting logic into nsLayoutUtils

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

nsBoxFrame has code to merge-sort a frame list, and I'd love to use it in bug 811521 for sorting a flex container's children.

Filing this bug on moving that frame-list-sorting code into nsLayoutUtils and making it more generic.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: remove unneeded state args from sorting methods

This patch just removes the (unneeded) nsBoxLayoutState argument from the sorting methods in question (in preparation for ripping them out of nsBoxFrame).

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 2: Simplify "is frame list already sorted" logic

This patch simplifies the logic for checking if a frame list is already sorted.  In particular, this patch just makes us directly compare each pair of adjacent frames, instead of using "maxOrdinal".

(This functionality will get separated out into its own templatized function in the next patch, but that patch is simpler if I've paved the way for that in this patch.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 2 v2: Simplify "is frame list already sorted" logic

(forgot a commit message; added one now)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 3: Split CheckBoxOrder into "IsFrameListSorted" and "SortFrameList", which take comparison function as a template parameter

This patch splits CheckBoxOrder into two helper-methods -- IsFrameListSorted() and SortFrameList() -- which take a comparison function as a template-argument.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 4: Move framelist-sorting code into nsLayoutUtils

Comment 6

[Daniel Holbert [:dholbert]]

(part 4 is essentially just a giant cut-and-paste, with a few "nsLayoutUtils::" prefix insertions & removals)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 683365 [details] [diff] [review]
part 1: remove unneeded state args from sorting methods

r=dbaron

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 683391 [details] [diff] [review]
part 2 v2: Simplify "is frame list already sorted" logic

It might be better to have the mFrames.IsEmpty() check before the SupportsOrdinalsInChildren check, just since it might be better to have the early return that doesn't involve a virtual function call before the one that does.

I think I slightly preferred the old one-iterator-plus-maxOrdinal approach rather than the two-iterators approach, but I'm ok either way.

r=dbaron

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 683397 [details] [diff] [review]
part 3: Split CheckBoxOrder into "IsFrameListSorted" and "SortFrameList", which take comparison function as a template parameter

Er, now I see where you were going with the previous patches; I guess you can ignore a bunch of my comments on them.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #9)
> Er, now I see where you were going with the previous patches; I guess you
> can ignore a bunch of my comments on them.

Yup -- sorry for not making that clearer.  (I'll disregard both points from comment 8, since that code is the way it is for the purpose of splitting out a generic "IsFrameListSorted" function.)

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: part 5: add post-condition to check that sorting makes us sorted

Thanks for the reviews!

One more tiny patch here that I forgot to attach -- just adding a sanity-check assertion that after we sort, we satisfy IsFrameListSorted().

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af6a637e9a95
https://hg.mozilla.org/mozilla-central/rev/c01b1fbdacdf
https://hg.mozilla.org/mozilla-central/rev/5e89b39fd7d0
https://hg.mozilla.org/mozilla-central/rev/58cb9bf14a4c
https://hg.mozilla.org/mozilla-central/rev/ceddb3b529de