Hi Rupinder, First of all, I'd like to say thank you very much for actually doing this work. It's an excellent start. We need to fix up a few things, and so that you get a taste for how I tend to treat code review, I'm treating you as though you were a seasoned developer. As such, I'm being a little more strict than I might otherwise. I hope this is an instructive and valuable experience for you...
On Sat, Mar 01, 2014 at 15:27:21 +0530, Rupinder Singh wrote: > From 51ceb361a2e79bfa290d8e2cff72555ca7dd39f4 Mon Sep 17 00:00:00 2001 > From: rsk1994 <[email protected]> > Date: Sat, 1 Mar 2014 20:44:04 +0530 > Subject: [PATCH] dom tree support for hr tag and its attributes This looks like you used git format-patch and then attached the output rather than using its output as an email to send. If you can't get the output of format-patch into your mailer (I see you use gmail) then you can try the git send-email tool which can do it for you. I'm going to assume the XML files are just renames and will remove them from the review. If I do not quote something, it's acceptable. If I have a general comment, I will not necessarily indicate every case where it happens. You should go through this review, update your patch and try again. > diff --git a/include/dom/dom.h b/include/dom/dom.h > index 2ef6e9b..35b54e3 100644 > --- a/include/dom/dom.h > +++ b/include/dom/dom.h > @@ -54,6 +54,7 @@ > #include <dom/html/html_opt_group_element.h> > #include <dom/html/html_option_element.h> > #include <dom/html/html_select_element.h> > +#include <dom/html/html_hr_element.h> > #include <dom/html/html_options_collection.h> Why did you insert hr_element here rather than in alphabetical order or at the end? > diff --git a/include/dom/html/html_hr_element.h > b/include/dom/html/html_hr_element.h > index 2e182d5..fb1006b 100644 > --- a/include/dom/html/html_hr_element.h > +++ b/include/dom/html/html_hr_element.h > @@ -2,6 +2,40 @@ > * This file is part of libdom. > * Licensed under the MIT License, > * http://www.opensource.org/licenses/mit-license.php > - * Copyright 2009 Bo Yang <[email protected]> > + * Copyright 2012 Daniel Silverstone <[email protected]> > */ This copyright change is very odd, I'd have expected you to simply *ADD* your copyright, not remove Bo's and add a statement that it's copyrighted to me. > +#include <dom/html/html_form_element.h> This include is superfluous. > +dom_exception dom_html_hr_element_get_no_shade( > + dom_html_hr_element *hr, bool *noShade); Please do bool *no_shade. Our coding style does not include camel case. > diff --git a/src/html/html_document_strings.h > b/src/html/html_document_strings.h > index f2d6fa2..b4dc5d7 100644 > --- a/src/html/html_document_strings.h > +++ b/src/html/html_document_strings.h > @@ -96,6 +96,7 @@ HTML_DOCUMENT_STRINGS_ACTION1(selected) > HTML_DOCUMENT_STRINGS_ACTION(select_multiple,select-multiple) > HTML_DOCUMENT_STRINGS_ACTION(select_one,select-one) > /* Some event strings for later */ > +HTML_DOCUMENT_STRINGS_ACTION1(width) width isn't an event string, please find the correct place in this header to include the width string. > diff --git a/src/html/html_hr_element.c b/src/html/html_hr_element.c > index 2e182d5..9eb911b 100644 > --- a/src/html/html_hr_element.c > +++ b/src/html/html_hr_element.c > @@ -2,6 +2,186 @@ > * This file is part of libdom. > * Licensed under the MIT License, > * http://www.opensource.org/licenses/mit-license.php > - * Copyright 2009 Bo Yang <[email protected]> > + * Copyright 2012 Daniel Silverstone <[email protected]> > */ Again, very odd copyright change here. > +dom_exception dom_html_hr_element_get_no_shade(dom_html_hr_element *ele, > + bool *noShade) > +{ > + return dom_html_element_get_bool_property(&ele->base, "noShade", > + > SLEN("noShade"),noShade); You're missing a space before the badly named 'noShade'. In addition, there's no point in capitalising the 'S' in the property name. > +/* The virtual copy function, see src/core/node.c for detail */ > +dom_exception _dom_html_hr_element_copy(dom_node_internal *old, > + dom_node_internal **copy) > +{ > + return _dom_html_element_copy(old, copy); > +} You don't seem to copy the attributes of the HR element. > diff --git a/src/html/html_hr_element.h b/src/html/html_hr_element.h > index 2e182d5..3eca538 100644 > --- a/src/html/html_hr_element.h > +++ b/src/html/html_hr_element.h > @@ -2,6 +2,59 @@ > * This file is part of libdom. > * Licensed under the MIT License, > * http://www.opensource.org/licenses/mit-license.php > - * Copyright 2009 Bo Yang <[email protected]> > + * Copyright 2012 Daniel Silverstone <[email protected]> > */ Again odd copyright. > +struct dom_html_hr_element { > + struct dom_html_element base; > + /**< The base class */ > + dom_string *align; > + dom_string *size; > + dom_string *width; > + bool noShade; Please use no_shade, not noShade. Currently I'm voting -1 on this patch series, and I've not even checked the whitespace sanity yet. Please make the above changes, and any others which you feel are necessary to make the change clean. I look forward to hearing from you with an updated patch. Regards, Daniel. -- Daniel Silverstone http://www.netsurf-browser.org/ PGP mail accepted and encouraged. Key Id: 3CCE BABE 206C 3B69
