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

Thu Jan 4 12:25:16 PST 2007

Rob Buis <rwlbuis at gmail.com> has denied Rob Buis <rwlbuis at gmail.com>'s request
for review:
Bug 9454: CSS2: Language not inherited from parent element
http://bugs.webkit.org/show_bug.cgi?id=9454

Attachment 12133: Patch
http://bugs.webkit.org/attachment.cgi?id=12133&action=edit

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
>+<html >

No need for the extra space.

>+  p:lang(x) { background:lime }

Please use green here, it is used throughout the layout tests.
The testcase seems fine, be sure to include expected results(.txt,.checksum and
.png). You can get them by running run-webkit-tests and run-webkit-tests -p.

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

You probably didnt mean to write "the" there.

>+		  Node *n = e->parent();

* needs to right next to Node (see style guidelines).

>+		      /*
>+		      else
>+		      if (n->isDocumentNode()) {
>+			  value = static_cast<Document*>(n)->contentLanguage();

>+		      */

AFAIK code in comments should not be included in patches. Also the code is not
100% according to style guidelines.

>+2006-12-31  Allan Sandfeld Jensen,,,	<set EMAIL_ADDRESS environment
variable>

Make sure this line is correct, you need to give your mail, and I see no need
for the ,,,

So coding wise this patch seems fine (except some style problems mentioned). As
soon as you fix the points above I am fine with r+ing it.