Bug 33600 - RenderObject::nextInPreOrderAfterChildren(RenderObject* stayWithin) does not stay within
Summary: RenderObject::nextInPreOrderAfterChildren(RenderObject* stayWithin) does not ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Carol Szabo
URL:
Keywords:
Depends on:
Blocks: 11031
  Show dependency tree / graph
 
Reported: 2010-01-13 08:20 PST by Carol Szabo
Modified: 2010-01-13 12:40 PST (History)
3 users (show)

See Also:

Attachments
Proposed patch (1.84 KB, patch)
2010-01-13 12:09 PST, Carol Szabo 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Carol Szabo 2010-01-13 08:20:03 PST 
This function does not perform as expected, if the stayWithin renderer has a nextSibling. The function when called repeatedly shall traverse till the end of the tree.
This may not have many visible consequences (except for CSS counters which are broken anyway until 32884 is fixed), but it should have a definite impact on performance.
Comment 1 Carol Szabo 2010-01-13 08:21:43 PST 
After 32884 is fixed this w3 test still does not pass as a result of this bug:
http://www.w3.org/Style/CSS/Test/CSS2.1/current/html4/counters-scope-implied-001.htm
Comment 2 Darin Adler 2010-01-13 08:59:03 PST 
We should fix this bug right away! Seems pretty straightforward. Presumably the code should match Node::traverseNextSibling which is doing the same sort of thing.
Comment 3 Carol Szabo 2010-01-13 09:02:00 PST 
I shall submit a patch within the next hour. With no LayoutTest attached if this is OK.
Comment 4 Darin Adler 2010-01-13 09:43:36 PST 
(In reply to comment #3)
> I shall submit a patch within the next hour. With no LayoutTest attached if
> this is OK.

We can accept the patch with no regression test on the basis that the counters test case will cover it in the near future.

Normally we go out of our way to avoid this sort of thing, and find a creative way to demonstrate the bug, but I assume you have not found way to test this before adding the new counters code.
Comment 5 Carol Szabo 2010-01-13 10:46:02 PST 
I made a fix, but apparently it crashes all editing/deleting LayoutTests. I'll have to look into this so the fix will take longer than expected.
Comment 6 Carol Szabo 2010-01-13 12:09:32 PST 
Created attachment 46482 [details]
Proposed patch

To the best of my knowledge the code I changed here conforms to the WebKit style specification, but the file itself has many style problems which I did not undertake to fix.
Comment 7 Carol Szabo 2010-01-13 12:13:56 PST 
(In reply to comment #5)
> I made a fix, but apparently it crashes all editing/deleting LayoutTests. I'll
> have to look into this so the fix will take longer than expected.

The problems I noticed with the editing/deleting LayoutTests were due to a typo in my code which I fixed  and verified before I have submitted the patch. Sorry for taking so long for such a trivial issue.
Comment 8 WebKit Commit Bot 2010-01-13 12:40:29 PST 
Comment on attachment 46482 [details]
Proposed patch

Clearing flags on attachment: 46482

Committed r53196: <http://trac.webkit.org/changeset/53196>
Comment 9 WebKit Commit Bot 2010-01-13 12:40:35 PST 
All reviewed patches have been landed.  Closing bug.