[Webkit-unassigned] [Bug 9454] CSS2: Language not inherited from parent element

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=9454


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #15932|review?                     |review+
               Flag|                            |




------- Comment #15 from darin at apple.com  2007-08-11 15:33 PDT -------
(From update of attachment 15932)
This looks really good.

+        if (isHTMLDocument())

Why restrict this to HTML documents? In the past we've found we need these
document-level functions for any documents with HTML content, even ones that
aren't HTML documents. Does Gecko have this function for non-HTML documents?

+            this->setContentLanguage(content);

Why an explicit "this->" here?

+    String m_contentLanguage;

New fields ought to be private, rather than protected, unless there's some need
a derived class needs access. We need to scale back our use of protected to the
minimum.

+                // but the it has not been clarified how lang and xml:lang
coexists

Need to take out the word "the" here.

+                Node* n = e->parent();
+                while (n && value.isEmpty()) {
                      [...]
+
+                    n = n->parent();
+                }

I think this would read even better as a for loop rather than a while loop. I
also think we have too much code here in-line. I'd like to see it put in a
helper function. There are some features we're planning in the future that will
require figuring out what the language is for a Node, so it would be nice to
have this loop and logic somewhere in the DOM outside the style system. Maybe
even:

    AtomicString Node::language()

I think I'm going to say r=me despite these quibbles. But please consider
revising the patch.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.