Closed Bug 507419 Opened 11 years ago Closed 8 years ago

nsTableRowFrame::AppendFrames and InsertFrames are not consistent in terms of ordering


(Core :: Layout: Tables, defect)

Not set





(Reporter: bzbarsky, Assigned: drexler)


(Whiteboard: [good first bug] [mentor=bernd] [lang=c++])


(1 file, 3 obsolete files)

The former adds the new frames to mFrames, then calls AppendCell on the table.

The latter does InsertCells on the table first, then does the insert into mFrames.

Is there a reason for this?  Should we make them consistent if we can?
There is no reason the order does not matter, yes we should make them consistent
Whiteboard: [good first bug] [mentor=bernd] [lang=c++]
Attached patch InsertFrames Reordering (obsolete) — Splinter Review
Made ordering in InsertFrames consistent with AppendFrames.
Attachment #576029 - Flags: review?(bernd_mozilla)
Why did you remove bz's query comment without fixing the issue?
(In reply to Bernd from comment #3)
> Why did you remove bz's query comment without fixing the issue?

Removing the comment was an oversight but as to why i didn't address it, i had been made to understand in one of my earlier patch submissions that i had to stick to the original requirements of the bug; in this case, the ordering of frames insertion. I will resend an update patch to address it then.
Attached patch InsertFrames Reordering (obsolete) — Splinter Review
Attachment #576029 - Attachment is obsolete: true
Attachment #576563 - Flags: feedback?(bernd_mozilla)
Attachment #576029 - Flags: review?(bernd_mozilla)
The ordering is the easy part, thats why I though its good to teach the practices. The QI dance has some subtle twists as basically Boris is communicating with me trough code. To understand the conversation I give some background:

The initial code looked like this 
  nsVoidArray cellChildren;	
  for (nsIFrame* childFrame = aFrameList; childFrame;
       childFrame = childFrame->GetNextSibling()) {
    if (IS_TABLE_CELL(childFrame->GetType())) {

and it got morphed in  bug 474369 where roc requested then the QI 
and this wish was full filled in bug 481932.

You can follow this by looking at hg blame at for the nsTableRowFrame.cpp (

Obviously Boris did not like it and added the comment in bug 281387.

The original code was necessary as we had multiple occasions where non table cells where fed into those routines as a result of a failing pseudo frame handling. 

Changing this code as you do makes the bold statement: our pseudo frame handling is correct. That is not obvious. But Boris and I do know it. 

I would prefer if you assert before that IS_TABLE_CELL(Frame->GetType()) is true and give the message that the pseudo frame handling went wrong if this condition is violated. The benefit of this is that later we can remove the other checks for IS_TABLE_CELL that take place in nsTableRowFrame.

By this you replace the check that QI does with an assert, which I believe is fine as we did not have a pseudo frame crash for quite some time. If the pseudo frame handling goes wrong it usually sooner or later crashes.  With the assert finding the reason for a crash is a nobrainer.

The AppendFrame function has the same pattern and fixing only at one point violates
"When fixing a problem, check to see if the problem occurs elsewhere in the same file, and fix it everywhere if possible."

How did you test this? A reasonable local testing would be run the crash tests in layout/tables and make a local reftest run (either entirely or only the table related directories + bugs). Do you know how to do this? After this has been completed it should go over to the try-server.
Are you able to push to try?
Attached patch Patch v3 (obsolete) — Splinter Review
Thanks for the background information related to testing it. Based on what you told me, i readjusted the patch and ran all the crashtests and reftests in the layout directory. I can't push to try yet.
Assignee: nobody → andrew.quartey
Attachment #576563 - Attachment is obsolete: true
Attachment #580599 - Flags: feedback?(bernd_mozilla)
Attachment #576563 - Flags: feedback?(bernd_mozilla)
> I can't push to try yet.

Then you should apply for try access :-), there is some sort of asymmetry between being Friend of the Tree and not having level 1 access.

and ask jdm to vouch, if he refuses ?? ask me.

The paper work needs some time..
Comment on attachment 580599 [details] [diff] [review]
Patch v3

s/Not a table frame/pseudo frame construction failure/

and this OK.
Attachment #580599 - Flags: feedback?(bernd_mozilla) → feedback+
(In reply to Bernd from comment #8)

> and ask jdm to vouch, if he refuses ?? ask me.
Sure..jdm vouched. I should get access sometime this week and then push this patch to try thereafter.
Attached patch Patch v4Splinter Review
Amended the assertion message and tested on try:
Attachment #580599 - Attachment is obsolete: true
Attachment #582311 - Flags: review?(bernd_mozilla)
Comment on attachment 582311 [details] [diff] [review]
Patch v4

Review of attachment 582311 [details] [diff] [review]:

s/table frame/table cell frame/

as we also have table frames and seeing them here would trigger the assert

I will do the check in after the next liftoff (20th dec.). The check in will go to inbound first. The reason for the delay is that I would like to have 6 weeks for dealing with follow up bugs without asking for aurora approval. I will CC you on the bugs that might come. Mozilla has a bunch of fuzzers with automation behind that will fire when new asserts appear.  I consider it good style that if you add an assert to bother also with the asserts that pop up. For me adding an assert is equivalent to: if this happens it is bad, really bad, I would like to know it and to deal with it.
Attachment #582311 - Flags: review?(bernd_mozilla) → review+
Sure. I will keep an eye out for those bugs that might crop up. :-)
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Andrew it looks like your patch did stick. Just let me recap what I hope you did learn from this bug

1. review is typically a multistage process
2. you need to test your patches on try
3. you have learned to use the full hg tool chain
4. there are timing constraints with checkins when you have enough time to deal with the fallout
5. and hopefully you see that layout people are friendly

Things that where not covered: 
1. debugging to understand the problem
2. writing a reftest/crashtest to ensure that this will never be broken again. 

I am not sure that such bugs would qualify as good first bugs but If you/or anybody else is interested I would love to route people through another bug where those skills are required.
<cheap teaser>I wrote</cheap teaser>

I hope that the next one will not be required:
How to deal with the regressions bugs. (short: fix them)

I hope you enjoyed the ride, at least I did.
Thanks Bernd.
You need to log in before you can comment on or make changes to this bug.