Status

()

Core
DOM: CSS Object Model
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Christopher Aillon (sabbatical, not receiving bugmail), Assigned: Christopher Aillon (sabbatical, not receiving bugmail))

Tracking

({memory-footprint, perf})

Trunk
x86
Linux
memory-footprint, perf
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

i. We should support CSSValueLists
  ii. We should support computing styles of -moz-border-{side}-colors
 iii. Computed style should QI less
  iv. Time sensitive areas (Txul) shouldn't depend on computed style.
   v. We should cache the computed style factory object.
  vi. Remove code that wants computed style of non-existant properties.
 vii. Use nsSize instead of nsRect where we only care about height and/or width.
viii. Other random foo.
Created attachment 90065 [details] [diff] [review]
Patch v1.1

Addresses a few things pointed out by sicking, some domclassinfo foo, updated
the MANIFEST_IDL, Windows makefile changes I forgot, and a couple comments from
jag.
Attachment #90035 - Attachment is obsolete: true
Created attachment 90209 [details] [diff] [review]
Patch v.1.2

This is ready for reviews.  Only minor changes from the last patch, the most
significant being adding a nsCSSValueListSH to DOMClassInfo.
Attachment #90065 - Attachment is obsolete: true
> +   * @param  index          Specifies the desired index of the
> +   *                        CSSValue in the list.

"the index of the desired CSSValue"

> +nsCSSValueListSH::GetItemAt(nsISupports *aNative, PRUint32 aIndex,
> +                              nsISupports **aResult)

Indentation?

> +  nsIDOMCSSValue *cssValue = nsnull;

Add a brief comment saying this is a weak ref so that *aResult gets the ownership?

> +nsIFactory *GlobalWindowImpl::sComputedDOMStyleFactory = nsnull;

We leak this on shutdown with the current patch....

> +  nsCOMPtr<nsIDOMCSSValue> val;

With this change, the end of this function can just assign val to aReturn and
addref aReturn. Or better yet just pass aReturn to all those functions...  While
you're doing that, make the out param a nsIDOMCSSValue**, ok?  If the funcs
fail, just set *aReturn to null and all that.

> +nsComputedDOMStyle::GetBorderLeftColors(nsIFrame *aFrame,
> +                                          nsIDOMCSSValue*& aValue)

Indentation; here and the next few functions

> +        NS_ENSURE_TRUE(primitive, NS_ERROR_OUT_OF_MEMORY);

This leaks valueList

> +          delete primitive;
> +          return NS_ERROR_OUT_OF_MEMORY;

This leaks valueList

> +        valueList->AppendCSSValue(primitive);

Check the return value, esp. since this can actually fail in a useful (OOM) way.

> +      nsresult rv = CallQueryInterface(valueList, &aValue);
> +      return rv;

Why do we need rv?  ;)

nsDOMCSSValueList.cpp:

> +#include "prprf.h"

Could you use prtypes.h instead, unless you really need the stuff in prprf?

> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMCSSValueList)

This is not ambiguous, is it?

> +  if (NS_SUCCEEDED(rv) && mCSSValues) {

Put that check inside the |+  if (!mCSSValues) {| block, with an early return.

> +    mCSSValues->AppendElement(aValue);

Check the return value here too, please...  Watch out for weirdness with
nsresult vs PRBool (can't recall whether that's nsSupportsArray or nsVoidArray
or both...).

+    separator.Assign(NS_LITERAL_STRING(" "));

separator.Assign(PRUnichar(' '));

> +    nsCOMPtr<nsIDOMCSSValue> cssValue;

Put this outside the for() loop.

> +    if (cssValue) {

Can we assert that there's a cssValue?



nsDOMCSSValueList.h:

> +  private:
> +    nsISupports*                mOwner;

Let's not have this member, ok?  It's trivial for this to become a dangling
pointer, so all it's doing is taking up space.

Created attachment 90303 [details] [diff] [review]
Patch v.1.3

Address bz's concerns.
Attachment #90209 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Keywords: footprint, perf
Created attachment 90399 [details] [diff] [review]
Patch v.1.3.1

Fix a potential bustage on some compilers.  And a few comment changes and
additions that I felt could be clarified.
Attachment #90303 - Attachment is obsolete: true
Comment on attachment 90399 [details] [diff] [review]
Patch v.1.3.1

- In nsDOMClassInfo.cpp:

+  // Get the computed -moz-binding directly from the style context
+  nsCOMPtr<nsIPresContext> pctx;
+  shell->GetPresContext(getter_AddRefs(pctx));
+  NS_ENSURE_TRUE(pctx, NS_ERROR_UNEXPECTED);
+
+  nsCOMPtr<nsIStyleContext> sctx;
+  rv = pctx->ResolveStyleContextFor(content, nsnull,
+				     getter_AddRefs(sctx));
+  NS_ENSURE_SUCCESS(rv, rv);

Is ResolveStyleContextFor() cheaper than GetPrimaryFrameFor() + GetStyleData()
on the frame?

...
-  if (value.IsEmpty()) {
+  if (display->mBinding.IsEmpty()) {
     // No binding, nothing left to do here.
-
     return NS_OK;
   }

Don't remove that empty line, always empty line before return in
nsDOMClassInfo.cpp

- In doDestroyComputedDOMStyle()

+{
...
+  return;
+}

This method returns void, so no need for that return call at the end of the
function.

- In nsDOMCSSValueList::GetLength():

+  PRUint32 count = 0;
+  if (mCSSValues) {
+    mCSSValues->Count(&count);
+  }
+
+  *aLength = count;
+  return NS_OK;

Why the temporary count variable here?

- In nsDOMCSSValueList.h:

+class nsDOMCSSValueList : public nsIDOMCSSValueList
+{
+  public:
+    NS_DECL_ISUPPORTS
+
+    // nsIDOMCSSValueList
+    NS_DECL_NSIDOMCSSVALUELIST
...

Pull all that back 2 spaces to match the indentation in the declaration of
other related classes.

Other than those nits, sr=jst
Attachment #90399 - Flags: superreview+
> doDestroyComputedDOMStyle

Where is this called from?

+        nsresult rv = valueList->AppendCSSValue(primitive);
+        if (NS_FAILED(rv)) {
+          delete valueList;
+          delete primitive;
+          delete rgb;

Take the |delete rgb;| part out -- deleting |primitive| deletes rgb for you.

nsDOMCSSValueList.h:

> +    nsISupports*                mOwner;

Please fully remove. ;)
Created attachment 90461 [details] [diff] [review]
Patch v.1.3.141592654....
Attachment #90399 - Attachment is obsolete: true
Comment on attachment 90461 [details] [diff] [review]
Patch v.1.3.141592654....

r=bzbarsky, rolling jst's sr along since you addressed his comments.

Make sure not to check in the "parent" changes to nsDOMClassInfo.{h,cpp},
please.
Attachment #90461 - Flags: superreview+
Attachment #90461 - Flags: review+
Checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
As expected, tinderbox Txul times dropped in the 1-2% range with this patch.
(comet's numbers seem exceptionally good, but I'll take it :-) )  There was a
lot of red this morning, so I have less numbers to go by than I'd like, but this
is still pretty indicative.

The last column has the percentage gain for the best time to the worst time,
which is why ash looks funny.  It got a better gain in the best time than the
worst time.

Tbox           Before            After         Pct Gain

comet        471 -  486ms     459 -  467ms    2.5 - 3.9
luna        1099 - 1109ms    1088 - 1096ms    1.0 - 1.2
monkey      1433 - 1460ms    1413 - 1436ms    1.4 - 1.6
ash         1737 - 1749ms    1710 - 1722ms    1.6 - 1.5
mecca       2053 - 2073ms    2029 - 2041ms    1.2 - 1.5
openwound   6191 - 6327ms    6163 - 6177ms    0.5 - 2.4
You need to log in before you can comment on or make changes to this bug.