Closed Bug 1171328 Opened 5 years ago Closed 5 years ago

Convert nsTableFrame::GetChildAreaOffset() and its friends to use LogicalMargin

Categories

(Core :: Layout: Tables, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(1 file, 2 obsolete files)

I started touching code related to this for border-collapse, but it later turns out that this function has some interaction with other part of table code. Hence file a separate bug for it.
Attached patch patch (obsolete) — Splinter Review
Attachment #8615099 - Flags: review?(dholbert)
Attached patch patch, unbitrotted (obsolete) — Splinter Review
I think this bitrotted a tiny bit while waiting for review (partly due to my patches in bug 1169432, and partly due to bug 1147834).

As penance (and to aid in my review), I unbitrotted it; here's the updated version. If you haven't already unbitrotted locally for your own work, this may save you some time.
Comment on attachment 8615099 [details] [diff] [review]
patch

Review feedback for the non-nsTableFrame.cpp files (did those first since there's fewer changes there): just 3 nits, below.

>+++ b/layout/generic/nsHTMLReflowState.cpp
>@@ -2337,17 +2337,19 @@ nsCSSOffsetState::InitOffsets(nscoord aI
>       ComputedPhysicalPadding().SizeTo(0,0,0,0);
>-      ComputedPhysicalBorderPadding() = tableFrame->GetIncludedOuterBCBorder();
>+      LogicalMargin collapseBorder = tableFrame->GetIncludedOuterBCBorder();
>+      ComputedPhysicalBorderPadding() =
>+        collapseBorder.GetPhysicalMargin(tableFrame->GetWritingMode());
>     }

nsHTMLReflowState has logical getters/setters for stuff like this now -- we should just directly use one of those, instead of ComputedPhysicalBorderPadding().

I think something like this should work here:
  SetComputedLogicalBorderPadding(collapseBorder);

...or maybe better, just drop the local variable and do:
  SetComputedLogicalBorderPadding(tableFrame->GetIncludedOuterBCBorder());

(I checked, & it looks like this barely fits in the 80-char limit. :))

>+++ b/layout/tables/nsTableFrame.h
>   void SetColumnDimensions(nscoord         aHeight,
>-                           const nsMargin& aReflowState);
>+                           const LogicalMargin& aBorderPadding);

Please reindent 'aHeight' on the previous line here (either adding space to keep it aligned, or dropping space so it doesn't have any sort of special alignment.)

>+++ b/layout/tables/nsTableOuterFrame.cpp
>@@ -237,17 +237,19 @@ nsTableOuterFrame::InitChildReflowState(
[...]
>   if (aReflowState.frame == InnerTableFrame() &&
>       InnerTableFrame()->IsBorderCollapse()) {
>-    collapseBorder  = InnerTableFrame()->GetIncludedOuterBCBorder();
>+    LogicalMargin border = InnerTableFrame()->GetIncludedOuterBCBorder();
>+    WritingMode wm = InnerTableFrame()->GetWritingMode();
>+    collapseBorder = border.GetPhysicalMargin(wm);
>     pCollapseBorder = &collapseBorder;
>     pCollapsePadding = &collapsePadding;
>   }
>   aReflowState.Init(&aPresContext, -1, -1, pCollapseBorder, pCollapsePadding);

Nit: use aReflowState.GetWritingMode() here instead of InnerTableFrame()->GetWritingMode() -- it's likely faster.  (aReflowState is in a semi-uninitialized state, but we can assume that we've initialized the writing mode in the parent-class constructor, nsCSSOffsetState.)

(side note: this API should really be using Maybe<> instead of local stack variables and maybe-null pointers to those variables. Anyway, that shouldn't happen here; just an observation.)
Comment on attachment 8615099 [details] [diff] [review]
patch

>+++ b/layout/tables/nsTableFrame.cpp
>@@ -74,35 +74,36 @@ struct nsTableReflowState {
>                      nscoord                  aAvailWidth,
>                      nscoord                  aAvailHeight)
>     : reflowState(aReflowState)
>   {
>     MOZ_ASSERT(reflowState.frame->GetType() == nsGkAtoms::tableFrame,
>                "nsTableReflowState should only be created for nsTableFrame");
>     nsTableFrame* table =
>       static_cast<nsTableFrame*>(reflowState.frame->FirstInFlow());
[...]
>+    WritingMode wm = table->GetWritingMode();

As noted at end of comment 3, it's likely faster to get the writing mode from |reflowState| instead of querying the frame.

(nsIFrame's GetWritingMode() method is a virtual function call, and it also has to query the style context [calling StyleVisibility()], and run through some logic to set itself up. All of this makes it more expensive than using [or copying, really] an already-cached WritingMode value.)

This applies to various places in nsTableFrame -- basically, wherever we have a reflow state for the same frame in-scope which we can use.

(so: beyond the code quoted above in nsTableReflowState(), this also applies to nsTableFrame::Reflow, GetSeparateModelBorderPadding, CalcDesiredHeight, DistributeHeightToRows, CalcBorderBoxHeight, and maybe a few other places that I missed.)
Also: for any utility methods in this patch where we *don't* have a reflow-state in-scope (and hence can't just easily take the WritingMode from that) -- e.g. GetOuterBCBorder, GetIncludedOuterBCBorder: Instead of making a local call to GetWritingMode(), those should probably take an additional WritingMode parameter, so that we don't have to make virtual function call to GetWritingMode() in all of these places.

Otherwise, it seems like this will cause a decent bump in the number of (unnecessary) virtual function calls that we need during reflow.

(These functions's callers -- e.g. GetExcludedOuterBCBorder -- may also need to change to take a WritingMode parameter as well.  When we're calling into these functions during Reflow, we should pass down the WritingMode taken from the reflow state.   Callers that aren't during reflow can make a single call to GetWritingMode() and pass that down.)
(jfkthame, could you sanity-check me on comments 4 & 5?

It seems like there's a clear performance win from consolidating calls to nsIFrame::GetWritingMode(), and using the cached value from the reflow state where possible.

This means adding an extra WritingMode parameter to a lot of utility functions (like GetOuterBCBorder, GetIncludedOuterBCBorder, and GetExcludedOuterBCBorder as noted above); this imposes a bit of a messiness penalty, and I'm not sure how bad to feel about that; but I think it's worth doing to avoid taking too much of a perf hit from the logicalization effort.  I'm also curious how strict we've been about this in other logicalization conversions so far.)
Flags: needinfo?(jfkthame)
Comment on attachment 8615099 [details] [diff] [review]
patch

So, this is r- for now, due to virtual function call concerns noted above.

(I skimmed through the nsTableFrame.cpp changes, and I think the arithmetic/physical-to-logical conversions look reasonable.  I'll do a more thorough review once my above comments are addressed, since that'll be changing a lot of this code anyway.)
Flags: needinfo?(quanxunzhen)
Attachment #8615099 - Flags: review?(dholbert) → review-
(In reply to Daniel Holbert [:dholbert] from comment #6)
> (jfkthame, could you sanity-check me on comments 4 & 5?

Yes, they sound sane to me. :)

> It seems like there's a clear performance win from consolidating calls to
> nsIFrame::GetWritingMode(), and using the cached value from the reflow state
> where possible.
> 
> This means adding an extra WritingMode parameter to a lot of utility
> functions (like GetOuterBCBorder, GetIncludedOuterBCBorder, and
> GetExcludedOuterBCBorder as noted above); this imposes a bit of a messiness
> penalty, and I'm not sure how bad to feel about that; but I think it's worth
> doing to avoid taking too much of a perf hit from the logicalization effort.
> I'm also curious how strict we've been about this in other logicalization
> conversions so far.)

I'd agree with this -- and it may well be something we can tune further in places, though in many cases where we know we have the appropriate writing mode in hand, we now pass it in to called methods.

In a situation like this, where we've got a bunch of utility functions that we know will be used from the same context (or so I assume, without reading all the code!), we should definitely avoid having them call nsIFrame::GetWritingMode() individually; as you observe, that has a non-trivial cost.
Flags: needinfo?(jfkthame)
It seems my original patch is perfectly merged by kdiff3 on top of your patches for bug 1169432.
Attached patch patchSplinter Review
Attachment #8615099 - Attachment is obsolete: true
Attachment #8616347 - Attachment is obsolete: true
Flags: needinfo?(quanxunzhen)
Attachment #8616509 - Flags: review?(dholbert)
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #9)
> It seems my original patch is perfectly merged by kdiff3 on top of your
> patches for bug 1169432.

(Nice. I had just used "hg qimport" which is pickier.)
(In reply to Daniel Holbert [:dholbert] from comment #3)
> (side note: this API should really be using Maybe<>

(I filed bug 1172616 on this, btw.)
Comment on attachment 8616509 [details] [diff] [review]
patch

Review of attachment 8616509 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/tables/nsTableFrame.cpp
@@ +2667,3 @@
>  nsTableFrame::GetChildAreaOffset(const nsHTMLReflowState* aReflowState) const
>  {
> +  WritingMode wm = aReflowState->GetWritingMode();

I don't think this can work as things stand, because a couple of BCPaintBorderIterator methods call GetChildAreaOffset but pass nullptr for aReflowState.
Comment on attachment 8616509 [details] [diff] [review]
patch

>+++ b/layout/generic/nsHTMLReflowState.cpp
>@@ -2388,17 +2388,18 @@ nsCSSOffsetState::InitOffsets(const Logi
>       ComputedPhysicalPadding().SizeTo(0,0,0,0);
>-      ComputedPhysicalBorderPadding() = tableFrame->GetIncludedOuterBCBorder();
>+      SetComputedLogicalBorderPadding(
>+        tableFrame->GetIncludedOuterBCBorder(GetWritingMode()));

No need to use the public getter here; just pass mWritingMode.

[Looks like we pass mWritingMode a bit more frequently than using the public getter in this file.  Not sure why we're using the getter elsewhere at all.  But anyway, mWritingMode is more common in this file, shorter, & clearer about its cost, so I'd rather we go with that.]

>+++ b/layout/tables/nsTableFrame.cpp
[...]
>+LogicalMargin
>+nsTableFrame::GetOuterBCBorder(const WritingMode aWM) const
> {
[...]
>   BCPropertyData* propData = GetBCProperty();
>   if (propData) {
>-    border.top = BC_BORDER_START_HALF_COORD(p2t, propData->mTopBorderWidth);
[...]
>-  }
>-  return border;
>+    return LogicalMargin(
>+      aWM,
>+      BC_BORDER_START_HALF_COORD(p2t, propData->mTopBorderWidth),
>+      BC_BORDER_END_HALF_COORD(p2t, propData->mRightBorderWidth),
>+      BC_BORDER_END_HALF_COORD(p2t, propData->mBottomBorderWidth),
>+      BC_BORDER_START_HALF_COORD(p2t, propData->mLeftBorderWidth));
>+  }
>+  return LogicalMargin(GetWritingMode());
>+}

If you like: might be worth inverting the "if (propData)" check in this function, and also in GetIncludedOuterBCBorder & GetSeparateModelBorderPadding, to make the early-return a 1-liner instead of a 6-liner. That lets us save on indentation for the more complex code. 

(If you make this change, it'd probably be best to give it its own patch, so that this main patch is more clearly a drop-in replacement & doesn't have any code-shuffling.)

>+static LogicalMargin
>+GetSeparateModelBorderPadding(const nsHTMLReflowState* aReflowState,
>+                              nsStyleContext* aStyleContext)
> {
>   // XXXbz Either we _do_ have a reflow state and then we can use its
>   // mComputedBorderPadding or we don't and then we get the padding
>   // wrong!
>-  const nsStyleBorder* border = aStyleContext.StyleBorder();
>-  aBorderPadding = border->GetComputedBorder();
>+  const nsStyleBorder* border = aStyleContext->StyleBorder();
>+  LogicalMargin borderPadding(WritingMode(aStyleContext),
>+                              border->GetComputedBorder());

This function (GetSeparateModelBorderPadding) should take aWM as an arg, instead of doing "WritingMode(aStyleContext)".  Its only caller, GetChildAreaOffset, already knows our WritingMode, so it shouldn't make this static helper rebuild it.

>+LogicalMargin
> nsTableFrame::GetChildAreaOffset(const nsHTMLReflowState* aReflowState) const
> {
[...]
>+  WritingMode wm = aReflowState->GetWritingMode();

aReflowState is allowed to be null here. So, this call may crash.

But -- its callers that pass in a null reflowstate pointer (in BCPaintBorderIterator) *do* already have the writing-mode cached, as "mTableWM". So, this function should probably just take a writing-mode parameter (join the club!). The BCPaintBorderIterator callers can pass mTableWM, and the other callers can pass in their reflow state's cached writing-mode.

>+  return IsBorderCollapse() ? GetIncludedOuterBCBorder(wm) :
>+    GetSeparateModelBorderPadding(aReflowState, mStyleContext);

As noted above, we should be passing the writing mode to GetSeparateModelBorderPadding here.

(I slightly wonder if we should just fold GetSeparateModelBorderPadding into this function, since it's a static helper with only one caller -- this method -- and this method is now becoming basically a one-liner. Anyway, that folding should probably happen separately from this patch, if it happens.)

>@@ -6460,25 +6469,25 @@ BCPaintBorderIterator::SetDamageArea(con
>   if (mTableWM.IsBidiLTR()) {
>-    mInitialOffsetX = childAreaOffset.left; // x position of first col in
>-                                            // damage area
>+    // x position of first col in damage area
>+    mInitialOffsetX = childAreaOffset.IStart(mTableWM);
>     leftCol = 0;
>     rightCol = mNumTableCols;
>   } else {
>     // x position of first col in damage area
>-    mInitialOffsetX = mTable->GetRect().width - childAreaOffset.right;
>+    mInitialOffsetX = mTable->GetRect().width - childAreaOffset.IEnd(mTableWM);

I'm pretty sure the IStart vs. IEnd distinction is wrong here, as replacements for left/right, given that the "if" conditional is testing the writing-mode's direction. ("IsBidiLTR").  Our LogicalMargin should already have the LTR vs. RTL inline-axis directionality baked into it already.  So this probably really needs to be IStart() in both cases. (not IEnd)

>   if (!mTableWM.IsBidiLTR()) {
>     uint32_t temp;
>-    mInitialOffsetX = mTable->GetRect().width - childAreaOffset.right;
>+    mInitialOffsetX = mTable->GetRect().width - childAreaOffset.IEnd(mTableWM);

Similarly, I think this "right" wants to be replaced with IStart, not IEnd, since we only hit this code if we're RTL. (in which case .right is the IStart side)

r=me with the above
Attachment #8616509 - Flags: review?(dholbert) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #13)
> I don't think this can work as things stand

(Yup, I think GetChildAreaOffset needs to join the take-a-WritingMode club, as noted in comment 14.)
(Not sure how previous comment ended up being double-posted; sorry for spam.)
https://hg.mozilla.org/mozilla-central/rev/2902d065f84a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.