Summary: | Add simplified layout path optimization for overflow recomputation and for positioned objects inside relative positioned containers. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dave Hyatt <hyatt> | ||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Dave Hyatt <hyatt> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | abarth, benjamin, eric, simon.fraser, webkit.review.bot, yutak | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||
Bug Depends on: | 56963 | ||||||||||||||||||||||
Bug Blocks: | 58176, 58500 | ||||||||||||||||||||||
Attachments: |
|
Description
Dave Hyatt
2011-03-23 02:18:25 PDT
Created attachment 86590 [details]
Patch
Attachment 86590 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/style/RenderStyle.cpp:500: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 86590 [details]
Patch
Clearing flags, since there are some things I want to clean up first.
Created attachment 86599 [details]
Patch
Attachment 86599 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/style/RenderStyle.cpp:496: Should only a single space after a punctuation in a comment. [whitespace/comments] [5]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 86628 [details]
Patch
Attachment 86628 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/style/RenderStyle.cpp:496: Should only a single space after a punctuation in a comment. [whitespace/comments] [5]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 86662 [details]
Patch updated to ToT
Attachment 86662 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/RenderBox.cpp:293: More than one command on the same line [whitespace/newline] [4]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 86665 [details]
Patch updated to ToT
Comment on attachment 86665 [details] Patch updated to ToT View in context: https://bugs.webkit.org/attachment.cgi?id=86665&action=review > Source/WebCore/ChangeLog:16 > + a transform changes, this is the hint returned now instead of a full layout. It looks like you are not handling opacity changes in this version of the patch. > Source/WebCore/rendering/RenderObject.h:-999 > - if (last->style()->top().isAuto() && last->style()->bottom().isAuto()) { > - RenderObject* parent = last->parent(); > - if (!parent->normalChildNeedsLayout()) { > - parent->setChildNeedsLayout(true, false); > - if (parent != newRoot) > - parent->markContainingBlocksForLayout(scheduleRelayout, newRoot); > - } > - } I am concerned about this change. It’s not clear to me how the new code added in RenderBox correctly accounts for the need to mark parents of the statically-positioned object. There were subtle cases when failing to do this cause the layout root to not be set correctly. I know that you pass all tests, but I am still worried that the tests only partially cover this. Comment on attachment 86665 [details]
Patch updated to ToT
I can’t really come up with a case that is covered by the two-chain marking, so given that existing tests pass, I am going to r+
Yeah, as mentioned in IRC, the old code would two-chain mark when removals/additions occurred in the tree. The old code would also two-chain mark whenever anything changed that resulted in a layout and not just margins. I think the scope of the new code should protect us from risk. Other browsers don't even pass the dynamic margin change causing static position to update, so I doubt it's commonly used on the Web. :) Fixed in r81802. http://trac.webkit.org/changeset/81802 might have broken Leopard Intel Release (Tests) The following tests are not passing: animations/state-at-end-event.html Rolling this out for now. There are complications with staticX that get more obvious when you try to extend the optimization. Attaching a test case that illustrates a failure even without this patch. Created attachment 86707 [details]
Test case that even fails in ToT
Created attachment 86708 [details]
Test case that even fails in ToT
Created attachment 86866 [details]
Patch
Attachment 86866 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1
Source/WebCore/rendering/RenderBlockLineLayout.cpp:2091: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 59 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 86963 [details]
Patch that consists only of the delta from the last r+ patch
I put up an additional patch that shows the additions since the last r+. Hopefully that makes the main patch easier to review. Comment on attachment 86866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86866&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:879 > + for (unsigned i = 0; i < positionedObjects.size(); ++i) size() returns a size_t so we typically use a size_t for the index variable > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2098 > + for (unsigned i = 0; i < trailingPositionedBoxes.size(); ++i) { same comment about size_t Landed in r81992. Hi, I think this patch is causing a lot of crashes in Chromium. (More accurately, there is a suspicious revision in r81992:r81997; I think it is this patch, but I cannot confirm right now because I don't have build environment at hand.) Crash trace looks like: http://build.chromium.org/p/chromium.webkit/builders/Win%20Reliability/builds/1200/steps/reliability%3A%20partial%20result%20of%20current%20build/logs/stdio Do you have any idea about this? (In reply to comment #25) > Hi, > > I think this patch is causing a lot of crashes in Chromium. > > (More accurately, there is a suspicious revision in r81992:r81997; I think it is this patch, but I cannot confirm right now because I don't have build environment at hand.) > > Crash trace looks like: > http://build.chromium.org/p/chromium.webkit/builders/Win%20Reliability/builds/1200/steps/reliability%3A%20partial%20result%20of%20current%20build/logs/stdio > > Do you have any idea about this? Will look into it. Filed https://bugs.webkit.org/show_bug.cgi?id=57217 to cover the crash reports. This caused bug 61174. |