236479 - Kill nsMappedAttributes::GetAttribute

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

All users of this should instead use nsMappedAttributes::GetAttr since it's both
faster and doesn't use nsHTMLValue.

Comment 1

[Wladimir Palant]

Attachment: First try

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If you're done with this patch please use the reviewtracker to ask for reveiws.
From a quick overview it looks really nice.

Comment 3

[Wladimir Palant]

I am done with the patch until you review it :)
After looking over the patch once more I found a declaration of a variable lang
in nsGenericHTMLElement.cpp, that I forgot to remove. The rest should be ok, I
have tested everything. You might want to have a closer look at
MapTableBorderInto() in nsHTMLTableElement.cpp though, the original
implementation was overcomplicated so I simplified it.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 144347 [details] [diff] [review]
First try

This is awesome. Lovly to see how the new code is both smaller and more
efficient.

Please make sure that nsHTMLSharedElement.cpp and nsHTMLSharedLeafElement.cpp
look exactly the same after this patch (this is caused by a premature fork on
my part).

>Index: base/src/nsAttrValue.h
>+inline PRBool
>+nsAttrValue::IsEmptyString() const
>+{
>+  return BaseType() == eStringBase && !mBits;
> }

You don't need the |BaseType()| test. The |!mBits| test will cover that too,
and the |!mBits| test would fail if eStringBase wasn't 0.

If you want to be absolutly sure you could assert that |!mBits == (BaseType()
== eStringBase && !GetPtr())|.

>Index: html/content/src/nsGenericHTMLElement.cpp
> nsGenericHTMLElement::MapCommonAttributesInto(const nsMappedAttributes* aAttributes,
...
>-    nsHTMLValue value;
>-    if (aAttributes->GetAttribute(nsHTMLAtoms::lang, value) !=
>-        NS_CONTENT_ATTR_NOT_THERE &&
>-        value.GetUnit() == eHTMLUnit_String) {
>+    const nsAttrValue* value = aAttributes->GetAttr(nsHTMLAtoms::lang);
>+    if (value && value->Type() == nsAttrValue::eString) {
>       nsAutoString lang;
>-      value.GetStringValue(lang);
>-      aData->mDisplayData->mLang.SetStringValue(lang,
>+      aData->mDisplayData->mLang.SetStringValue(value->GetStringValue(),

|lang| is no longer used so just remove it.

> nsGenericHTMLElement::MapImageAlignAttributeInto(const nsMappedAttributes* aAttributes,
>                                                  nsRuleData* aRuleData)
> {
>   if (aRuleData->mSID == eStyleStruct_Display || aRuleData->mSID == eStyleStruct_TextReset) {
>-    nsHTMLValue value;
>-    aAttributes->GetAttribute(nsHTMLAtoms::align, value);
>-    if (value.GetUnit() == eHTMLUnit_Enumerated) {
>-      PRUint8 align = (PRUint8)(value.GetIntValue());
>+    const nsAttrValue* value = aAttributes->GetAttr(nsHTMLAtoms::align);
>+    if (value && value->Type() == nsAttrValue::eEnum) {
>+      PRUint8 align = (PRUint8)(value->GetEnumValue());

Please make |align| be an PRInt32 while you're here. That way you don't need an
ugly cast. Same in nsHTMLShared(Leaf)Element.

> nsGenericHTMLElement::MapImageSizeAttributesInto(const nsMappedAttributes* aAttributes,
...
>+    const nsAttrValue* value = aAttributes->GetAttr(nsHTMLAtoms::width);
>+    if (value && value->Type() == nsAttrValue::eInteger)
>+      aData->mPositionData->mWidth.SetFloatValue((float)value->GetIntegerValue(), eCSSUnit_Pixel);
>+    else if (value && value->Type() == nsAttrValue::ePercent)
>+      aData->mPositionData->mWidth.SetPercentValue(value->GetPercentValue());

This pattern seems quite common and is currently somewhat wastefully
implemented. I.e. to check if a value exists and call
SetFloatValue/SetPercentValue depending on if the value is an eInteger or an
ePercent. A function like:

void SetToIntOrPercent(nsAttrValue* aAttrValue, nsCSSValue& aCSSValue)
{
  if (aAttrValue) {
    nsAttrValue::ValueType type = aAttrValue->Type();
    if (type == nsAttrValue::eInteger) {
      aCSSValue.SetFloatValue(aAttrValue->GetIntegerValue(), eCSSUnit_Pixel);
    }
    else if (type == nsAttrValue::ePercent) {
      aCSSValue.SetPercentValue(aAttrValue->GetPercentValue());
    }
  }
}

could be quite helpfull. Probably living in nsGenericHTMLElement (for now). But
that should probably be done as a separate patch so that we don't have to redo
this entire one. I'll file a bug.

>Index: html/content/src/nsHTMLFontElement.cpp
...
>     // pointSize: int, enum
>     if (font.mSize.GetUnit() == eCSSUnit_Null) {
>-      aAttributes->GetAttribute(nsHTMLAtoms::pointSize, value);
>-      if (value.GetUnit() == eHTMLUnit_Integer ||
>-          value.GetUnit() == eHTMLUnit_Enumerated) {
>-        PRInt32 val = value.GetIntValue();
>-        font.mSize.SetFloatValue((float)val, eCSSUnit_Point);
>-      }
>+      const nsAttrValue* value = aAttributes->GetAttr(nsHTMLAtoms::pointSize);
>+      if (value && value->Type() == nsAttrValue::eInteger)
>+        font.mSize.SetFloatValue((float)value->GetIntegerValue(), eCSSUnit_Point);
>+      else if (value && value->Type() == nsAttrValue::eEnum)
>+        font.mSize.SetFloatValue((float)value->GetEnumValue(), eCSSUnit_Point);

Nowadays the pointSize can never be an enum so just remove that |else if|
section.
Same goes for fontWeight.

Sorry, I forgot to fix this when I changed the way pointSize and fontWeight are
parsed.

>Index: html/content/src/nsHTMLTableElement.cpp
>-      if (value.GetUnit() == eHTMLUnit_Enumerated) {
>-        if ((NS_STYLE_TEXT_ALIGN_CENTER == value.GetIntValue()) ||
>-            (NS_STYLE_TEXT_ALIGN_MOZ_CENTER == value.GetIntValue())) {
>+      if (value && value->Type() == nsAttrValue::eEnum) {
>+        if ((NS_STYLE_TEXT_ALIGN_CENTER == value->GetEnumValue()) ||
>+            (NS_STYLE_TEXT_ALIGN_MOZ_CENTER == value->GetEnumValue())) {

Please reverse the order of the expressions here (value->GetEnumValue() ==
NS_STYLE...) and remove the extra parenthesis

>     if (readDisplay->mDisplay == NS_STYLE_DISPLAY_TABLE_CELL) {
>-      nsHTMLValue value;
>-      aAttributes->GetAttribute(nsHTMLAtoms::cellpadding, value);
>-      if (value.GetUnit() == eHTMLUnit_Integer || value.GetUnit() == eHTMLUnit_Percent) {
>+      const nsAttrValue* value = aAttributes->GetAttr(nsHTMLAtoms::cellpadding);
>+      if (value && (value->Type() == nsAttrValue::eInteger || value->Type() == nsAttrValue::ePercent)) {
>         // We have cellpadding.  This will override our padding values if we don't
>         // have any set.
>         nsCSSValue padVal;
>-        if (value.GetUnit() == eHTMLUnit_Integer)
>-          padVal.SetFloatValue((float)value.GetIntValue(), eCSSUnit_Pixel);
>+        if (value->Type() == nsAttrValue::eInteger)
>+          padVal.SetFloatValue((float)value->GetIntegerValue(), eCSSUnit_Pixel);

There's a lot of calls to Type() here. Please just call it once and save the
result. Type() is a more expensive call then GetUnit() was.

>-        nsHTMLValue value;
>-        if (aAttributes->GetAttribute(nsHTMLAtoms::border, value) !=
>-            NS_CONTENT_ATTR_NOT_THERE &&
>-            ((value.GetUnit() == eHTMLUnit_Integer &&
>-              value.GetIntValue() > 0) ||
>-             value.IsEmptyString())) {
>+        const nsAttrValue* value = aAttributes->GetAttr(nsHTMLAtoms::border);
>+        if (value && ((value->Type() == nsAttrValue::eInteger &&
>+            value->GetIntegerValue() > 0) || value->IsEmptyString())) {

Please line up the expressions here like the old code so that it's easier to
see what is grouped together by the parenthesis.

With the above, r=me

Comment 5

[Wladimir Palant]

Attachment: Patch with sicking's corrections

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 144674 [details] [diff] [review]
Patch with sicking's corrections

you left a debug-printf in there.

r=me with that fixed.

No need for a new patch though.

Comment 7

[Wladimir Palant]

Attachment: Patch with sicking's corrections v2

Sorry, forgot to remove some debugging code.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 144675 [details] [diff] [review]
Patch with sicking's corrections v2

sr=jst

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in!

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

trev, i'm leaving the markup as fixed to you since you own this bug. Btw, thanks
for the great patch!