[webkit-reviews] review granted: [Bug 44622] implicitlyStyledElementShouldBeRemovedWhenApplyingStyle, removeHTMLFontStyle, and removeHTMLBidiEmbeddingStyle should be merged : [Attachment 65492] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 44622: implicitlyStyledElementShouldBeRemovedWhenApplyingStyle,
removeHTMLFontStyle, and removeHTMLBidiEmbeddingStyle should be merged
https://bugs.webkit.org/show_bug.cgi?id=44622

Attachment 65492: Patch
https://bugs.webkit.org/attachment.cgi?id=65492&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Seems OK to merge all of this into a single function.

> +enum EPushDownType {ShouldBePushedDown, ShouldNotBePushedDown };

Missing space after brace here.

> +struct CSSPropertyElementCorrespondance {

The word "correspondance" is spelled "correspondence".

I'm not sure this is an "element correspondence". I think it's information
about achieving the same effect as the property with HTML elements and
attributes. Maybe given the context of the source file, HTMLEquivalent would be
a suitable name for the structure and then HTMLEquivalents could be the name of
the array?

> +    {CSSPropertyFontWeight, false, CSSValueBold, &bTag, 0,
ShouldBePushedDown},

You should have space before and after these braces.

> +    for (size_t i = 0; i < sizeof(CSSPropertyElementMapping) /
sizeof(CSSPropertyElementCorrespondance); i++) {

This code would be easier to read if you stuck CSSPropertyElementMapping[i]
into a local variable. I would name the local variable "equivalents".

> +	   ASSERT(CSSPropertyElementMapping[i].element ||
CSSPropertyElementMapping[i].attribute);
> +	   if (CSSPropertyElementMapping[i].element &&
!element->hasTagName(*CSSPropertyElementMapping[i].element))
> +	       continue;
> +	   if (CSSPropertyElementMapping[i].attribute &&
!element->hasAttribute(*CSSPropertyElementMapping[i].attribute))
> +	       continue;

> +	   else if (styleValue->cssText() == mapValue->cssText())
> +	       continue; // If CSS value is primitive, then skip if they are
equal.

This seems like an inefficient way to compare. Later we should look into this.
Serializing just to check if equal seems wasteful.