Rename table layout strategy "column width" APIs to use "isize" instead

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla40
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
Lots of our table code is about determining column widths. In particular, the nsITableLayoutStrategy interface does most of its interesting work in the function "ComputeColumnWidths".  (and the implementations of this method call nsTableColFrame::GetFinalWidth/SetFinalWidth)

In vertical writing-modes, though, column direction will be swapped (so that each column will be a horizontal thing, instead of a vertical thing).  Then, we'll need to be computing & setting column *heights*, not *widths*.

In general, we should be computing the inline-dimension size of table columns; so we should generify this language to just use "ISize" instead of "Width".
(Assignee)

Comment 1

4 years ago
[sorry, had assignee & CC fields' contents swapped when filing]
Assignee: jfkthame → dholbert
(Assignee)

Comment 2

4 years ago
Created attachment 8594213 [details] [diff] [review]
part 1: Rename nsITableLayoutStrategy::ComputeColumnWidths and associated comments to use "ISize" instead of "Width"
Attachment #8594213 - Flags: review?(jfkthame)
(Assignee)

Comment 3

4 years ago
Created attachment 8594215 [details] [diff] [review]
part 2: Rename nsTableColFrame Get/Set/ResetFinalWidth methods to use "ISize" instead of "Width"

Following the rabbit-hole -- the function changed in part 1 (ComputeColumnWidths) does its work using nsTableColFrame::GetFinalWidth/SetFinalWidth.

So, this patch does the same renaming on these nsTableColFrame methods (and the member-var "mFinalWidth" controlled by these methods, since its usages are pretty minimal & simple).
Attachment #8594215 - Flags: review?(jfkthame)
(Assignee)

Comment 4

4 years ago
Created attachment 8594217 [details] [diff] [review]
part 3: Rename nsTableFrame::GetColumnWidth to use "ISize" instead of "Width"

Following the rabbit-hole one more step: as shown in previous patch, nsTableColFrame::GetFinalWidth (now called GetFinalISize) is also exposed via a nsTableFrame method, "GetColumnWidth".

This patch renames that method as well, for consistency / completeness (or an approximation thereof).
Attachment #8594217 - Flags: review?(jfkthame)
The renaming here all looks reasonable, though it puts us in a state where various "ISize" methods are in fact still working on physical-coordinate widths, so the new names are rather misleading at this stage. But you're intending to fix that in following patches/bugs, I'm sure.....
Attachment #8594213 - Flags: review?(jfkthame) → review+
Attachment #8594215 - Flags: review?(jfkthame) → review+
Attachment #8594217 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 6

4 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #5)
> The renaming here all looks reasonable, though it puts us in a state where
> various "ISize" methods are in fact still working on physical-coordinate
> widths, so the new names are rather misleading at this stage.

True -- though, these classes are already in that state (& inconsistently so). These TableLayoutStrategy classes already have methods GetPrefISize() and MarkIntrinsicISizesDirty(), which actually operate on width-flavored values.  These methods were renamed last year in bug 1031241 (parts 1 and 3).

So, the patches here just make this code more consistent about displaying an "ISize" facade. :)

> But you're
> intending to fix that in following patches/bugs, I'm sure.....

At some point soon, yes. (not necessarily immediately, but sometime this quarter, in support of bug 	1077521.)
(Assignee)

Comment 7

4 years ago
(Thanks for the reviews!)
(Assignee)

Updated

4 years ago
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/352e8f8c5915
https://hg.mozilla.org/mozilla-central/rev/15f970a06d93
https://hg.mozilla.org/mozilla-central/rev/98fd24ddd601
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.