Summary: | CSS Counter Nesting still does not work according to the spec. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carol Szabo <carol> | ||||||||||||
Component: | CSS | Assignee: | Carol Szabo <carol> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bdakin, commit-queue, darin, eric, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 11031, 31814 | ||||||||||||||
Attachments: |
|
Description
Carol Szabo
2009-11-20 08:42:46 PST
Created attachment 43583 [details]
Test Case
I tried to attach this with the bug creation, but the attachment was dropped.
Created attachment 43589 [details]
Proposed Patch
This patch is an intermediary step towards fixing 11031. So it addresses some of the issues Darin raised there and fixes the findPlaceForCounter algorithm.
Created attachment 43740 [details]
Proposed Patch
This is the first part of a comprehensive patch to 11031
Comment on attachment 43740 [details] Proposed Patch > + // We found a reset counter that is on a renderer that is a sibling of ours or a parent. > + if (isReset && (currentRenderer->parent() == counterOwner->parent())) { Formatting nit: We normally don't use parentheses in this common type of expression, and later in this same function this patch does not. > + // CurrentCounter, the counter at the EndSearchRenderer is not reset. Missing comma after the word "EndSearchRenderer" in this comment. > + if (!isReset || (currentRenderer->parent() != counterOwner->parent())) { Formatting nit: We normally don't use parentheses in this common type of expression, and later in this same function this patch does not. > + // We are no longer interested in previous siblings of the currentRenderer or their children > + // as counters they may have attached cannot be the previous sibling of the counter we are placing Missing period here. > + // This function is designed so that the same test is not done twice in an iteration, except for this one > + // which may be done twice in some cases. I think this design choice has made the function a little harder to read than most with more repeated code than I would like to see. Created attachment 44101 [details]
Proposed Patch
I have addressed all style related comments that Darin made.
I did not address the readability issue as my understanding is that Darin's concern not withstanding the current form is acceptable and I wasn't able to find a solution that would make the code significantly smaller and more readable.
One thought that I had was to codify in all 7 decision points as bits in an offset and build a decision vector of chars, where the 8 bits would represent the about 8 different actions in the algorithm. This way the code size would be probably the same, maybe somewhat smaller, the speed might be slightly lower, but probably not significantly, 90% of the code would be easy to read, the entire complexity of the algorithm being packaged in the decision vector which would be as hard to figure out as the current code is to read if not harder hence in the end I decided against this solution.
If I have not made my self clear above here is what I had in mind:
const char ActionSetParentToCurrent=1;
const char ActionSetParentToCurrentsParent=2;
const char ActionSetPreviousSiblingToCurrent=4;
const char ActionReturnParent=8;
.....
const char DecisionIsReset=1
const char DecisionIsCurrentReset=2
const char DecisionCurrentAndOwnerSiblings=4;
const char DecisionCurrentIsEndOfSearch=8;
.....
const char decisionVector[128] = {
0,
0,
ActionSetPreviousSiblingToCurrent,
ActionSetPreviousSiblingToZero,
ActionSetPreviousSiblingToZero | ActionSetParentToCurrent,
ActionSetPreviousSiblingToZero | ActionSetParentToCurrent | ReturnParent,
.....
};
while(currentRenderer) {
current = makeCounterNode(...);
action = decisionVector [ DecisionIsReset * isReset + DecisionIsCurrentReset * current->actsAsReset() + ... ];
if (action & ActionSetParentToCurrent)
parent=current;
if (action & ActionSetPreviousSiblingToCurrent)
previousSibling=current;
if (action & ActionReturnParent)
return parent;
.....
if (previousSibling)
currentRenderer = previousSiblingOrParent(currentRenderer);
else
currentRenderer = currentRenderer->previousInPreOrder();
}
return false;
style-queue ran check-webkit-style on attachment 44101 [details] without any errors.
Comment on attachment 44101 [details] Proposed Patch Rejecting patch 44101 from commit-queue. Failed to run "['git', 'svn', 'rebase']" exit_code: 1 Last 500 characters of output: geLog r51773 = e09427d72cfc7ad43d286711fd170387100dad22 (refs/remotes/trunk) M LayoutTests/platform/win/Skipped M LayoutTests/ChangeLog r51775 = c68367ef88c3fdc4a45069529814374838fcba35 (refs/remotes/trunk) First, rewinding head to replay your work on top of it... error: Untracked working tree file 'LayoutTests/http/tests/security/cross-frame-access-getOwnPropertyDescriptor-expected.txt' would be overwritten by merge. could not detach HEAD rebase refs/remotes/trunk: command returned error: 1 Sorry, I"ll fix that on the bot. That's caused by bug 28603. Comment on attachment 44101 [details]
Proposed Patch
Rejecting patch 44101 from commit-queue.
Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
g.html
M WebCore/ChangeLog
M WebCore/rendering/RenderCounter.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
The following files contain tab characters:
trunk/LayoutTests/fast/css/counters/nesting.html
Please use spaces instead to indent.
If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
at /usr/local/git/libexec/git-core/git-svn line 558
Created attachment 44464 [details]
Proposed Patch
Fixed tab in html test case, otherwise the patch is identical with the previously reviewed one.
style-queue ran check-webkit-style on attachment 44464 [details] without any errors.
Comment on attachment 44464 [details] Proposed Patch Clearing flags on attachment: 44464 Committed r51851: <http://trac.webkit.org/changeset/51851> All reviewed patches have been landed. Closing bug. |