798567 - convert CSSValue / CSSPrimitiveValue / CSSValueList to WebIDL

Status: RESOLVED FIXED

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

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: wip

This needs to be cleaned a bit.

I'm currently dealing with an assert that a rootedObject in the bindings is not a temporary it really isn't a temporary, but in genericGetter when we call get_cssvalueType() we end up calling nsROCSSPrimitivevalue::GetCssText() which scribles on the stack instead of calling CssvalueType()

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: patch

tell me what style / other stuff I got wrong here :)

Sorry its a rather huge patch, but a lot of it is just s/nsIDOMCSSValue/CSSValue/ on all the nsComputedDOMStyle::DoGet*() functions

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: interdiff of things I noticed while working on other stuff

- fix GetFloatValue() for CSS_NUMBER mType check

its been a while, so I'm not absolutely sure, but I don't believe make mochitest-plain in layout/style caught this :(Would you like me to add one here? if so should it be something other than have an element whose computed style has a css number value for something and test that GetFloatValue() for it doesn't throw?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Would you like me to add one here?

Please.  And yeah, do something like test that getFloatValue on the opacity value returns the right thing, say.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I'm sorry for the lag here...  I'm making progress on this; hope to have it done early this coming week.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 674872 [details] [diff] [review]
interdiff of things I noticed while working on other stuff

>+++ b/layout/style/CSSValue.h
>+#pragma once

Given recent discussion about this, we should probably use include guards instead.  :(

I'll update the example codegen accordingly.

>+class CSSValue : public nsWrapperCache

This needs to inherit from nsISupports before nsWrapperCache, if it's an nsISupports-based DOM binding.  I don't think you need to actually implement nsISupports here, since this class is pure-virtual anyway.  But incidentally that would mean you don't need the manual addref/release decls.

I think if you do that you can also get rid of AsDOMCSSValue.

>+++ b/layout/style/nsROCSSPrimitiveValue.cpp

The indentation in here looks off in the context for the diff....  presumably it's that way from the big patch.

r=me with the above comments addressed.

Review for the big patch coming up.  Please post interdiffs when all is said and done?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 674872 [details] [diff] [review]
interdiff of things I noticed while working on other stuff

Actually, one more issue here.  CSSValue should be in the mozilla::dom namespace, no?

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #7)
> Comment on attachment 674872 [details] [diff] [review]
> interdiff of things I noticed while working on other stuff
> 
> Actually, one more issue here.  CSSValue should be in the mozilla::dom
> namespace, no?

I wasn't sure if you wanted it there or in  mozila::css so I left it and figured you'd tell me.

(In reply to Boris Zbarsky (:bz) from comment #6)
> Comment on attachment 674872 [details] [diff] [review]
> interdiff of things I noticed while working on other stuff
> 
> >+++ b/layout/style/CSSValue.h
> >+#pragma once
> 
> Given recent discussion about this, we should probably use include guards
> instead.  :(

yeah, what is it supposed to be? mozilla_dom_CSSValue_h_ ?

> I'll update the example codegen accordingly.
> 
> >+class CSSValue : public nsWrapperCache
> 
> This needs to inherit from nsISupports before nsWrapperCache, if it's an
> nsISupports-based DOM binding.  I don't think you need to actually implement
> nsISupports here, since this class is pure-virtual anyway.  But incidentally
> that would mean you don't need the manual addref/release decls.
> 
> I think if you do that you can also get rid of AsDOMCSSValue.

ok, I wasn't sure if I needed to do that or not, and it seemed to work as is, but I'll fix that.

> >+++ b/layout/style/nsROCSSPrimitiveValue.cpp
> 
> The indentation in here looks off in the context for the diff.... 
> presumably it's that way from the big patch.

I believe its right at the end, but I known to often get it wrong :/

> Review for the big patch coming up.  Please post interdiffs when all is said
> and done?

on top of this interdiff or including it? either way wfm

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I wasn't sure if you wanted it there or in  mozila::css

For the DOM stuff, I prefer mozilla::dom.  ;)

> yeah, what is it supposed to be? mozilla_dom_CSSValue_h_ ?

Sure.

> ok, I wasn't sure if I needed to do that or not, and it seemed to work as is

I'm pretty sure I can write testcases that will at least crash with the code as-is...

> on top of this interdiff or including it? either way wfm

On top.  Basically a diff from "what bz reviewed" to "what's in my tree".  ;)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 671261 [details] [diff] [review]
patch

>+++ b/dom/interfaces/css/nsIDOMCSSStyleDeclaration.idl
>+[binaryname(ScriptGetPropertyCSSValue)]
>   nsIDOMCSSValue     getPropertyCSSValue(in DOMString propertyName);

I would rather we did one of two things:

1)  Name the function that has a CSSValue** ourparam
    GetPropertyCSSValueInternal
2)  Rewrite the existing GetPropertyCSSValue to return an
    already_AddRefed<CSSValue> and then we'll just have two
    GetPropertyCSSValues with different signatures, which is fine.

>+++ b/dom/webidl/CSSPrimitiveValue.webidl
>+[Throws]

Please indent this annotation by two spaces, and similar for the other ones in this file.

>+++ b/dom/webidl/CSSStyleDeclaration.webidl
>+  CSSValue? getPropertyCSSValue(DOMString property);

Oh, _very_ good catch.

>+++ b/dom/webidl/CSSValue.webidl
>+[Throws]
>+           attribute DOMString        cssText;

I'd prefer this were just indented sanely.

>+                                        // raises(DOMException) on setting

No need; the [Throws] says it throws.  ;)

>+++ b/dom/webidl/CSSValueList.webidl
>+  CSSValue           item(unsigned long index);

This will regress access to the list elements using [n] notation, no?  Do we really not have a test for that?  If so, would you mind adding one, verifying that it fails, then marking that method as "getter" and implementing the IndexedGetter method for it?

>+++ b/dom/webidl/WebIDL.mk
>+	CSSPrimitiveValue.webidl \
>   CSSStyleDeclaration.webidl \
>+	CSSValue.webidl \
>+	CSSValueList.webidl \

Please fix the indent.  This list is using spaces, not tabs, for the indentation.

>+++ b/layout/style/nsCSSRules.h
>+  NS_IMETHOD GetPropertyCSSValue(const nsAString& aProp, CSSValue **aVal) MOZ_OVERRIDE;

So if we do what I propose above, this would look like this:

  already_AddRefed<CSSValue> GetPropertyCSSValue(const nsAString& aProp,
                                                 mozilla::ErrorResult& aRv);

>+++ b/layout/style/nsComputedDOMStyle.cpp
> SetValueToCalc(const nsStyleCoord::Calc *aCalc, nsROCSSPrimitiveValue *aValue)
>+  nsRefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue(nullptr);

Hmm.  So I'm not sure we need a parent object for nsROCSSPrimitiveValue at all.  It can only be returned from CSSValueList and the computed style declaration, which are both WebIDL objects.  So I think you can just make GetParentObject() on it return null and not need this constructor argument...

Similar for the CSSValueList; I don't think it needs an interesting parent object.

>+++ b/layout/style/nsDOMCSSDeclaration.cpp
> nsDOMCSSDeclaration::GetPropertyCSSValue(const nsAString& aPropertyName,
>-  return NS_OK;
>+  return NS_ERROR_NOT_IMPLEMENTED;

Please leave that as NS_OK.

>+++ b/layout/style/nsDOMCSSValueList.cpp
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsDOMCSSValueList)

Should be outdended by 2 spaces, as should most of the lines down to the '{' of WrapObject.

>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_MEMBER(mComputedStyle, nsComputedDOMStyle)

This should just be NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR, I think.  Though we don't need it at all if we remove the member.

>+  uint32_t values = tmp->Length();
>+  for (uint32_t i = 0; i < values; i++) {
>+    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_RAWPTR(mCSSValues[i]->AsDOMCSSValue())
>+  }

This can be NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_OF_NSCOMPTR(mCSSValues), I believe, once CSSValue inherits from nsISupports.

You need a NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS in here.

>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsDOMCSSValueList)

And in here a NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER.

Basically, you want to do something like NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1, but instead of a single smart-pointer field you have your array.

>+++ b/layout/style/nsDOMCSSValueList.h
>+    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(nsDOMCSSValueList)

Please fix the indent.

>+      virtual uint16_t CssValueType() const MOZ_OVERRIDE MOZ_FINAL;

And here.

>+  uint32_t Length()

This can be const.

>+  JSObject *WrapObject(JSContext *cx, JSObject *scope, bool *triedToWrap);

This is virtual.  Might as well say so.

>+++ b/layout/style/nsICSSDeclaration.h
>+  NS_IMETHOD ScriptGetPropertyCSSValue(const nsAString& aProp, nsIDOMCSSValue** aVal)
>+  {
>+    CSSValue* val;
>+    nsresult rv = GetPropertyCSSValue(aProp, &val);
>+    if (val) {

This is not safe.  If an implementation returns error without setting the our param (e.g. like nsCSSFontFaceStyleDecl does) this will test random memory.. and then probably dereference it.

Another reason to switch to a GetPropertyCSSValue that returns already_AddRefed<CSSValue>. ;)

>+  already_AddRefed<CSSValue>
>     GetPropertyCSSValue(const nsAString& aPropName, mozilla::ErrorResult& rv) {

So basically, this would become virtual is what I'm proposing.

>+#define NS_DECL_NSIDOMCSSSTYLEDECLARATION_HELPER \
>+      NS_IMETHOD GetCssText(nsAString & aCssText); \

Please fix the indent here.  Though you might not need this at all if we switch the way GetPropertyCSSValue works.

>+++ b/layout/style/nsROCSSPrimitiveValue.cpp
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsROCSSPrimitiveValue)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_MEMBER(mComputedStyle, nsComputedDOMStyle)

See comments for valuelist's traverse.

>+nsROCSSPrimitiveValue::GetFloatValue(uint16_t aUnitType, ErrorResult& aRv)
>     case CSS_IN :
>+      if (mType == CSS_PX) {
>+      return mValue.mAppUnits / nsPresContext::AppUnitsPerCSSInch();

Please fix indent.

>     case CSS_PC :
>+      if (mType == CSS_PX) {
>+      return mValue.mAppUnits * 6.0f /
>         nsPresContext::AppUnitsPerCSSInch();

And here.

>     case CSS_PERCENTAGE :
>+      if (mType == CSS_PERCENTAGE) {
>+        return mValue.mFloat * 100;
>+  }

And here.

>     case CSS_NUMBER :
>+      if (mType != CSS_NUMBER) {
>+      return mValue.mFloat;

And here.  Plus the change in your second patch.

>@@ -333,30 +389,46 @@ nsROCSSPrimitiveValue::GetFloatValue(uint16_t aUnitType, float* aReturn)
>+      aRv.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);

Please fix indent.

> NS_IMETHODIMP
> nsROCSSPrimitiveValue::GetStringValue(nsAString& aReturn)

Please document somewhere that this is infallible, which is why you don't need a WebIDL version, since this one is API-compatible.

>+nsROCSSPrimitiveValue::GetCounterValue(ErrorResult& aRv)
>+  aRv.Throw(NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR);
>+      return nullptr;
>+      }

Please fix the indent.

>+nsROCSSPrimitiveValue::GetRectValue(nsIDOMRect** aRect)
>+ErrorResult error;

And here.

>+  if (rect) {
>+    rect.forget(aRect);
>+    return NS_OK;
>+  }
>+
>+  return error.ErrorCode();

How about:

  rect.forget(aRect);
  return error.ErrorCode();

without the branching?  Or even skip the temporary and do:

  ErrorResult error;
  *aRect = GetRectValue(error).get();
  return error.ErrorCode();

>+nsROCSSPrimitiveValue::GetRGBColorValue(nsIDOMRGBColor** aColor)

Indentation is off through this whole function.  Also, same comment as for GetRectValue in terms of not needing the branch..

>+++ b/layout/style/nsROCSSPrimitiveValue.h
>+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>+    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(nsROCSSPrimitiveValue)

Fix indent.

>+  uint16_t PrimitiveType()

Add a comment right before this that says "// CSSPrimitiveValue"?

r=me with the above comments addressed, and thank you for doing this!  Sorry the review took so long.  :(

Comment 11

[Trevor Saunders (:tbsaunde)]

> 2)  Rewrite the existing GetPropertyCSSValue to return an
>     already_AddRefed<CSSValue> and then we'll just have two
>     GetPropertyCSSValues with different signatures, which is fine.

So, the only real reason I didn't do that was lazyness, but actually just different signatures isn't good enough because AIUI c++ wants you to over ride all  overloads if you override one.  I guess we could just have a bunch of the same impl of the xpcom GetPropertyCssValue() method around but that seems silly.

> >+++ b/dom/webidl/CSSValueList.webidl
> >+  CSSValue           item(unsigned long index);
> 
> This will regress access to the list elements using [n] notation, no?  Do we
> really not have a test for that?  If so, would you mind adding one,
> verifying that it fails, then marking that method as "getter" and
> implementing the IndexedGetter method for it?

ok

> >+++ b/layout/style/nsComputedDOMStyle.cpp
> > SetValueToCalc(const nsStyleCoord::Calc *aCalc, nsROCSSPrimitiveValue *aValue)
> >+  nsRefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue(nullptr);
> 
> Hmm.  So I'm not sure we need a parent object for nsROCSSPrimitiveValue at
> all.  It can only be returned from CSSValueList and the computed style
> declaration, which are both WebIDL objects.  So I think you can just make
> GetParentObject() on it return null and not need this constructor argument...

yeah, I think CSSDeclaration was still prefable when I did that.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> c++ wants you to over ride all  overloads if you override one.

I don't think that's the case.  Something like this:

  class  A {
    virtual int foo();
    void foo(int* arg);
  };
  class B : public A {
    virtual int foo();
    using A::foo;
  };

works fine last I checked.  Does doing that make the GetPropertyCSSValue situation better?

Comment 13

[Trevor Saunders (:tbsaunde)]

Attachment: fix comments

I think the test covers the stuff I broke, and the rest of it gets the other comments.  Sorry it took a while to get back to this :(

Comment 14

[Boris Zbarsky [:bzbarsky]]

Will try to get to this next week...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 688886 [details] [diff] [review]
fix comments

>+++ b/dom/bindings/Bindings.conf
>+++ b/dom/imptests/failures/html/tests/submission/Mozilla/test_pageload-video.html.json
>+  "The document for a standalone media file should have one child in the body.": true

Why is that a new issue with this patch?  Would like to undertand this....

>+++ b/dom/webidl/CSSValue.webidl
>-[Throws]
>-           attribute DOMString        cssText;
>+  [Throws]
>+  attribute DOMString        cssText;

Unfortunately, "sanely" in this case means leaving the "attribute" lined up with the other "attribute" as in the spec and indenting the "[Throws]" to line up with it.

>+++ b/dom/webidl/CSSValueList.webidl
>   readonly attribute unsigned long    length;
>-  CSSValue           item(unsigned long index);
>+  getter CSSValue           item(unsigned long index);

Fix the indent so "item" lines up with the "unsigned"?

>+++ b/layout/style/CSSValue.h
>+class CSSValue : public nsISupports,
>+  public nsWrapperCache

Please line up the two "public" here.

>+++ b/layout/style/nsDOMCSSValueList.cpp
>-  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>-  NS_INTERFACE_MAP_ENTRY(nsIDOMCSSValue)
>-  NS_INTERFACE_MAP_ENTRY(nsISupports)
>+NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>+NS_INTERFACE_MAP_ENTRY(nsIDOMCSSValue)
>+NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, CSSValue)

The old indent here was correct...  Please put it back.

>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsDOMCSSValueList)

I don't think NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_OF_NSCOMPTR and NS_IMPL_CYCLE_COLLECTION_UNLINK_NSTARRAY exist anymore...  The new setup is, I think, sane enough that you can just do:

IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(nsDOMCSSValueList, mCSSValues)

and it'll do all the NS_IMPL_CYCLE_COLLECTION_* stuff for you.

>+++ b/layout/style/nsROCSSPrimitiveValue.cpp
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS

This should be indented by two spaces.

>   NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsROCSSPrimitiveValue)

This line should be outdented by two spaces.

>   NS_IMPL_CYCLE_COLLECTION_UNLINK_END

As should this one.

>+++ b/layout/style/nsROCSSPrimitiveValue.h
>+  void GetStringValue(nsString& aString, mozilla::ErrorResult& aRv);

I don't understand what will call this.  In the IDL, getStringValue is not marked [Throws]....  Again, either we should mark the IDL method as [Throws] or we can add comments saying that the XPCOM getStringValue never returns error or something.

r=me with those nits fixed and the new test failure bit explained.

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #15)
> Comment on attachment 688886 [details] [diff] [review]
> fix comments
> 
> >+++ b/dom/bindings/Bindings.conf
> >+++ b/dom/imptests/failures/html/tests/submission/Mozilla/test_pageload-video.html.json
> >+  "The document for a standalone media file should have one child in the body.": true
> 
> Why is that a new issue with this patch?  Would like to undertand this....

I think (though I haven't confirm with try yet) that its an issue with my machine( it constantly fails a couple of media related a11y tests too).

> >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsDOMCSSValueList)
> 
> I don't think NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_OF_NSCOMPTR and
> NS_IMPL_CYCLE_COLLECTION_UNLINK_NSTARRAY exist anymore...  The new setup is,
> I think, sane enough that you can just do:
> 
> IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(nsDOMCSSValueList, mCSSValues)
> 
> and it'll do all the NS_IMPL_CYCLE_COLLECTION_* stuff for you.

yeah, its based off an old rev, it'll be fixed when I rebase before landing.

> >+++ b/layout/style/nsROCSSPrimitiveValue.h
> >+  void GetStringValue(nsString& aString, mozilla::ErrorResult& aRv);
> 
> I don't understand what will call this.  In the IDL, getStringValue is not
> marked [Throws]....  Again, either we should mark the IDL method as [Throws]
> or we can add comments saying that the XPCOM getStringValue never returns
> error or something.

I checked the xpcom version saw it failed in some odd case, added a version that could throw, and then made the silly mistake of forgeting to update the idl, I'll add [Throws]

Comment 17

[Boris Zbarsky [:bzbarsky]]

> I think (though I haven't confirm with try yet) that its an issue with my machine

OK.  If try is green without that bit, please take it out.  ;)

Comment 18

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5aed5087fb7b

Comment 19

[Trevor Saunders (:tbsaunde)]

Boris, when you get a chance can you look at the graph and see if the regression looks real?

Regression :( Trace Malloc Allocs increase 0.557% on CentOS (x86_64) release 5 (Final) Mozilla-Inbound
------------------------------------------------------------------------------------------------------
    Previous: avg 531435.467 stddev 1233.172 of 30 runs up to revision 93beff102db2
    New     : avg 534396.000 stddev 520.961 of 5 runs since revision 5aed5087fb7b
    Change  : +2960.533 (0.557% / z=2.401)
    Graph   : http://mzl.la/TXosAn

Comment 20

[Boris Zbarsky [:bzbarsky]]

Looks like a flat graph to me....

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5aed5087fb7b

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5aed5087fb7b

Comment 23

[Wladimir Palant]

Adding addon-compat keyword - current Adblock Plus version uses nsIDOMCSSValueList.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Hrm.  This was back before we were thinking to check for such things.  :(

Addons mxr shows 10 hits for nsIDOMCSSValueList, mostly in this AppIntegration.jsm thing.  It sounds like we need to put it back for now and work on exposing WebIDL stuff in components...

Trevor, do you have time to do that (soon!), or should I?

Comment 25

[Wladimir Palant]

I already fixed it in Adblock Plus and I can do a minor release if this helps - normally however I wouldn't consider the issue important enough to release before the change hits Firefox Beta (it sometimes causes Adblock Plus context menu entries to not appear even though they should). AppIntegration.jsm is outdated Adblock Plus code by the way, you probably found one of our forks - context menu handling lives in ui.js now.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Yeah, those look like various forks of ABP (plus another addon that doesn't use that file).

But we still shouldn't break them.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Bug 830396 is tracking readding the interface.