Rename "computedPaginatedHeight" to "computedUnpaginatedHeight", and move it inside the only clause where it's used

RESOLVED FIXED

Status

()

Core
Layout: Tables
--
minor
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 363025 [details] [diff] [review]
patch v1

In browsing nsTableCellFrame::Reflow, I noticed that the two things about the "computedPaginatedHeight" temporary variable: 
  (a) it's really the *un*-paginated height (for use during pagination)
  (b) it's declared at the top level, but it's only used inside of a very small clause.

The attached patch addresses this by (a) renaming the variable and (b) moving the variable declaration into the tiny clause where it's actually used.  (I've also cleaned up a few really long lines in the surrounding code to get them under 80 characters.)
(Assignee)

Updated

10 years ago
Summary: Rename "computedPaginatedHeight" to "computedUnpaginatedHeight", and move it inside of the only clause where it's used → Rename "computedPaginatedHeight" to "computedUnpaginatedHeight", and move it inside the only clause where it's used

Comment 1

10 years ago
I am pretty happy about  (a) and (b) but I think that the introduction of the local variable to get the lines under 80 chars makes the code less clear than before. 

The called code at
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableCellFrame.cpp&rev=3.414&mark=774,775,785-793#763

looks like a performance penalty. We first look up a the row parent of the cell. Its naturally the first row, and then instead of starting the loop there we loop over *all* rows till we find the first row that is covered by the cell.
(Assignee)

Comment 2

9 years ago
Created attachment 389548 [details] [diff] [review]
patch v2

(In reply to comment #1)
> I am pretty happy about  (a) and (b) but I think that the introduction of the
> local variable to get the lines under 80 chars makes the code less clear than
> before.

Ok -- this version strips out the local variable inclusion.

When recompiling nsTableCellFrame.cpp, I also noticed 2 unrelated compiler warnings of the form: "warning: format ‘%p’ expects type ‘void*’, but argument 2 has type ‘nsIFrame*’".  These are for debugging printf statements that print pointer values (memory addresses).  This new patch version fixes those warnings, too, using static_cast<void*>. (A quick mxr search for "static_cast<void*>" shows that we already use this workaround for this compiler warning in a lot of other printf statements.)

> The called code at
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableCellFrame.cpp&rev=3.414&mark=774,775,785-793#763
> 
> looks like a performance penalty

Yup -- sounds like a different bug, though.
Attachment #363025 - Attachment is obsolete: true
Attachment #389548 - Flags: review?(bernd_mozilla)

Updated

9 years ago
Attachment #389548 - Flags: review?(bernd_mozilla) → review+

Comment 3

9 years ago
Comment on attachment 389548 [details] [diff] [review]
patch v2

please file the followup bug
(Assignee)

Comment 4

9 years ago
Filed bug 505519 on the perf penalty that Bernd noticed.
(Assignee)

Comment 5

9 years ago
Patch landed:
http://hg.mozilla.org/mozilla-central/rev/b1d9fe5a27e6
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

9 years ago
Created attachment 389747 [details] [diff] [review]
patch as landed

I noticed & removed a trailing space character in one of my modified lines before landing.  The line was:
> +      CalcUnpaginagedHeight(aPresContext, (nsTableCellFrame&)*this, 

Here's the corrected patch (as landed), for the record.
Attachment #389548 - Attachment is obsolete: true
Attachment #389747 - Flags: review+
Any reason for the opt printf? (just wondering)
(Assignee)

Comment 8

8 years ago
I'd suggest checking hg history and asking whoever added it. :)  (I just tweaked those printf's to fix warnings, as noted in comment 2 -- I don't know why they're there).
You need to log in before you can comment on or make changes to this bug.