Closed Bug 230976 Opened 20 years ago Closed 20 years ago

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


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

Not set





(Reporter: caillon, Assigned: caillon)


(Keywords: memory-footprint)


(1 file)

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/
   text    data     bss     dec     hex filename
3622534  221856   23392 3867782  3b0486 layout/build/

> size layout/build/
   text    data     bss     dec     hex filename
3617601  221900   23424 3862925  3af18d layout/build/
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.
Attached patch Patch v1Splinter Review
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.
Attachment #139151 - Flags: superreview?(bz-vacation)
Attachment #139151 - Flags: review?(bz-vacation)
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.
Attachment #139151 - Flags: superreview?(bz-vacation)
Attachment #139151 - Flags: superreview+
Attachment #139151 - Flags: review?(bz-vacation)
Attachment #139151 - Flags: review+
> OK this change with dbaron?

It's fine with me.
>>-  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 =

Other changes were made and checked in.
Closed: 20 years ago
Resolution: --- → FIXED
Zdiff numbers from tinderboxes:

luna:    -5860
brad:    -3536
beast:   -1854
redwood: -3604
imola:   -4160
egg:     -4052
You need to log in before you can comment on or make changes to this bug.