368902 - Unnecessary looping over collapsed children in nsSprocketLayout

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Andreas Lange]

Noticed while reading code for the deCOMtamination in bug 243370. This code is unchanged since revision 1.1 of the file (from back in 2000), so it certainly works, but it looks wrong!

In pseudocode, we have the following functions:

nsSprocketLayout::GetPrefSize(aBox, aState) {
  child = aBox->GetChild();
  while (child) {
     // ignore collapsed children
     if (!child->isCollapsed(aState)) {
        /* ... add up child->GetPrefSize() */
     }
     child = child->GetNextBox();
  }
}

But, GetMinSize and GetMaxSize do this:

nsSprocketLayout::GetMinSize(aBox, aState) {
  child = aBox->GetChild();
  while (child) {
     // ignore collapsed children
     if (!aBox->isCollapsed(aState)) {
        /* ... add up child->GetMinSize() */
     }
     child = child->GetNextBox();
  }
}

Note that all functions iterate all children, but GetPrefSize uses |child->IsCollapsed()| as condition for doing anything with |child|, while GetMinSize and GetMaxSize check |aBox->IsCollapsed()| which certainly shouldn't change while within the loop!

I don't know how to test this, but it looks like a thinko was made here and then got copied a few times. Before the deCOM this looked like
  
  PRBool isCollapsed;
  aBox->IsCollapsed(aState, &isCollapsed);
  while (!isCollapsed)

which made it harder to see.

Comment 1

[Andreas Lange]

Attachment: Change |aBox| to |child| where appropriate

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 253534 [details] [diff] [review]
Change |aBox| to |child| where appropriate

r+sr=bzbarsky

The reason this happened to work is that the Get*Size impls also check IsCollapsed() and return 0x0 for min/pref and "infinite" size for max if the box is collapsed.

Comment 3

[Phil Ringnalda (:philor)]

layout/xul/base/src/nsSprocketLayout.cpp 1.54

Comment 4

[Jeff Walden [:Waldo]]

A testcase that would fail if comment 2 hadn't been in effect here would be nice.