Bug 31723

Summary: CSS Counter Nesting still does not work according to the spec.
Product: WebKit Reporter: Carol Szabo <carol>
Component: CSSAssignee: 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 Flags
Test Case
none
Proposed Patch
none
Proposed Patch
darin: review+
Proposed Patch
darin: review+, commit-queue: commit-queue-
Proposed Patch none

Description Carol Szabo 2009-11-20 08:42:46 PST
The following attached test case does not render correctly.
Patch to come soon. This is patch is a partial for the big fix for 11031.
Comment 1 Carol Szabo 2009-11-20 08:43:52 PST
Created attachment 43583 [details]
Test Case

I tried to attach this with the bug creation, but the attachment was dropped.
Comment 2 Carol Szabo 2009-11-20 09:20:45 PST
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.
Comment 3 Carol Szabo 2009-11-23 16:10:12 PST
Created attachment 43740 [details]
Proposed Patch

This is the first part of a comprehensive patch to 11031
Comment 4 Darin Adler 2009-11-30 10:47:16 PST
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.
Comment 5 Carol Szabo 2009-12-01 14:11:38 PST
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;
Comment 6 WebKit Review Bot 2009-12-01 14:15:45 PST
style-queue ran check-webkit-style on attachment 44101 [details] without any errors.
Comment 7 WebKit Commit Bot 2009-12-07 10:21:28 PST
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
Comment 8 Eric Seidel (no email) 2009-12-07 10:30:49 PST
Sorry, I"ll fix that on the bot.  That's caused by bug 28603.
Comment 9 WebKit Commit Bot 2009-12-07 11:55:02 PST
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
Comment 10 Carol Szabo 2009-12-08 02:21:11 PST
Created attachment 44464 [details]
Proposed Patch

Fixed tab in html test case, otherwise the patch is identical with the previously reviewed one.
Comment 11 WebKit Review Bot 2009-12-08 02:22:50 PST
style-queue ran check-webkit-style on attachment 44464 [details] without any errors.
Comment 12 WebKit Commit Bot 2009-12-08 07:21:05 PST
Comment on attachment 44464 [details]
Proposed Patch

Clearing flags on attachment: 44464

Committed r51851: <http://trac.webkit.org/changeset/51851>
Comment 13 WebKit Commit Bot 2009-12-08 07:21:13 PST
All reviewed patches have been landed.  Closing bug.