If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Convert CSSStyleDeclaration to Paris bindings

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla17
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:?)

Details

Attachments

(7 attachments, 16 obsolete attachments)

1.04 KB, text/html
Details
962 bytes, text/html
Details
5.86 KB, patch
peterv
: review+
dbaron
: review+
Details | Diff | Splinter Review
20.48 KB, patch
peterv
: review+
dbaron
: review+
Details | Diff | Splinter Review
17.96 KB, patch
Details | Diff | Splinter Review
15.39 KB, patch
Details | Diff | Splinter Review
7.94 KB, patch
peterv
: review+
Details | Diff | Splinter Review
There are several pieces of work here.  Specifically:

1)  Need to make DOMCSSDeclarationImpl cycle-collected and wrapper-cached.  Move
    the nsWrapperCache inheritance higher up the inheritance chain.
2)  Expose the new-binding APIs on it.
3)  Hook up the WebIDL generation for CSSStyleDeclaration.webidl from
    nsCSSPropList.h and the like, and flip the binding on.

Peter's comments on #1:

  It seems to me that you'll need to make DOMCSSStyleRule participate and
  traverse/trace/unlink anything in DOMCSSDeclarationImpl (and
  nsDOMCSSDeclaration) from there. You also need to implement QI in
  DOMCSSDeclarationImpl and forward the QI for the
  nsXPCOMCycleCollectionParticipant and nsCycleCollectionISupports to
  DOMCSSStyleRule's QI. I think everything else should just work for now, but I
  might be missing something. Things will probably become more complicated once
  we preserve the wrappers for DOMCSSStyleRule too.
Depends on: 753518
Created attachment 622491 [details] [diff] [review]
Add an explicit way for GetParentObject to return an (nsISupports*,nsWrapperCache*) pair.
Attachment #622491 - Flags: review?(peterv)
Blocks: 753522
Comment on attachment 622491 [details] [diff] [review]
Add an explicit way for GetParentObject to return an (nsISupports*,nsWrapperCache*) pair.

typo+bzexport fail.  :(
Attachment #622491 - Attachment is obsolete: true
Attachment #622491 - Flags: review?(peterv)
Created attachment 623568 [details]
Testcase to remeasure/reprofile once this is fixed

A good bit of the cost here is the Resolve on the current CSS decl.

And the reason for that is that CSS decls support indexed access.  Something the spec conveniently forgot to mention....

This also means that fixing this bug is a bit more of a pain.  Do we want to keep the old indexed access setup (with a resolve hook and whatnot), or do we want to try to make this an actual proxy and just pay the resulting performance cost for .style.foo access?  :(
Created attachment 623570 [details]
Testcase to measure some performance stuff

Ok, so I tried to quantify the perf hit from doing a proxy.

Using the attached testcase on my machine, the average time needed per property get/set is as follows, if I'm measuring right (with old bindings; new bindings are a little faster for cssdecl):

* Get property with no value from CSS decl: 48ns
* Get property with value from CSS decl: 185ns
* Set property on CSS decl: 1044ns
* Get .item property on nodelist proxy: 63ns

Note that the testcase does 1e6 iterations, so the times alerted, in ms are pretty much the times per operation in ns.

The 185 for a get is actually largely gated on all the string-copying and string allocation plus resulting GC; if I bump the iteration count by a factor of 10, the per-operation cost of that item goes up to close to 300ns due to more GC.

In any case, switching from a non-proxy to a proxy would basically make us pay at least 15ns and at most 63ns extra for those middle two items in the list.  That might well be ok, esp. because there's tons the JS engine can do to make the proxies faster.  Though we might need to remeasure with the actual CSS decl proxy, because I seem to recall that we use a linear cache for the proto's properties, and CSS decl has a _lot_ of properties on the proto, unlike nodelists.
For what it's worth, I did some timing with the Paris bindings for CSSStyleDeclaration.  The times there are like so:

* Get property with no value from CSS decl: 46ns
* Get property with value from CSS decl: 176ns (might just be GC noise)
* Set property on CSS decl: 1050ns (this one is pretty noisy, of course)
* Get .item property on nodelist proxy: 62ns

So not a significant difference.
Created attachment 623906 [details] [diff] [review]
Part 1: GetParentObject changes.  Not requesting review yet, because this isn't really right.
Created attachment 623908 [details] [diff] [review]
part 2.  Make all CSS declarations wrappercached and make them correctly handle preserved wrappers.

Asking for several reviews: mccr8 for the skippability changes smaug suggested, dbaron for the general CSS stuf, peterv and smaug for the DOMCSSDeclarationImpl hackery
Attachment #623908 - Flags: review?(peterv)
Attachment #623908 - Flags: review?(dbaron)
Attachment #623908 - Flags: review?(continuation)
Attachment #623908 - Flags: review?(bugs)
Created attachment 623910 [details] [diff] [review]
part 3.  Expose the API needed for Paris bindings on nsDOMCSSDeclaration.
Attachment #623910 - Flags: review?(dbaron)
Created attachment 623912 [details] [diff] [review]
part 4.  Set up auto-generation of CSSStyleDeclaration.webidl from nsCSSPropList.h and enable Paris bindings for CSSStyleDeclaration.

Actually hooks things up.  Kyle, could you review the various build system bits?  Peter, the .conf file changes and whatever else you want to review in dom/.  David, the small changes to layout/style?
Attachment #623912 - Flags: review?(peterv)
Attachment #623912 - Flags: review?(khuey)
Attachment #623912 - Flags: review?(dbaron)
Do nsDOMCSSAttributeDeclaration and nsComputedDOMStyle still need to be AMBIGUOUS given that they are now singly inherited?  I'm not 100% clear on when exactly that is needed.
They;re still multiply inherited from nsISupports, which is the part that matters here.
Ah, I see, I thought it said nsCSSDeclaration not nsICSSDeclaration.  That makes more sense...
Comment on attachment 623908 [details] [diff] [review]
part 2.  Make all CSS declarations wrappercached and make them correctly handle preserved wrappers.


>-NS_IMPL_ADDREF(DOMCSSStyleRule)
>-NS_IMPL_RELEASE(DOMCSSStyleRule)
>+NS_IMPL_CYCLE_COLLECTING_ADDREF(DOMCSSStyleRule)
>+NS_IMPL_CYCLE_COLLECTING_RELEASE(DOMCSSStyleRule)
Just curious, are there cases when we end up addref/release a lot?
(Since CC-addref/release is quite a bit slower)


>+NS_INTERFACE_MAP_BEGIN(nsCSSKeyframeStyleDeclaration)
>+  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>+  NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(nsCSSKeyframeStyleDeclaration)
>+NS_INTERFACE_MAP_END_INHERITING(nsDOMCSSDeclaration)
Couldn't you use
NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsCSSKeyframeStyleDeclaration)
and drop NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(nsCSSKeyframeStyleDeclaration)
Attachment #623908 - Flags: review?(bugs) → review+
> Just curious, are there cases when we end up addref/release a lot?

DOM manipulation of DOMCSStyleRule in general is pretty rare; when it does happen it's not too perf-critical.

> Couldn't you use

Yes, I could.  Will do!
Comment on attachment 623908 [details] [diff] [review]
part 2.  Make all CSS declarations wrappercached and make them correctly handle preserved wrappers.

Review of attachment 623908 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsComputedDOMStyle.cpp
@@ +178,2 @@
>  NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(nsComputedDOMStyle)
> +  if (tmp->mContent && nsGenericElement::CanSkip(tmp->mContent, true)) {

I'm afraid Olli has led you astray here.  He wrote this code as though mContent owns the nsComputedDOMStyle, but last time I looked this wasn't the case.  If the mContent doesn't own nsComputedDOMStyle, then just change CAN_SKIP, CAN_SKIP_IN_CC and CAN_SKIP_THIS to "return tmp->IsBlack()".  (This is because if the cache is black, then it will be alive, and it owns the style, so we don't need to traverse the style.)  We should add a macro for that.  You can also remove the warnings about things needing to be updated, and not being part of garbage cycles, etc.

::: layout/style/nsDOMCSSAttrDeclaration.cpp
@@ +78,4 @@
>  
> +// nsDOMCSSAttributeDeclaration has only one cycle collected field,
> +// other than its preserved wrapper, so if mElement is going to be
> +// skipped, the attribute declaration can't be part of a garbage

You can remove this comment and the one above WRAPPERCACHE_1, because this weirdo optimization can be removed.

@@ +83,1 @@
>  NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(nsDOMCSSAttributeDeclaration)

I think it would be nice to have a comment in here saying that mElement owns the declaration.

@@ +83,5 @@
>  NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(nsDOMCSSAttributeDeclaration)
> +  if (tmp->mElement && nsGenericElement::CanSkip(tmp->mElement, true)) {
> +    if (tmp->PreservingWrapper()) {
> +      JSObject* o = tmp->GetWrapperPreserveColor();
> +      xpc_UnmarkGrayObject(o);

You could just do GetWrapper() here and drop the UnmarkGrayObject, as GetWrapper just does the same thing as this whole code sequence.

@@ +92,4 @@
>  NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_END
>  
>  NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(nsDOMCSSAttributeDeclaration)
> +  if (tmp->PreservingWrapper()) {

I don't think you need to check PreservingWrapper() here: our cached wrapper owns us in either case.  In other words, this whole added clump can be tmp->IsBlack().

@@ +101,1 @@
>    return !tmp->mElement || nsGenericElement::CanSkipInCC(tmp->mElement);

This isn't really right any more.  If mElement is NULL, we can't assume anything about skippability, because we have an additional out edge, to our wrapper (well, sometimes).  We can skip a declaration if our cached wrapper is black, or if we have an element and we're going to skip the element (because the element owns the decl).

The full function could then be:
return tmp->IsBlack() || (tmp->mElement && nsGenericElement::CanSkipInCC(tmp->mElement))

Here and in CAN_SKIP_THIS, it is probably okay to leave it as tmp->IsBlack() and not worry about edge cases too much until proven otherwise.

@@ +101,5 @@
>    return !tmp->mElement || nsGenericElement::CanSkipInCC(tmp->mElement);
>  NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_END
>  
>  // CanSkipThis returns false to avoid problems with incomplete unlinking.
>  NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_BEGIN(nsDOMCSSAttributeDeclaration)

The body of CAN_SKIP_THIS can be "return tmp->IsBlack();".  Or you could add the extra clause checking mElement if you want (using CanSkipThis instead of CanSkipInCC).
Attachment #623908 - Flags: review?(continuation) → review-
Sorry. /me should never write code to an email without looking at the existing code.
Comment on attachment 623912 [details] [diff] [review]
part 4.  Set up auto-generation of CSSStyleDeclaration.webidl from nsCSSPropList.h and enable Paris bindings for CSSStyleDeclaration.

Review of attachment 623912 [details] [diff] [review]:
-----------------------------------------------------------------

r- because I want to see it again, but it looks relatively good.

::: dom/bindings/Makefile.in
@@ +86,5 @@
> +                            $(srcdir)/GenerateCSSDeclarationWebIDL.py \
> +                            $(GLOBAL_DEPS)
> +	$(CPP) -I$(topsrcdir)/layout/style $(webidl_base)/CSSStyleDeclarationProps.h | \
> +	  PYTHONDONTWRITEBYTECODE=1 $(PYTHON) $(topsrcdir)/config/pythonpath.py \
> +	  $(srcdir)/GenerateCSSDeclarationWebIDL.py $(webidl_base)/CSSStyleDeclaration.webidl > CSSStyleDeclaration.webidl

Why do you need pythonpath.py here at all?  Is that cargo-culting run amok?

@@ +90,5 @@
> +	  $(srcdir)/GenerateCSSDeclarationWebIDL.py $(webidl_base)/CSSStyleDeclaration.webidl > CSSStyleDeclaration.webidl
> +
> +
> +$(webidl_files): %: $(webidl_base)/%
> +	$(INSTALL) $(IFLAGS1) $(webidl_base)/$* .

Would this still be necessary if we renamed CSSStyleDeclaration.webidl?

@@ +141,5 @@
>    $(binding_cpp_files) \
>    $(globalgen_targets) \
>    ParserResults.pkl \
>    webidlyacc.py \
>    parser.out \

Please add the generated webidl files to GARBAGE.

::: dom/webidl/CSSStyleDeclaration.webidl
@@ +1,1 @@
> +interface CSSRule;

Can we name this CSSStyleDeclaration.webidl.in?  Both to make it clear that it's preprocessed, and to avoid any weirdness that happens with two CSSStyleDeclaration.webidl files on the VPATH.
Attachment #623912 - Flags: review?(khuey) → review-
> Is that cargo-culting run amok?

Absolutely.  I have no idea what all that gunk means; I just copied an existing line that does what I want.  Want me to remove that bit?

> Would this still be necessary if we renamed CSSStyleDeclaration.webidl?

To keep other things simple, yes.  The alternative would be to modify the codegen invocations to handle WebIDL files that live in different directories, with different prefixes, etc.  That would be needed because the CSSStyleDeclaration.webidl would live in the objdir, of course, while the rest live in the srcdir (and the test webidl lives in the test dir).

> Please add the generated webidl files to GARBAGE.

Will do.

> Can we name this CSSStyleDeclaration.webidl.in? 

Yes. Will do.
(In reply to Boris Zbarsky (:bz) from comment #18)
> > Is that cargo-culting run amok?
> 
> Absolutely.  I have no idea what all that gunk means; I just copied an
> existing line that does what I want.  Want me to remove that bit?

Yes please.  I'm 99.999% sure you don't need it.

> > Would this still be necessary if we renamed CSSStyleDeclaration.webidl?
> 
> To keep other things simple, yes.  The alternative would be to modify the
> codegen invocations to handle WebIDL files that live in different
> directories, with different prefixes, etc.  That would be needed because the
> CSSStyleDeclaration.webidl would live in the objdir, of course, while the
> rest live in the srcdir (and the test webidl lives in the test dir).

Ok.
Note that some of the stuff in this patch has migrated to the patch in bug 755080.
Depends on: 755080
> Please add the generated webidl files to GARBAGE.

In particular, bug 755080 handles this (adds $(all_webidl_files) to GARBAGE).
Created attachment 625707 [details] [diff] [review]
Part 4. Set up auto-generation of CSSStyleDeclaration.webidl from nsCSSPropList.h and enable Paris bindings for CSSStyleDeclaration. Updated to Kyle's review comments.
Attachment #625707 - Flags: review?(peterv)
Attachment #625707 - Flags: review?(khuey)
Attachment #625707 - Flags: review?(dbaron)
Attachment #623912 - Attachment is obsolete: true
Attachment #623912 - Flags: review?(peterv)
Attachment #623912 - Flags: review?(dbaron)
Comment on attachment 625707 [details] [diff] [review]
Part 4. Set up auto-generation of CSSStyleDeclaration.webidl from nsCSSPropList.h and enable Paris bindings for CSSStyleDeclaration. Updated to Kyle's review comments.

Review of attachment 625707 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Bindings.conf
@@ +68,5 @@
> +  'nativeType': 'nsDOMCSSDeclaration',
> +  'prefable': True,
> +  'castable': True,
> +  # All the property getters are infallible, but listing them here
> +  # would be nuts.

Long term we probably want the ability to specify "all are infallible except foo, bar".
Attachment #625707 - Flags: review?(khuey) → review+
Nah, long term (think "days") we want to move infallibility annotations into IDL, and then I can just autogenerate them here.  ;)
Attachment #625707 - Flags: review?(peterv) → review+
Comment on attachment 625707 [details] [diff] [review]
Part 4. Set up auto-generation of CSSStyleDeclaration.webidl from nsCSSPropList.h and enable Paris bindings for CSSStyleDeclaration. Updated to Kyle's review comments.

Review of attachment 625707 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/CSSStyleDeclaration.webidl.in
@@ +12,5 @@
> +  CSSValue getPropertyCSSValue(DOMString property);
> +  DOMString getPropertyPriority(DOMString property);
> +  // XXXbz we need to either support default values and set priority
> +  // to "" or we need to set undefined to empty, or we need to make it
> +  // nullable or something.

Is there a bug on string default values?
> Is there a bug on string default values?

Filed bug 759622.  Will put the bug number in the comment.
And yes, that code is wrong as written; we should make the prop nullable for now, I guess.  I'll do that.
Though actually, with the new Optional handling it can just work... I guess I should update to that.
Attachment #623908 - Flags: review?(peterv)
Attachment #623908 - Flags: review?(dbaron)
Created attachment 628206 [details] [diff] [review]
Part 3 updated to use Optional<nsAString> for SetProperty
Attachment #628206 - Flags: review?(dbaron)
Attachment #623910 - Attachment is obsolete: true
Attachment #623910 - Flags: review?(dbaron)
Comment on attachment 628206 [details] [diff] [review]
Part 3 updated to use Optional<nsAString> for SetProperty

Review of attachment 628206 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsDOMCSSDeclaration.h
@@ +77,5 @@
> +    GetCssText(static_cast<nsAString&>(aString));
> +  }
> +  uint32_t GetLength() {
> +    uint32_t length;
> +    GetLength(&length);

PRUint32 for the local, please, to avoid breaking obscure platforms.
> PRUint32 for the local, please, to avoid breaking obscure platforms.

OK.
Created attachment 635930 [details] [diff] [review]
part 1.  Make GetParentObject() on CSS declarations return a ParentObject struct.
Attachment #635930 - Flags: review?(peterv)
Attachment #635930 - Flags: review?(dbaron)
Attachment #623906 - Attachment is obsolete: true
Created attachment 635943 [details] [diff] [review]
Part 2, with mccr8's comments hopefully applied
Attachment #635943 - Flags: review?(peterv)
Attachment #635943 - Flags: review?(dbaron)
Attachment #635943 - Flags: review?(continuation)
Attachment #635943 - Flags: review?(bugs)
Attachment #623908 - Attachment is obsolete: true
Created attachment 635944 [details] [diff] [review]
part 1.  Make GetParentObject() on CSS declarations return a useful nsINode if possible.
Attachment #635944 - Flags: review?(peterv)
Attachment #635944 - Flags: review?(dbaron)
Attachment #635930 - Attachment is obsolete: true
Attachment #635930 - Flags: review?(peterv)
Attachment #635930 - Flags: review?(dbaron)
Comment on attachment 635943 [details] [diff] [review]
Part 2, with mccr8's comments hopefully applied

Review of attachment 635943 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.  (I only reviewed the CAN_SKIP stuff in the two files.)

::: layout/style/nsDOMCSSAttrDeclaration.cpp
@@ +42,2 @@
>  
> +// If mElement holds a strong ref to us, so if it's going to be

Nit: This comment looks slightly mangled. I think you can drop the "If" in "If mElement".
Attachment #635943 - Flags: review?(continuation) → review+
Created attachment 635999 [details] [diff] [review]
part 3.  Expose the API needed for Paris bindings on nsDOMCSSDeclaration.
Attachment #635999 - Flags: review?(peterv)
Attachment #635999 - Flags: review?(dbaron)
Attachment #628206 - Attachment is obsolete: true
Attachment #628206 - Flags: review?(dbaron)

Updated

5 years ago
Attachment #625707 - Attachment description: Updated to Kyle's review comments → Part 4. Set up auto-generation of CSSStyleDeclaration.webidl from nsCSSPropList.h and enable Paris bindings for CSSStyleDeclaration. Updated to Kyle's review comments.

Updated

5 years ago
Attachment #635943 - Flags: review?(bugs) → review+

Comment 37

5 years ago
+cc: dholbert. You may need to catch these nsCSSPropList.h changes prior to landing flexbox in m-c
There's nothing to catch.  I'm just using nsCSSPropList.h as-is at build time to generate the WebIDL.  So as long as flexbox gets added there (which it needs to be to parse), it'll all just work and get exposed in the IDL automatically.
nsCSSFontFaceStyleDecl is a nsIDOMCSSStyleDeclaration but not a nsDOMCSSDeclaration, it hits the "All CSS declarations must be wrappercached" assertion (and doesn't use the new binding).
Relevant: bug 765588.
Hmm.  I didn't realize we were using the same classinfo for it.  I'll undo that change.

But that's a problem, because the proto for that object should be CSSStyleDeclaration.prototype as things stand, right?

I'm tempted to create a separate nsIDOMFontFaceDeclaration interface for that object, or something.  The current setup is just broken if we want to use the existing CSSStyleDeclaration WebIDL, since most of it makes no sense for the font-face declarations.  David, thoughts?
OK, I just talked to dbaron about this.

The plan is to do the following:

1)  Split out, in WebIDL, CSSStyleDeclaration and CSS2Properties.
2)  Map CSSStyleDeclaration to nsICSSDeclaration and CSS2Properties to nsDOMCSSDeclaration.
3)  Make font-face declarations wrappercached and so forth.
3)  Make font-face rules implement CSSStyleDeclaration but not CSS2Properties.
4)  Make the classinfo deal with all this as needed.
Depends on: 765588
Created attachment 637277 [details] [diff] [review]
Part 1 now including  (GetParentObject)font-face rules
Attachment #637277 - Flags: review?(peterv)
Attachment #637277 - Flags: review?(dbaron)
Attachment #635944 - Attachment is obsolete: true
Attachment #635944 - Flags: review?(peterv)
Attachment #635944 - Flags: review?(dbaron)
Created attachment 637329 [details] [diff] [review]
Part 2 (adding wrappercache to all CSS declarations) now including font-face declarations
Attachment #637329 - Flags: review?(peterv)
Attachment #637329 - Flags: review?(dbaron)
Attachment #635943 - Attachment is obsolete: true
Attachment #635943 - Flags: review?(peterv)
Attachment #635943 - Flags: review?(dbaron)
Created attachment 637350 [details] [diff] [review]
Part 3 (WebIDL binding API) updated to deal with font-face declarations
Attachment #637350 - Flags: review?(peterv)
Attachment #637350 - Flags: review?(dbaron)
Attachment #635999 - Attachment is obsolete: true
Attachment #635999 - Flags: review?(peterv)
Attachment #635999 - Flags: review?(dbaron)
Created attachment 637353 [details] [diff] [review]
Part 4 (enable the new bindings) updated for font-face declarations
Attachment #637353 - Flags: review?(peterv)
Attachment #637353 - Flags: review?(dbaron)
Attachment #625707 - Attachment is obsolete: true
Attachment #625707 - Flags: review?(dbaron)

Updated

5 years ago
blocking-kilimanjaro: --- → ?
Comment on attachment 637277 [details] [diff] [review]
Part 1 now including  (GetParentObject)font-face rules

r=dbaron on all but the nsDOMClassInfo.cpp parts
Attachment #637277 - Flags: review?(dbaron) → review+
Comment on attachment 637329 [details] [diff] [review]
Part 2 (adding wrappercache to all CSS declarations) now including font-face declarations

  r=dbaron, though this also requires somebody more familiar with
  the cycle collector and wrapper cache to review
Attachment #637329 - Flags: review?(dbaron) → review+
Comment on attachment 637350 [details] [diff] [review]
Part 3 (WebIDL binding API) updated to deal with font-face declarations

  Hmmm.  I feel like there ought to be a better way than to add
  an inline method for each property, but r=dbaron.

>+  void GetCssText(nsString& aString) {
>+    GetCssText(static_cast<nsAString&>(aString));
>+  }

  Why is this cast needed?
  (Same in Item, GetPropertyPriority.)

(as bz already explained to me when I emailed him the review comments from an airplane earlier, it's needed for overload resolution)

  So maybe it's worth adding a comment saying that the cast is for overload resolution?
Attachment #637350 - Flags: review?(dbaron) → review+
Comment on attachment 637353 [details] [diff] [review]
Part 4 (enable the new bindings) updated for font-face declarations

  >+  # All the property getters are infallible, but listing them here
  >+  # would be nuts.

  Maybe at least a FIXME, so that we know it's something that could
  be fixed later given a better mechanism in the *.conf files?

  r=dbaron on the parts I understand, but peterv or somebody
  familiar with the bindings code should also review
Attachment #637353 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] from comment #50)
> Comment on attachment 637353 [details] [diff] [review]
> Part 4 (enable the new bindings) updated for font-face declarations
> 
>   >+  # All the property getters are infallible, but listing them here
>   >+  # would be nuts.
> 
>   Maybe at least a FIXME, so that we know it's something that could
>   be fixed later given a better mechanism in the *.conf files?

(Now that the annotations live in the IDL files (bug 757164), this actually shouldn't be too hard.)
Attachment #637277 - Flags: review?(peterv) → review+
Comment on attachment 637329 [details] [diff] [review]
Part 2 (adding wrappercache to all CSS declarations) now including font-face declarations

Review of attachment 637329 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/StyleRule.cpp
@@ +983,5 @@
>    NS_IMETHOD_(nsrefcnt) AddRef(void);
>    NS_IMETHOD_(nsrefcnt) Release(void);
>  
> +  // We need to forward QI for cycle collection things to DOMCSSStyleRule
> +  NS_IMETHOD QueryInterface(REFNSIID aIID, void** aInstancePtr);

Maybe switch to NS_DECL_ISUPPORTS_INHERITED (it's not completely right, but it looks like that's what we use for aggregation, see also your change in nsCSSFontFaceStyleDecl).

@@ +1160,5 @@
>  {
>  }
>  
>  NS_INTERFACE_MAP_BEGIN(DOMCSSStyleRule)
> +  NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(DOMCSSStyleRule)

Use NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION.

::: layout/style/nsCSSRules.cpp
@@ +1643,5 @@
>  
>  DOMCI_DATA(CSSFontFaceRule, nsCSSFontFaceRule)
>  
>  // QueryInterface implementation for nsCSSFontFaceRule
>  NS_INTERFACE_MAP_BEGIN(nsCSSFontFaceRule)

You need to change this to NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION.

::: layout/style/nsCSSRules.h
@@ +305,5 @@
>    virtual nsIDocument* DocToUpdate();
>  
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_AMBIGUOUS(nsCSSKeyframeStyleDeclaration,
> +                                                         nsICSSDeclaration)

I'm a bit worried that nsCSSKeyframeRule isn't hooked up to the CC. AFAICT it holds a strong ref to the nsCSSKeyframeStyleDeclaration until it's destroyed. It seems like it'd be trivial to make that leak (put the rule as a property on the declaration). It's probably a rare problem, so I'm ok with doing it in a follow-up.
Attachment #637329 - Flags: review?(peterv) → review+
Added comments as dbaron asked.  I can't make the getters infallible because on nsComputedDOMStyle they're actually fallible.  :(

Made the changes to NS_DECL_ISUPPORTS_INHERITED and NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION.  Peter, good catch on nsCSSFontFaceRule!

Filed bug 774980 on hooking up nsCSSKeyframeRule to cycle collection.
Blocks: 774980
Blocks: 777385

Updated

5 years ago
Blocks: 777877
No longer blocks: 777877
I landed the two first patches (the reviewed ones), because bug 780060 is depending on them:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/4a98866591a0
  https://hg.mozilla.org/integration/mozilla-inbound/rev/61951a8d507f
Whiteboard: [leave open]
Blocks: 780060
Turns out part 2 actually depends on nsWrappeCacheInlines being included, which was happening indirectly in part 3.  Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/4bc7154190dd to include it explicitly.
And http://hg.mozilla.org/integration/mozilla-inbound/rev/2ed51e5b476a to fix the broken QI impls involved.
https://hg.mozilla.org/mozilla-central/rev/4a98866591a0
https://hg.mozilla.org/mozilla-central/rev/61951a8d507f
https://hg.mozilla.org/mozilla-central/rev/4bc7154190dd
https://hg.mozilla.org/mozilla-central/rev/2ed51e5b476a
Created attachment 654744 [details] [diff] [review]
Part 3b (WebIDL binding API - add indexed getter API)
Attachment #654744 - Flags: review?(bzbarsky)
Comment on attachment 637350 [details] [diff] [review]
Part 3 (WebIDL binding API) updated to deal with font-face declarations

Review of attachment 637350 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsICSSDeclaration.h
@@ +74,5 @@
> +                         const nsAString& aValue,
> +                         const nsAString& aPriority) = 0;
> +  NS_IMETHOD GetLength(PRUint32* aLength) = 0;
> +  NS_IMETHOD Item(PRUint32 aIndex, nsAString& aReturn) = 0;
> +  NS_IMETHOD GetParentRule(nsIDOMCSSRule * *aParentRule) = 0;  

Trailing whitespace.

@@ +123,5 @@
> +    nsCOMPtr<nsIDOMCSSRule> rule;
> +    GetParentRule(getter_AddRefs(rule));
> +    return rule.forget();
> +  }
> +  

Trailing whitespace.
Attachment #637350 - Flags: review?(peterv) → review+
Created attachment 654752 [details] [diff] [review]
Part 4b (enable the new bindings - add getter and move infallability to WebIDL)
Attachment #654752 - Flags: review?(bzbarsky)
Created attachment 654753 [details] [diff] [review]
Part 4c (enable the new bindings - fix test failures)
Attachment #654753 - Flags: review?(bzbarsky)
Attachment #654753 - Attachment description: v4c (enable the new bindings - fix test failures) → Part 4c (enable the new bindings - fix test failures)
Comment on attachment 654744 [details] [diff] [review]
Part 3b (WebIDL binding API - add indexed getter API)

>+++ b/layout/style/nsDOMCSSDeclaration.cpp
>-  if (decl) {
>-    decl->GetNthProperty(aIndex, aReturn);

This is the only consumer of GetNthProperty.  So just change that to return a bool for whether it does something (and document that in the header), and here we can do:

  aFound = decl && decl->GetNthProperty(aIndex, aPropName);

>+++ b/layout/style/nsICSSDeclaration.h
>   void Item(uint32_t aIndex, nsString& aPropName) {
>-    Item(aIndex, static_cast<nsAString&>(aPropName));

I don't think we need to make this change.

>+  void IndexedGetter(uint32_t aIndex, bool& aFound, nsString& aPropName)
>+  {
>+    IndexedGetter(aIndex, aFound, static_cast<nsAString&>(aPropName));
>+  }

And then we don't need this bit either.

r=me with those fixed.
Attachment #654744 - Flags: review?(bzbarsky) → review+
Attachment #637277 - Flags: checkin+
Attachment #637329 - Flags: checkin+
Comment on attachment 654752 [details] [diff] [review]
Part 4b (enable the new bindings - add getter and move infallability to WebIDL)

r=me
Attachment #654752 - Flags: review?(bzbarsky) → review+
Comment on attachment 654753 [details] [diff] [review]
Part 4c (enable the new bindings - fix test failures)

r=me
Attachment #654753 - Flags: review?(bzbarsky) → review+
Depends on: 742195
Part 4 depends on bug 742195 and bug 785188.
Depends on: 785188
Created attachment 654799 [details] [diff] [review]
Part 3 updated with Peter's bits
Attachment #637350 - Attachment is obsolete: true
Attachment #637353 - Flags: review?(peterv) → review+
Created attachment 654930 [details] [diff] [review]
Part 4 with Peter's bits
Attachment #637353 - Attachment is obsolete: true
Attachment #654744 - Attachment is obsolete: true
Attachment #654752 - Attachment is obsolete: true
Attachment #654753 - Attachment is obsolete: true
Created attachment 655086 [details] [diff] [review]
Part 5 (make the IC work with the new binding)

Got r=bz in person.
Attachment #655086 - Flags: review+
   https://hg.mozilla.org/integration/mozilla-inbound/rev/75f3cd90e743
   https://hg.mozilla.org/integration/mozilla-inbound/rev/c526d9dfb684
   https://hg.mozilla.org/integration/mozilla-inbound/rev/dc3f29a730b1
https://hg.mozilla.org/mozilla-central/rev/75f3cd90e743
https://hg.mozilla.org/mozilla-central/rev/c526d9dfb684
https://hg.mozilla.org/mozilla-central/rev/dc3f29a730b1
Depends on: 785981
Depends on: 786105
This is fixed.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla17
Note: patches 3/4/5 regressed the Number of Constructors benchmark on Linux by +3.
(In reply to Matt Brubeck (:mbrubeck) from comment #72)
> Note: patches 3/4/5 regressed the Number of Constructors benchmark on Linux
> by +3.

Ah, this is just bug 774757.

Updated

5 years ago
Blocks: 795221

Updated

5 years ago
No longer blocks: 795221
Depends on: 795221
Blocks: 698072

Updated

5 years ago
Depends on: 830260
Blocks: 777877
Blocks: 876725
You need to log in before you can comment on or make changes to this bug.