[webkit-reviews] review denied: [Bug 9454] CSS2: Language not
inherited from parent element : [Attachment 12133] Patch
bugzilla-request-daemon at macosforge.org
bugzilla-request-daemon at macosforge.org
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.
More information about the webkit-reviews
mailing list