Re: patch for adding hr tag to libdom

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 <rsk1coder99@gmail.com>
> 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 <struggleyb.nku@gmail.com>
> + * Copyright 2012 Daniel Silverstone <dsilvers@netsurf-browser.org>
> */

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 <struggleyb.nku@gmail.com>
> + * Copyright 2012 Daniel Silverstone <dsilvers@netsurf-browser.org>
> */

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 <struggleyb.nku@gmail.com>
> + * Copyright 2012 Daniel Silverstone <dsilvers@netsurf-browser.org>
> */

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