921174 - Clean up nsHTMLButtonControlFrame reflow code

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: Rename reflow state variables

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).

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 2: Don't use reflow status for two separate reflows

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.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 3: Use dedicated nsHTMLReflowMetrics for button contents, and give ReflowButtonContents full responsibility for populating button's desired-size

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 810806 [details] [diff] [review]
part 1: Rename reflow state variables

r=me

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 810818 [details] [diff] [review]
part 2: Don't use reflow status for two separate reflows

r=me

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: [ignore; reordered to later in patch queue]

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: part 4: Move 'focusPadding' variable to be a local in ReflowButtonContents

[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)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: part 5: Assert that we have exactly one child, and it's the button-content frame

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.)

Comment 9

[Boris Zbarsky [: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

r=me

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 810940 [details] [diff] [review]
part 4: Move 'focusPadding' variable to be a local in ReflowButtonContents

r=me

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 810941 [details] [diff] [review]
part 5: Assert that we have exactly one child, and it's the button-content frame

r=me

Comment 12

[Daniel Holbert [:dholbert]]

Thanks for the reviews!

Try run: https://tbpl.mozilla.org/?tree=Try&rev=4bb23b89fc41

Comment 13

[Daniel Holbert [:dholbert]]

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8b7e4858ddd
https://hg.mozilla.org/integration/mozilla-inbound/rev/7164978f006d
https://hg.mozilla.org/integration/mozilla-inbound/rev/57288f5ade9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/066962a4f2ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/06ee4167c4bb

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f8b7e4858ddd
https://hg.mozilla.org/mozilla-central/rev/7164978f006d
https://hg.mozilla.org/mozilla-central/rev/57288f5ade9d
https://hg.mozilla.org/mozilla-central/rev/066962a4f2ee
https://hg.mozilla.org/mozilla-central/rev/06ee4167c4bb