Last Comment Bug 507419 - nsTableRowFrame::AppendFrames and InsertFrames are not consistent in terms of ordering
: nsTableRowFrame::AppendFrames and InsertFrames are not consistent in terms of...
Status: RESOLVED FIXED
[good first bug] [mentor=bernd] [lang...
:
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Andrew Quartey [:drexler]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-30 10:22 PDT by Boris Zbarsky [:bz]
Modified: 2012-01-04 11:33 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
InsertFrames Reordering (2.33 KB, patch)
2011-11-21 16:36 PST, Andrew Quartey [:drexler]
no flags Details | Diff | Review
InsertFrames Reordering (2.41 KB, patch)
2011-11-23 11:21 PST, Andrew Quartey [:drexler]
no flags Details | Diff | Review
Patch v3 (3.87 KB, patch)
2011-12-09 18:11 PST, Andrew Quartey [:drexler]
bernd_mozilla: feedback+
Details | Diff | Review
Patch v4 (3.92 KB, patch)
2011-12-16 10:27 PST, Andrew Quartey [:drexler]
bernd_mozilla: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] 2009-07-30 10:22:40 PDT
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 2011-11-13 11:56:59 PST
There is no reason the order does not matter, yes we should make them consistent
Comment 2 Andrew Quartey [:drexler] 2011-11-21 16:36:22 PST
Created attachment 576029 [details] [diff] [review]
InsertFrames Reordering

Made ordering in InsertFrames consistent with AppendFrames.
Comment 3 Bernd 2011-11-21 23:16:56 PST
Why did you remove bz's query comment without fixing the issue?
Comment 4 Andrew Quartey [:drexler] 2011-11-23 11:17:29 PST
(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] 2011-11-23 11:21:25 PST
Created attachment 576563 [details] [diff] [review]
InsertFrames Reordering
Comment 6 Bernd 2011-11-27 09:11:55 PST
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] 2011-12-09 18:11:45 PST
Created attachment 580599 [details] [diff] [review]
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 2011-12-11 01:26:00 PST
> 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 2011-12-11 01:28:43 PST
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] 2011-12-11 16:02:50 PST
(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] 2011-12-16 10:27:09 PST
Created attachment 582311 [details] [diff] [review]
Patch v4

Amended the assertion message and tested on try: https://tbpl.mozilla.org/?tree=Try&rev=dc0fc2502cc1.
Comment 12 Bernd 2011-12-17 03:26:54 PST
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] 2011-12-17 08:16:41 PST
Sure. I will keep an eye out for those bugs that might crop up. :-)
Comment 15 Matt Brubeck (:mbrubeck) 2011-12-27 11:26:30 PST
https://hg.mozilla.org/mozilla-central/rev/1b2730e56e6f
Comment 16 Bernd 2011-12-28 13:29:59 PST
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] 2012-01-04 11:33:41 PST
Thanks Bernd.

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