[webkit-reviews] review requested: [Bug 11031] Another crazy counters bug : [Attachment 43487] Proposed Patch

[bugzilla-daemon at webkit.org]

Carol Szabo <carol.szabo at nokia.com> has asked  for review:
Bug 11031: Another crazy counters bug
https://bugs.webkit.org/show_bug.cgi?id=11031

Attachment 43487: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=43487&action=review

------- Additional Comments from Carol Szabo <carol.szabo at nokia.com>
This patch addresses Darin's concerns regarding my previous submission.
The patch was broken up in 3 installments of which this is the last.
-Changed Comment styles
-Changed Code layout and fixed coding style errors (all that I could find).
-Explained in detail the findPlaceForCounter algorithm as well as its design.
The code is somewhat lengthier than the previous implementation, but it handles
a lot more complex cases and it is faster as it shortcuts a good portion of the
render tree once it found a predecessor to the counter. If anyone can find a
meaningful reduction please let me know.

Continuing concerns:
- I do not understand why the Renderer to CounterNodes map uses
RefPtr<AtomicStringImpl> as the key, instead of AtomicString. Since Darin
wanted me to use AtomicString in most code for counter identifiers, there is a
lot of conversion going on to and from these map lookups which need
RefPtr<AtomicStringImpl>.

Regarding the alternative solution that Shinchiro Hamaji proposed, after a
brief analysis I found that it fails to address a few concerns that my solution
addresses:
-  When a new counter reference counter is created which comes in preorder
before any other reference to a counter with the same id, it may be that some
counter node trees must be merged, and previous nodes that were reset nodes
become increment nodes, because they were forced to reset by the virtue of not
having a reset node come before them.
- Sometimes a subtree of renderers is attached to the main tree as a whole and
the root of the attached tree does not have a counter but the branches have
such a counter. These counters may not be correctly updated by Shichiro's
patch, as he does not take any action if the root node does not have a counter
with it.
- Shinchiro does not address the problems with the findPlaceForCounter
algorithm.
Also Shinchiro's algorithm raises some performance issues:
- Shinchiro's algorithm invalidates counters with all ids when the style of a
node changes even if some of the counter directives stay the same.
- When a node with a counter is inserted all renders that follow that renderer
in the render tree are searched for counters and if the counters are found,
they are invalidated. This appears to be much slower than my approach of
searching the counters with the same id and find out whether they are affected
and doing this only for root counters.