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

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Bernd]

There is no reason the order does not matter, yes we should make them consistent

Comment 2

[Andrew Quartey [:drexler]]

Attachment: InsertFrames Reordering

Made ordering in InsertFrames consistent with AppendFrames.

Comment 3

[Bernd]

Why did you remove bz's query comment without fixing the issue?

Comment 4

[Andrew Quartey [:drexler]]

(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.

Comment 5

[Andrew Quartey [:drexler]]

Attachment: InsertFrames Reordering

Comment 6

[Bernd]

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())) {
      cellChildren.AppendElement(childFrame);
    }
  }

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 mxr.mozilla.org for the nsTableRowFrame.cpp (http://hg.mozilla.org/mozilla-central/annotate/36b62de19e9e/layout/tables/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 https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide
"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?

Comment 7

[Andrew Quartey [:drexler]]

Attachment: Patch v3

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.

Comment 8

[Bernd]

> 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. 
http://www.mozilla.org/hacking/commit-access-policy/
and http://www.mozilla.org/hacking/committer/

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

The paper work needs some time..

Comment 9

[Bernd]

Comment on attachment 580599 [details] [diff] [review]
Patch v3

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

and this OK.

Comment 10

[Andrew Quartey [:drexler]]

(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.

Comment 11

[Andrew Quartey [:drexler]]

Attachment: Patch v4

Amended the assertion message and tested on try: https://tbpl.mozilla.org/?tree=Try&rev=dc0fc2502cc1.

Comment 12

[Bernd]

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.

Comment 13

[Andrew Quartey [:drexler]]

Sure. I will keep an eye out for those bugs that might crop up. :-)

Comment 14

[Bernd]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b2730e56e6f

Comment 15

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/1b2730e56e6f

Comment 16

[Bernd]

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 http://www-archive.mozilla.org/newlayout/doc/fosdem2004/slide1.html</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.

Comment 17

[Andrew Quartey [:drexler]]

Thanks Bernd.