1169432 - Modernize nsTableFrame a bit

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Daniel Holbert [:dholbert]]

When reading through our table reflow code, I noticed some opportunities to modernize the code. Filing this bug to cover a few patches I wrote for this.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: use nsAutoPtr for strongly-owned dynamically-allocated members

nsTableFrame has some strongly-owned, dynamically allocated members data --
mCellMap and mTableLayoutStrategy.  (only stored on first-in-flow -- all continuations just have null pointers and reference these from the first-in-flow)

These should be nsAutoPtr so we don't have to bother with explicit deletion. This patch makes it so.

(Tangent: I actually tried UniquePtr (the new hotness) at first, but had some trouble getting "mTableLayoutStrategy = MakeUnique<BasicTableLayoutStrategy>(this)" to return, since MakeUnique returns something templated with BasicTableLayoutStrategy, whereas mTableLayoutStrategy would have type UniquePtr<nsITableLayoutStrategy>, and those templated types weren't interconvertable. So, I just stuck with good ol' nsAutoPtr.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 1: use nsAutoPtr for strongly-owned dynamically-allocated members [Not landing, see bug 1172166]

(Pulled out some unrelated if/else bracing from this patch; meant for those to be in the next patch.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 2: refactor logic in nsTableFrame::Init

This patch improves nsTableFrame::Init, in particular:
 - It now has a precondition for *both* dynamically-allocated vars that it's responsible for setting (instead of just one)
 - The logic flow was formerly weird -- we had duplicated aPrevInFlow checks. (one negated, one not).  This patch collapses that to a single check, with "first in flow" vs "not first in flow" cases.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 3: Convert a nsTArray "IndexOf() != NoIndex" check to "Contains" for brevity

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 4: Convert NS_ASSERTION(false, ...) to NS_ERROR(...)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: part 5: Use range-based 'for' loops when walking frame lists

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 8612574 [details] [diff] [review]
part 1: use nsAutoPtr for strongly-owned dynamically-allocated members [Not landing, see bug 1172166]

I'm not sure this is the best design for these members.  Both
nsTableFrame::GetCellMap() / LayoutStrategy() are implemented as
"return FirstInFlow()->member" which is suboptimal when we have
these members on all continuations.  We could optimize away the
virtual FirstInFlow() calls by replicating these pointers instead.

GetCellMap() / LayoutStrategy() are called in *a lot* of places.

Comment 8

[Daniel Holbert [:dholbert]]

(oops, missed the bugmail for comment 7 & only just now noticed it; sorry for lag in reply.)

So I think you're suggesting:
 - These pointer values should perhaps be copied across all of the in-flow frames
 - hence, the pointer needs to stay weak, with manual deletion on the first-in-flow
 - hence, we shouldn't take 'part 1', because that prevents us from doing that

If so: that's fine with me, I'm happy to obsolete 'part 1' & file a followup about replicating these pointer-values.

Comment 9

[Mats Palmgren (inactive)]

Yes, I think that would be worth doing.  Please CC me on the new bug.

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8612576 [details] [diff] [review]
part 2: refactor logic in nsTableFrame::Init

Feel free to change all these NS_PRECONDITIONs to MOZ_ASSERTs.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 8612580 [details] [diff] [review]
part 4: Convert NS_ASSERTION(false, ...) to NS_ERROR(...)

Feel free to change all these NS_ERRORs to MOZ_ASSERT_UNREACHABLEs.

The description from mfbt/Assertions.h is:

/*
 * Unconditional assert in debug builds for (assumed) unreachable code paths
 * that have a safe return without crashing in release builds.
 */

which seems appropriate in this case.

Comment 12

[Daniel Holbert [:dholbert]]

Filed bug 1172166 for comment 7 / comment 8.

(In reply to Mats Palmgren (:mats) from comment #10)
> Feel free to change all these NS_PRECONDITIONs to MOZ_ASSERTs.
[...]
> Feel free to change all these NS_ERRORs to MOZ_ASSERT_UNREACHABLEs.

Thanks -- I'm not going to promote them to be fatal at this point, I think.

(roc posted somewhere a while back about non-fatal assertions (with safe return paths) being preferable to fatal assertions, for debuggability & for a happier experience when running debug builds for browsing. Fatal assertions have a tradeoff between being more likely to be noticed, vs. being more likely to ruin someone's day by terminating their debugging session and/or debug-browser-dogfooding [and possibly blocking them from hitting the actual error they're trying to investigate]. It's possible these particular assertions are guaranteed-to-succeed enough that they can safely be made fatal, but I'll punt on that analysis/conversion for now.)

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d576f294df12
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed236855a3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/a14d4adad0e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/b84888e85411

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d576f294df12
https://hg.mozilla.org/mozilla-central/rev/8ed236855a3a
https://hg.mozilla.org/mozilla-central/rev/a14d4adad0e6
https://hg.mozilla.org/mozilla-central/rev/b84888e85411