[webkit-reviews] review denied: [Bug 21312] text-transform: lowercase is not lang-dependent (Turkish languages : tr, az) : [Attachment 28363] tentative 1st step toward a fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jungshik Shin <jshin at chromium.org>'s
request for review:
Bug 21312: text-transform: lowercase is not lang-dependent (Turkish languages :
tr,az)
https://bugs.webkit.org/show_bug.cgi?id=21312

Attachment 28363: tentative 1st step toward a fix
https://bugs.webkit.org/attachment.cgi?id=28363&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
There are 3 things done here:

    1) Move the language-determining code for the style system from
CSSStyleSelector into a member function in the Node class.
    2) Adding a cache in every Node object to store that language code.
    3) Changes to the logic of the language-determining code.

I can't easily comment on much of (3), because the patch doesn't show the
change from the old to the new due to the use of #if 0.

There are some issues with (1):

    A) The style system only needs the language for elements, not other nodes,
I would have the code in Element, not Node. But we won't want to dedicate 4
bytes in every Element to caching the language for that element. See below. In
general, we need to keep the number of function members in these basic classes
to a minimum, so it's unfortunate that the code is moving here.

    B) Breaking the language-computation out into a separate function is fine.
That could be done as a first step of refactoring inside the CSSStyleSelector
class. I don't think the function needs to be a member of the Node classes.

The biggest problems are with (2):

    C) There's no way we can make all nodes 4 bytes bigger just to make it
faster to retrieve the value of the lang attribute, so that aspect of this
patch is unacceptable. There are techniques we could use, such as putting the
cached language into the rare data or using an external hash table that could
keep the cost down for typical elements.

    D) There's no code to invalidate the cached language when attributes change
or when the DOM tree structure changes. For example, if you store the value of
an attribute such as langAttr, you then need code to deal with the case where
that attribute changes. This becomes even more complex when you cache the value
of the lang attribute of a parent. Then that parent's attribute changes and
somehow the cached value needs to be invalidated for all the descendants of
that parent. The cost of a cache is the invalidation cost, and since there's no
invalidation here it's unclear we could afford it.

One issue with (3):

    E) It's generally much better to use iteration to walk through the parent
list than to use recursion; we don't want an algorithm that uses machine stack
proportional to the depth of the DOM tree if we can avoid it.