[webkit-reviews] review granted: [Bug 9454] CSS2: Language not inherited from parent element : [Attachment 15932] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Aug 11 15:33:50 PDT 2007

Darin Adler <darin at apple.com> has granted Allan Sandfeld <kde at carewolf.com>'s
request for review:
Bug 9454: CSS2: Language not inherited from parent element

Attachment 15932: Patch

------- Additional Comments from Darin Adler <darin at apple.com>
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

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

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

    AtomicString Node::language()

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

More information about the webkit-reviews mailing list