Bug 44622 – implicitlyStyledElementShouldBeRemovedWhenApplyingStyle, removeHTMLFontStyle, and removeHTMLBidiEmbeddingStyle should be merged

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

implicitlyStyledElementShouldBeRemovedWhenApplyingStyle, removeHTMLFontStyle, and removeHTMLBidiEmbeddingStyle all share the same logic and should be merged int one function.  Also it's required to fix the bug 44560.

[Ryosuke Niwa]

Created attachment 65492 [details]
Patch

[Darin Adler]

Comment on attachment 65492 [details]
Patch

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.

[Ryosuke Niwa]

Thanks for the review.

(In reply to comment #2)
> (From update of attachment 65492 [details])
> Seems OK to merge all of this into a single function.
> 
> > +enum EPushDownType {ShouldBePushedDown, ShouldNotBePushedDown };
> 
> Missing space after brace here.

Will fix.

> > +struct CSSPropertyElementCorrespondance {
> 
> The word "correspondance" is spelled "correspondence".

Sorry about the typo.  I badly need a spell checker on XCode.

> 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?

I had an intention in using it in StyleChange as well but I guess we can rename it when I do that.  Will rename to HTMLEquivalents and HTMLEquivalent as you suggested.

> > +    {CSSPropertyFontWeight, false, CSSValueBold, &bTag, 0, ShouldBePushedDown},
> 
> You should have space before and after these braces.

Will fix.

> > +    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".

Will fix.

> > +        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.

Yes, it's a unnecessary performance degression but because CSSValue doesn't have operator== or matches, we can do it any other way.  I'm more than happy to add operator== or matches.  It'll make a lot of things easier.

[Ryosuke Niwa]

Landed as http://trac.webkit.org/changeset/66186.