[webkit-reviews] review denied: [Bug 30784] WebKit cannot remove nested bold tags : [Attachment 41927] fixes the bug by making getPropertiesNotInComputedStyle more flexible (fixed expected result)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 30784: WebKit cannot remove nested bold tags
https://bugs.webkit.org/show_bug.cgi?id=30784

Attachment 41927: fixes the bug by making getPropertiesNotInComputedStyle more
flexible (fixed expected result)
https://bugs.webkit.org/attachment.cgi?id=41927&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +int getFontWeightValue(CSSStyleDeclaration* style) {

Naming here does not match the WebKit coding style. We use nouns to name
functions with return values, not "get". I also think the word "value" is not
so good here in the name. There is already a font weight value, and it's a
CSSValue object. The name of the function needs to describe what it does. It
seems to me that this function actually just returns a boolean stating whether
or not the font weight is a "bold" weight. So it would probably be better to
make a function named fontWeightIsBold that returns a boolean. Or perhaps you
have plans for the future that involve multiple font weights and therefore want
to use the font weight numbers.

Formatting here does not match the WebKit coding style. The brace goes on the
beginning of the next line.

I think the code needs a comment explaining exactly why "there are only two
font-weights for rich text editing purposes". A concrete explanation of this.

> +	   case CSSValue100    :
> +	   case CSSValue200    :
> +	   case CSSValue300    :
> +	   case CSSValue400    :
> +	   case CSSValue500    :
> +	   case CSSValueNormal :
> +	       return 500;
> +	   case CSSValueBold   :
> +	   case CSSValue600    :
> +	   case CSSValue700    :
> +	   case CSSValue800    :
> +	   case CSSValue900    :
> +	       return 700;

Formatting here does not match the WebKit coding style. We don't line up the
colons like this.

Patch otherwise looks pretty good to me.

review- because of the issues mentioned above