1323138 - CSSEditUtil should use atom for css property if possible

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Makoto Kato [:m_kato]]

There is a lot of string compare and etc.  We should use nsGkAtom instead.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1323138 - CSSEditUtils should use atom for CSS property if possible.

There is a lot of string compare when using CSS property name.  We should use nsGkAtoms instead.

Review commit: https://reviewboard.mozilla.org/r/98310/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98310/

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8818184 [details]
Bug 1323138 - CSSEditUtils should use atom for CSS property if possible.

https://reviewboard.mozilla.org/r/98310/#review98534

::: editor/libeditor/CSSEditUtils.cpp:342
(Diff revision 1)
>    if (nsGkAtoms::b == aProperty ||
>        nsGkAtoms::i == aProperty ||
>        nsGkAtoms::tt == aProperty ||
>        nsGkAtoms::u == aProperty ||
>        nsGkAtoms::strike == aProperty ||
>        (nsGkAtoms::font == aProperty && aAttribute &&

(Just random thoughts, though, like a method IsAnyOf(const nsIAtom* aAtom...) might be useful?)

::: editor/libeditor/CSSEditUtils.cpp:390
(Diff revision 1)
>      return true;
>    }
>  
>    // attributes TEXT, BACKGROUND and BGCOLOR on BODY
> -  if (aAttribute && node->IsHTMLElement(nsGkAtoms::body) &&
> -      (aAttribute->EqualsLiteral("text")
> +  if (node->IsHTMLElement(nsGkAtoms::body) &&
> +      (aAttribute == nsGkAtoms::text || aAttribute == nsGkAtoms::background ||

(Then, this could be more readable, but there must be a lot of similar cases, so, not scope of this bug.)

::: editor/libeditor/CSSEditUtils.cpp:946
(Diff revision 1)
> -  *aCount = cssPropertyArray.Length();
> -  for (int32_t index = 0; index < *aCount; index++) {
> +  int32_t count = cssPropertyArray.Length();
> +  for (int32_t index = 0; index < count; index++) {

Perhaps, you should use size_t for count and index here. (Although, not your fault.)

::: editor/libeditor/CSSEditUtils.cpp:979
(Diff revision 1)
>                                               nsIAtom* aHTMLProperty,
> -                                             const nsAString* aAttribute,
> +                                             nsIAtom* aAttribute,
>                                               const nsAString* aValue,
>                                               bool aSuppressTransaction)
>  {
> -  MOZ_ASSERT(aElement);
> +  if (!aElement) {

I wonder, shouldn't you use NS_WARN_IF() here?

::: editor/libeditor/CSSEditUtils.cpp:1160
(Diff revision 1)
>      } else if (nsGkAtoms::strike == aHTMLProperty) {
>        nsAutoString val;
>        val.AssignLiteral("line-through");
> -      aIsSet = ChangeStyleTransaction::ValueIncludes(valueString, val);
> -    } else if (aHTMLAttribute &&
> -               ((nsGkAtoms::font == aHTMLProperty &&
> +      isSet = ChangeStyleTransaction::ValueIncludes(valueString, val);
> +    } else if ((nsGkAtoms::font == aHTMLProperty &&
> +                 aHTMLAttribute == nsGkAtoms::color) ||

Looks like odd indent here.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd40189cf066
CSSEditUtils should use atom for CSS property if possible. r=masayuki

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fd40189cf066