155525 - Computed style potpourri

Status: RESOLVED FIXED

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

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v.1.0

:-)

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: 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.

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: 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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> +   * @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.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v.1.3

Address bz's concerns.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: 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.

Comment 7

[Johnny Stenback (:jst)]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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. ;)

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v.1.3.141592654....

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Mac build foo

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Checked in

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

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