Last Comment Bug 716408 - Minor cleanup in table code
: Minor cleanup in table code
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Mats Palmgren (:mats)
:
:
Mentors:
: 333287 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-08 14:01 PST by Mats Palmgren (:mats)
Modified: 2012-01-17 07:37 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove null-checks of GetFirstInFlow() result (2.03 KB, patch)
2012-01-08 14:08 PST, Mats Palmgren (:mats)
bernd_mozilla: review-
Details | Diff | Splinter Review
Remove unnecessary nsTableFrame::GetFirstInFlow() calls to get IsBorderCollapse() bit (6.34 KB, patch)
2012-01-08 14:12 PST, Mats Palmgren (:mats)
bernd_mozilla: review+
Details | Diff | Splinter Review
Make nsTableFrame::GetTableFrame abort if the given frame isn't a table frame descendant (32.46 KB, patch)
2012-01-08 14:19 PST, Mats Palmgren (:mats)
bernd_mozilla: review+
Details | Diff | Splinter Review
Initialize all bits of mBits to zero (1.02 KB, patch)
2012-01-08 14:20 PST, Mats Palmgren (:mats)
bernd_mozilla: review+
Details | Diff | Splinter Review
No need to null-check for delete, no need to null out members in dtor (894 bytes, patch)
2012-01-08 14:22 PST, Mats Palmgren (:mats)
bernd_mozilla: review+
Details | Diff | Splinter Review
Assert that a prev-in-flow is null or of the same frame type. No need to null out mCellMap, the ctor does that and re-Init of frames isn't allowed (2.21 KB, patch)
2012-01-08 14:23 PST, Mats Palmgren (:mats)
bernd_mozilla: review+
Details | Diff | Splinter Review
Indent nsTableFrame::DidSetStyleContext by two spaces, not three (1.97 KB, patch)
2012-01-08 14:35 PST, Mats Palmgren (:mats)
bernd_mozilla: review+
Details | Diff | Splinter Review
Remove null-checks of GetFirstInFlow() result (4.46 KB, patch)
2012-01-15 14:09 PST, Mats Palmgren (:mats)
bernd_mozilla: review+
Details | Diff | Splinter Review

Description Mats Palmgren (:mats) 2012-01-08 14:01:38 PST

    
Comment 1 Mats Palmgren (:mats) 2012-01-08 14:08:15 PST
Created attachment 586835 [details] [diff] [review]
Remove null-checks of GetFirstInFlow() result

GetFirstInFlow() can never return null so we can remove these null-checks
Comment 2 Mats Palmgren (:mats) 2012-01-08 14:12:24 PST
Created attachment 586836 [details] [diff] [review]
Remove unnecessary nsTableFrame::GetFirstInFlow() calls to get IsBorderCollapse() bit

The mIsBorderCollapse bit is set on all table frames in Init so there's
no need to get this bit from the first-in-flow frame.
Comment 3 Mats Palmgren (:mats) 2012-01-08 14:19:19 PST
Created attachment 586840 [details] [diff] [review]
Make nsTableFrame::GetTableFrame abort if the given frame isn't a table frame descendant

Make GetTableFrame() abort if the arg isn't a table frame descendant.
Consequently, remove null-checks of GetTableFrame() results.
Comment 4 Mats Palmgren (:mats) 2012-01-08 14:20:30 PST
Created attachment 586841 [details] [diff] [review]
Initialize all bits of mBits to zero
Comment 5 Mats Palmgren (:mats) 2012-01-08 14:22:21 PST
Created attachment 586842 [details] [diff] [review]
No need to null-check for delete, no need to null out members in dtor
Comment 6 Mats Palmgren (:mats) 2012-01-08 14:23:52 PST
Created attachment 586843 [details] [diff] [review]
Assert that a prev-in-flow is null or of the same frame type. No need to null out mCellMap, the ctor does that and re-Init of frames isn't allowed
Comment 7 Mats Palmgren (:mats) 2012-01-08 14:35:33 PST
Created attachment 586844 [details] [diff] [review]
Indent nsTableFrame::DidSetStyleContext by two spaces, not three
Comment 8 Bernd 2012-01-08 22:28:32 PST
*** Bug 333287 has been marked as a duplicate of this bug. ***
Comment 9 Bernd 2012-01-08 22:45:23 PST
I will only on the weekend (14.01, 15.01) get to this reviews
Comment 10 Bernd 2012-01-15 00:59:37 PST
Comment on attachment 586835 [details] [diff] [review]
Remove null-checks of GetFirstInFlow() result

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

please remove also mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#3311 it doubles 
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSplittableFrame.cpp#197 
Further remove 
http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#6237
http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#6248

The patch is OK in itself, but lets get rid of it all once.
Comment 11 Mats Palmgren (:mats) 2012-01-15 14:09:45 PST
Created attachment 588777 [details] [diff] [review]
Remove null-checks of GetFirstInFlow() result

Hmm, yes, those changes got lost somehow...

I took the opportunity of simplifying GetColumnWidth as well.
Comment 12 Bernd 2012-01-15 21:36:27 PST
Comment on attachment 588777 [details] [diff] [review]
Remove null-checks of GetFirstInFlow() result

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

thats what I wanted.
Comment 13 Bernd 2012-01-15 21:58:16 PST
Comment on attachment 586836 [details] [diff] [review]
Remove unnecessary nsTableFrame::GetFirstInFlow() calls to get IsBorderCollapse() bit

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

yep the master himself was inconsistent with respect to this, http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableRowGroupFrame.cpp&rev=3.411&mark=394#394
Comment 14 Bernd 2012-01-15 22:19:55 PST
Comment on attachment 586840 [details] [diff] [review]
Make nsTableFrame::GetTableFrame abort if the given frame isn't a table frame descendant

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

I like the change to NS_RUNTIMEABORT("unable to find table parent");

Note You need to log in before you can comment on or make changes to this bug.