Closed
Bug 753517
Opened 12 years ago
Closed 12 years ago
Convert CSSStyleDeclaration to Paris bindings
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(7 files, 16 obsolete files)
1.04 KB,
text/html
|
Details | |
962 bytes,
text/html
|
Details | |
5.86 KB,
patch
|
peterv
:
review+
dbaron
:
review+
bzbarsky
:
checkin+
|
Details | Diff | Splinter Review |
20.48 KB,
patch
|
peterv
:
review+
dbaron
:
review+
bzbarsky
:
checkin+
|
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.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #622491 -
Flags: review?(peterv)
Assignee | ||
Comment 2•12 years ago
|
||
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)
Assignee | ||
Comment 3•12 years ago
|
||
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? :(
Assignee | ||
Comment 4•12 years ago
|
||
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.
Assignee | ||
Comment 5•12 years ago
|
||
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.
Assignee | ||
Comment 6•12 years ago
|
||
Assignee | ||
Comment 7•12 years ago
|
||
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)
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #623910 -
Flags: review?(dbaron)
Assignee | ||
Comment 9•12 years ago
|
||
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)
Comment 10•12 years ago
|
||
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.
Assignee | ||
Comment 11•12 years ago
|
||
They;re still multiply inherited from nsISupports, which is the part that matters here.
Comment 12•12 years ago
|
||
Ah, I see, I thought it said nsCSSDeclaration not nsICSSDeclaration. That makes more sense...
Comment 13•12 years ago
|
||
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+
Assignee | ||
Comment 14•12 years ago
|
||
> 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 15•12 years ago
|
||
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-
Comment 16•12 years ago
|
||
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-
Assignee | ||
Comment 18•12 years ago
|
||
> 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.
Assignee | ||
Comment 20•12 years ago
|
||
Note that some of the stuff in this patch has migrated to the patch in bug 755080.
Depends on: 755080
Assignee | ||
Comment 21•12 years ago
|
||
> Please add the generated webidl files to GARBAGE. In particular, bug 755080 handles this (adds $(all_webidl_files) to GARBAGE).
Assignee | ||
Comment 22•12 years ago
|
||
Attachment #625707 -
Flags: review?(peterv)
Attachment #625707 -
Flags: review?(khuey)
Attachment #625707 -
Flags: review?(dbaron)
Assignee | ||
Updated•12 years ago
|
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+
Assignee | ||
Comment 24•12 years ago
|
||
Nah, long term (think "days") we want to move infallibility annotations into IDL, and then I can just autogenerate them here. ;)
Updated•12 years ago
|
Attachment #625707 -
Flags: review?(peterv) → review+
Comment 25•12 years ago
|
||
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?
Assignee | ||
Comment 26•12 years ago
|
||
> Is there a bug on string default values? Filed bug 759622. Will put the bug number in the comment.
Assignee | ||
Comment 27•12 years ago
|
||
And yes, that code is wrong as written; we should make the prop nullable for now, I guess. I'll do that.
Assignee | ||
Comment 28•12 years ago
|
||
Though actually, with the new Optional handling it can just work... I guess I should update to that.
Assignee | ||
Updated•12 years ago
|
Attachment #623908 -
Flags: review?(peterv)
Attachment #623908 -
Flags: review?(dbaron)
Assignee | ||
Comment 29•12 years ago
|
||
Attachment #628206 -
Flags: review?(dbaron)
Assignee | ||
Updated•12 years ago
|
Attachment #623910 -
Attachment is obsolete: true
Attachment #623910 -
Flags: review?(dbaron)
Comment 30•12 years ago
|
||
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.
Assignee | ||
Comment 31•12 years ago
|
||
> PRUint32 for the local, please, to avoid breaking obscure platforms.
OK.
Assignee | ||
Comment 32•12 years ago
|
||
Attachment #635930 -
Flags: review?(peterv)
Attachment #635930 -
Flags: review?(dbaron)
Assignee | ||
Updated•12 years ago
|
Attachment #623906 -
Attachment is obsolete: true
Assignee | ||
Comment 33•12 years ago
|
||
Attachment #635943 -
Flags: review?(peterv)
Attachment #635943 -
Flags: review?(dbaron)
Attachment #635943 -
Flags: review?(continuation)
Attachment #635943 -
Flags: review?(bugs)
Assignee | ||
Updated•12 years ago
|
Attachment #623908 -
Attachment is obsolete: true
Assignee | ||
Comment 34•12 years ago
|
||
Attachment #635944 -
Flags: review?(peterv)
Attachment #635944 -
Flags: review?(dbaron)
Assignee | ||
Updated•12 years ago
|
Attachment #635930 -
Attachment is obsolete: true
Attachment #635930 -
Flags: review?(peterv)
Attachment #635930 -
Flags: review?(dbaron)
Comment 35•12 years ago
|
||
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+
Assignee | ||
Comment 36•12 years ago
|
||
Attachment #635999 -
Flags: review?(peterv)
Attachment #635999 -
Flags: review?(dbaron)
Assignee | ||
Updated•12 years ago
|
Attachment #628206 -
Attachment is obsolete: true
Attachment #628206 -
Flags: review?(dbaron)
Updated•12 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•12 years ago
|
Attachment #635943 -
Flags: review?(bugs) → review+
Comment 37•12 years ago
|
||
+cc: dholbert. You may need to catch these nsCSSPropList.h changes prior to landing flexbox in m-c
Assignee | ||
Comment 38•12 years ago
|
||
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.
Comment 39•12 years ago
|
||
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).
Comment 40•12 years ago
|
||
Relevant: bug 765588.
Assignee | ||
Comment 41•12 years ago
|
||
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?
Assignee | ||
Comment 42•12 years ago
|
||
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.
Assignee | ||
Comment 43•12 years ago
|
||
Attachment #637277 -
Flags: review?(peterv)
Attachment #637277 -
Flags: review?(dbaron)
Assignee | ||
Updated•12 years ago
|
Attachment #635944 -
Attachment is obsolete: true
Attachment #635944 -
Flags: review?(peterv)
Attachment #635944 -
Flags: review?(dbaron)
Assignee | ||
Comment 44•12 years ago
|
||
Attachment #637329 -
Flags: review?(peterv)
Attachment #637329 -
Flags: review?(dbaron)
Assignee | ||
Updated•12 years ago
|
Attachment #635943 -
Attachment is obsolete: true
Attachment #635943 -
Flags: review?(peterv)
Attachment #635943 -
Flags: review?(dbaron)
Assignee | ||
Comment 45•12 years ago
|
||
Attachment #637350 -
Flags: review?(peterv)
Attachment #637350 -
Flags: review?(dbaron)
Assignee | ||
Updated•12 years ago
|
Attachment #635999 -
Attachment is obsolete: true
Attachment #635999 -
Flags: review?(peterv)
Attachment #635999 -
Flags: review?(dbaron)
Assignee | ||
Comment 46•12 years ago
|
||
Attachment #637353 -
Flags: review?(peterv)
Attachment #637353 -
Flags: review?(dbaron)
Assignee | ||
Updated•12 years ago
|
Attachment #625707 -
Attachment is obsolete: true
Attachment #625707 -
Flags: review?(dbaron)
Updated•12 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+
Comment 51•12 years ago
|
||
(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.)
Updated•12 years ago
|
Attachment #637277 -
Flags: review?(peterv) → review+
Comment 52•12 years ago
|
||
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+
Assignee | ||
Comment 53•12 years ago
|
||
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.
Assignee | ||
Comment 54•12 years ago
|
||
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]
Assignee | ||
Comment 55•12 years ago
|
||
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.
Assignee | ||
Comment 56•12 years ago
|
||
And http://hg.mozilla.org/integration/mozilla-inbound/rev/2ed51e5b476a to fix the broken QI impls involved.
Comment 57•12 years ago
|
||
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
Comment 58•12 years ago
|
||
Attachment #654744 -
Flags: review?(bzbarsky)
Comment 59•12 years ago
|
||
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+
Comment 60•12 years ago
|
||
Attachment #654752 -
Flags: review?(bzbarsky)
Comment 61•12 years ago
|
||
Attachment #654753 -
Flags: review?(bzbarsky)
Updated•12 years ago
|
Attachment #654753 -
Attachment description: v4c (enable the new bindings - fix test failures) → Part 4c (enable the new bindings - fix test failures)
Assignee | ||
Comment 62•12 years ago
|
||
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+
Assignee | ||
Updated•12 years ago
|
Attachment #637277 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #637329 -
Flags: checkin+
Assignee | ||
Comment 63•12 years ago
|
||
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+
Assignee | ||
Comment 64•12 years ago
|
||
Comment on attachment 654753 [details] [diff] [review] Part 4c (enable the new bindings - fix test failures) r=me
Attachment #654753 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 66•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #637350 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #637353 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 67•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #637353 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #654744 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #654752 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #654753 -
Attachment is obsolete: true
Assignee | ||
Comment 69•12 years ago
|
||
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
Comment 70•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/75f3cd90e743 https://hg.mozilla.org/mozilla-central/rev/c526d9dfb684 https://hg.mozilla.org/mozilla-central/rev/dc3f29a730b1
Assignee | ||
Comment 71•12 years ago
|
||
This is fixed.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla17
Comment 72•12 years ago
|
||
Note: patches 3/4/5 regressed the Number of Constructors benchmark on Linux by +3.
Comment 73•12 years ago
|
||
(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•12 years ago
|
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•