Closed Bug 921174 Opened 6 years ago Closed 6 years ago

Clean up nsHTMLButtonControlFrame reflow code


(Core :: Layout: Form Controls, defect)

Not set





(Reporter: dholbert, Assigned: dholbert)




(5 files, 1 obsolete file)

While working on bug 913759, I noticed that the nsHTMLButtonControlFrame's reflow code is more confusing than it needs to be, due to some ambiguity in variable-naming and due to an odd division of labor between Reflow and ReflowButtonContents.

Filing this bug on cleaning that code up.
This patch renames the reflowState variables in ReflowContents(), for clarity.

Currently, ReflowContents() takes "aReflowState" (the reflow state of the button frame), and creates its own local "reflowState", and then works with both of them, making it confusing to follow what reflow state we're using for what.

This renames them to "aButtonReflowState" and "contentsReflowState" (plus a few newlines to avoid newly-introduced 80-char variations).
Attachment #810806 - Flags: review?(bzbarsky)
This patch makes us declare a local nsReflowStatus for use for when reflowing the button's contents-frame. (and we'll now sanity-check that it's _COMPLETE, for good measure)

(Currently, Reflow() passes its *own* aStatus to ReflowButtonContents(), which then uses that for its internal ReflowChild call, which is kind of bogus.  (and then we never bother to check it, so there's no reason we can't just declare a local status)

Elsewhere, we tend to declare a separate nsReflowStatus for child reflows, and I think we should stick to that here, for saner variable-lifetime & for a saner function-signature.
Attachment #810818 - Flags: review?(bzbarsky)
Right now, the "aDesiredSize" passed into nsHTMLButtonControlFrame is overloaded -- we use it for our internal reflow of the button contents frame, and then we repurpose it to store the button's own desired size.

This means its value is adjusted over time (and e.g. ::Reflow() sets its width but not its height or ascent), which makes it confusing to track what its values mean at any given point in time.

SO: This patch makes us use a *dedicated* nsHTMLReflowMetrics (desired size) for the button's contents frame.

Also: this patch pushes all responsibility for setting the button's *own* desired size into ReflowButtonContents.  (It helps to do this computation there, because if we're intrinsically sized, we've already got the intrinsic-size-clamped-to-the-min/max-height* available there, which is what we want to use for computing our button's desired height.)

* called "actualDesiredHeight" before my patch, & called "buttonContentBoxHeight" after my patch
Attachment #810849 - Flags: review?(bzbarsky)
Comment on attachment 810806 [details] [diff] [review]
part 1: Rename reflow state variables

Attachment #810806 - Flags: review?(bzbarsky) → review+
Comment on attachment 810818 [details] [diff] [review]
part 2: Don't use reflow status for two separate reflows

Attachment #810818 - Flags: review?(bzbarsky) → review+
Attachment #810937 - Flags: review?(bzbarsky)
[actually, the assertion-patch is gonna be last; I'm inserting another patch as #4, since it's more logically tied to patch #3 than the assertion-patch is]

This patch makes 'focusPadding' local to ReflowButtonContents, since that's the only place it's used now. (after patch 3)
Attachment #810937 - Attachment is obsolete: true
Attachment #810937 - Flags: review?(bzbarsky)
Attachment #810940 - Flags: review?(bzbarsky)
Attachment #810937 - Attachment description: part 4: Assert that we have exactly 1 child, and it's a button-content frame → [ignore; reordered to later in patch queue]
Here's the last patch for this bug -- this just adds some assertions to make sure we have exactly one child, and it's the button-content frame.

(We already implicitly assume this; any children beyond the first would be dropped on the floor and never reflowed, which would be bad. This just makes that assumption more explicit.)
Attachment #810941 - Flags: review?(bzbarsky)
Comment on attachment 810849 [details] [diff] [review]
part 3: Use dedicated nsHTMLReflowMetrics for button contents, and give ReflowButtonContents full responsibility for populating button's desired-size

Attachment #810849 - Flags: review?(bzbarsky) → review+
Comment on attachment 810940 [details] [diff] [review]
part 4: Move 'focusPadding' variable to be a local in ReflowButtonContents

Attachment #810940 - Flags: review?(bzbarsky) → review+
Comment on attachment 810941 [details] [diff] [review]
part 5: Assert that we have exactly one child, and it's the button-content frame

Attachment #810941 - Flags: review?(bzbarsky) → review+
Thanks for the reviews!

Try run:
You need to log in before you can comment on or make changes to this bug.