230976 - computed style should make use of atoms for rocss primitive value identifiers

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Throughout nsComputedDOMStyle.cpp, there are a ton of lines similar to:

nsComputedDOMStyle.cpp:345:    val->SetIdent(NS_LITERAL_STRING("none"));
nsComputedDOMStyle.cpp:367:    val->SetIdent(NS_LITERAL_STRING("none"));
nsComputedDOMStyle.cpp:389:    val->SetIdent(NS_LITERAL_STRING("none"));
nsComputedDOMStyle.cpp:544:    val->SetIdent(NS_LITERAL_STRING("none"));
nsComputedDOMStyle.cpp:566:    val->SetIdent(NS_LITERAL_STRING("normal"));
nsComputedDOMStyle.cpp:592:    val->SetIdent(NS_LITERAL_STRING("normal"));
nsComputedDOMStyle.cpp:614:    val->SetIdent(NS_LITERAL_STRING("normal"));

We should start using atoms for those to avoid the string duplication.  I have a
patch to do so which I will attach shortly, adding an nsIAtom* type to the union
in nsROCSSPrimitiveValue.  It looks like we'll save about 5k with this patch
(before and after numbers follow).

> size layout/build/libgklayout.so
   text    data     bss     dec     hex filename
3622534  221856   23392 3867782  3b0486 layout/build/libgklayout.so

> size layout/build/libgklayout.so
   text    data     bss     dec     hex filename
3617601  221900   23424 3862925  3af18d layout/build/libgklayout.so

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Actually, I want to land my fix for bug 230973 before attaching this, so I can
get rid of the "disc" string and not need to worry about it becoming an atom.

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v1

Also note that I got rid of the else clause for the cases where:

  const nsStyleFoo* myStruct = nsnull;
  GetStyleData(eStyleStruct_Foo, (const nsStyleStruct*&)myStruct, aFrame);
  if (myStruct) {
    ...
  } else {
    ...
  }

since we should always be able to get a style struct, and those were redundant
with additional strings.  I'll probably remove the checks in a followup patch.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 139151 [details] [diff] [review]
Patch v1

>Index: content/html/style/src/nsComputedDOMStyle.cpp
>@@ -1272,26 +1242,22 @@ nsComputedDOMStyle::GetZIndex(nsIFrame *
>       default:
>         NS_WARNING("Double Check the Unit!");
>-        val->SetIdent(NS_LITERAL_STRING("auto"));
>+      case eStyleUnit_Auto:

Add a comment that you are falling through on purpose.

>Index: content/html/style/src/nsROCSSPrimitiveValue.cpp
>     case CSS_IDENT :
>+      {
>+        nsAutoString atomValue;
>+        mValue.mAtom->ToString(atomValue);
>+        tmpStr.Append(atomValue);
>+        break;
>+      }

How about using getUTF8String and then AppendUTF8toUTF16 here?	Saves the need
for that random autostring...

>Index: content/html/style/src/nsROCSSPrimitiveValue.h

>-  void SetIdent(const nsAString& aString)
>+  void SetIdent(const nsACString& aString)

Are we using this?  If not, I'd rather not have it.

>Index: content/shared/public/nsLayoutAtomList.h

OK this change with dbaron?

r+sr=bzbarsky with that stuff fixed.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> OK this change with dbaron?

It's fine with me.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

>>-  void SetIdent(const nsAString& aString)
>>+  void SetIdent(const nsACString& aString)
>
>Are we using this?  If not, I'd rather not have it.

Yeah, we're still using it for the stuff we lookup from nsCSSProps, e.g.

     const nsAFlatCString& clear =
       nsCSSProps::SearchKeywordTable(display->mBreakType,
                                      nsCSSProps::kClearKTable);
     val->SetIdent(clear);

Other changes were made and checked in.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Zdiff numbers from tinderboxes:

luna:    -5860
brad:    -3536
beast:   -1854
redwood: -3604
imola:   -4160
egg:     -4052