Closed Bug 1173305 Opened 4 years ago Closed 4 years ago

convert BasicTableLayoutStrategy to work with logical coordinates


(Core :: Layout: Tables, defect)

Not set



Tracking Status
firefox41 --- fixed


(Reporter: jfkthame, Assigned: jfkthame)


(Depends on 1 open bug)



(1 file, 1 obsolete file)

Just attaching some current WIP here; not yet ready for review.
A lot of this is simple renaming of the s/width/isize/ kind, but within the Get*ISizeInfo methods we also make it respect writing mode when accessing the style data. There's no change to current behavior, because we (temporarily) force all tables to be horizontal, but once table cells/rows/frames are also converted, this will make basic table layout work properly in vertical modes.
Attachment #8622048 - Flags: review?(dholbert)
Attachment #8617839 - Attachment is obsolete: true
Assignee: nobody → jfkthame
Comment on attachment 8622048 [details] [diff] [review]
Convert BasicTableLayoutStrategy to work with logical coordinates

Thanks for doing this!

Looks like there are some comments in the header file, alongside the code that you're changing, which need s/width/ISize/ to keep up with the code. If you want to take care of these in a followup comment-only patch, feel free (though I think the explicit mentions of variable names should probably be updated in this patch here.)


>+++ b/layout/tables/BasicTableLayoutStrategy.h
>@@ -36,17 +36,17 @@ private:
>     // Compute intrinsic width member variables on the columns.
>     void ComputeColumnIntrinsicISizes(nsRenderingContext* aRenderingContext);
>     // Distribute a colspanning cell's percent width (if any) to its columns.
>-    void DistributePctWidthToColumns(float aSpanPrefPct,
>+    void DistributePctISizeToColumns(float aSpanPrefPct,
>                                      int32_t aFirstCol,
>                                      int32_t aColCount);

s/percent width/percent ISize/ in the comment here, I think?

(Same goes for the method above it, I guess.)

>     // Distribute a width of some BltsWidthType type to a set of columns.
>     //  aWidth: The amount of width to be distributed
>     //  aFirstCol: The index (in the table) of the first column to be
>     //             considered for receiving width
>     //  aColCount: The number of consecutive columns (starting with aFirstCol)
>     //              BTLS_PREF_WIDTH are intended to be used for dividing up
>     //              colspan's min & pref width.  BTLS_FINAL_WIDTH is intended
>     //              to be used for distributing the table's final width across
>     //              all its columns)
>     //  aSpanHasSpecifiedWidth: Should be true iff:
>     //                           - We're distributing a colspanning cell's
>     //                             pref or min width to its columns
>     //                           - The colspanning cell has a specified width.
>-    void DistributeWidthToColumns(nscoord aWidth,
>+    void DistributeISizeToColumns(nscoord aISize,
>-                                  bool aSpanHasSpecifiedWidth);
>+                                  bool aSpanHasSpecifiedISize);

Looks like this documentation needs some s/width/ISize/, too -- especially the explicit mentions of the params that you're renaming ("aWidth:" and "aSpanHasSpecifiedWidth:" -- these no longer exist).
Comment on attachment 8622048 [details] [diff] [review]
Convert BasicTableLayoutStrategy to work with logical coordinates

Review feedback on the .cpp file:

>+++ b/layout/tables/BasicTableLayoutStrategy.cpp
>-        nscoord w = nsLayoutUtils::ComputeWidthValue(aRenderingContext,
>-                                                     aFrame, 0, 0, 0, width);
>+        nscoord w = nsLayoutUtils::ComputeISizeValue(aRenderingContext,
>+                                                     aFrame, 0, 0, 0, iSize);
>         nscoord w =
>-            nsLayoutUtils::ComputeWidthValue(aRenderingContext, aFrame,
>-                                             0, 0, 0, maxWidth);
>+            nsLayoutUtils::ComputeISizeValue(aRenderingContext, aFrame,
>+                                             0, 0, 0, maxISize);

I think "w" is short for "width" here... so, this variable likely needs a rename, now that it's really an ISize. We probably shouldn't use "i", since that'd seem too much like a loop counter. Maybe it should be called "c"? (for "coord" and/or "computed")

(Note that the second "nscoord w" variable quoted here is in a ConvertsToLength() clause, whereas we have "float p" in a eStyleUnit_Percent clause alongside it. I think renaming to "c" seems good for that section, because then we'd have "c" (for coord) juxtaposed up against "p" (for percent).)

This "w" rename could also happen in a separate patch, if you like.

>-        // Consider the widths on the column.
>-        CellWidthInfo colInfo = GetColWidthInfo(aRenderingContext, colFrame);
>+        // Consider the isizes on the column.
>+        CellISizeInfo colInfo =
>+            GetColISizeInfo(aRenderingContext, colFrame, wm);

This wrapping feels awkward to me -- maybe because the 4-space indent in this file makes wrapping like this less useful.

Seems cleaner to just wrap the args, e.g.:
        CellWidthInfo colInfo = GetColWidthInfo(aRenderingContext,
                                                colFrame, wm);

(Maybe it's just me; if you'd like to keep this the way you've got it, that's ok with me too.)

>+            colInfo =
>+                GetColISizeInfo(aRenderingContext, colFrame->GetParent(), wm);

(Same here. Feels particularly awkward here, since the wrapping only buys us a few characters of de-indentation.)

>-            CellWidthInfo info = GetCellWidthInfo(aRenderingContext, cellFrame);
>+            CellISizeInfo info =
>+                GetCellISizeInfo(aRenderingContext, cellFrame, wm);

(Same here.)

>+BasicTableLayoutStrategy::DistributeISizeToColumns(nscoord aISize,
> {
>                  (aFirstCol == 0 && 
>                   aColCount == mTableFrame->GetCellMap()->GetColCount()),
>-            "Computing final column widths, but didn't get full column range");
>+            "Computing final column inline-sizes, but didn't get full column range");

This last line ends up over 80 chars due to the s/widths/inline-sizes/.

Wrap it somewhere (or use "isize") to keep it under 80.

r=me with the above addressed
Attachment #8622048 - Flags: review?(dholbert) → review+
Pushed, with cleanup and extra comment love as requested. :)

Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1358216
You need to log in before you can comment on or make changes to this bug.