Comment on attachment 720479 [details] [diff] [review] SVGFEMergeElement Review of attachment 720479 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/svg/content/src/nsSVGFilters.cpp @@ +30,5 @@ > > +NS_INTERFACE_TABLE_HEAD(SVGFEMergeElement) > + NS_NODE_INTERFACE_TABLE3(SVGFEMergeElement, nsIDOMNode, nsIDOMElement, > + nsIDOMSVGElement) > +NS_INTERFACE_MAP_END_INHERITING(SVGFEMergeElementBase) NS_IMPL_ISUPPORTS_INHERITED3 @@ +30,5 @@ > > +NS_INTERFACE_TABLE_HEAD(SVGFEMergeElement) > + NS_NODE_INTERFACE_TABLE3(SVGFEMergeElement, nsIDOMNode, nsIDOMElement, > + nsIDOMSVGElement) > +NS_INTERFACE_MAP_END_INHERITING(SVGFEMergeElementBase) {}s get their own line for the constructor

Comment on attachment 720485 [details] [diff] [review] SVGFEFloodElement Review of attachment 720485 [details] [diff] [review]: ----------------------------------------------------------------- Same comments. For future reference, it would be nice to have one patch that *just* splits out the classes into their own file, and one that does the renaming/WebIDL stuff. ::: content/svg/content/src/nsSVGFilters.cpp @@ -2205,5 @@ > -public: > - virtual bool SubregionIsUnionOfRegions() { return false; } > - > - // interfaces: > - NS_DECL_ISUPPORTS_INHERITED Did you lose virtual bool SubregionIsUnionOfRegions() { return false; } ?

I think extending IsNodeOfType so that we have an eFilter (c.f. eAnimation) would be better than having a separate IsFilterElement method.

Using IsNodeOfType.

Comment on attachment 721586 [details] [diff] [review] Move SVGFEImageElement Review of attachment 721586 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this was a *lot* easier to review.

Comment on attachment 721587 [details] [diff] [review] WebIDL API for FEImage Review of attachment 721587 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/svg/content/src/SVGFEImageElement.cpp @@ +15,5 @@ > namespace mozilla { > namespace dom { > > +JSObject* > +SVGFEImageElement::WrapNode(JSContext *aCx, JSObject *aScope, bool *aTriedToWrap) * to the left @@ +40,3 @@ > imgINotificationObserver, nsIImageLoadingContent, > imgIOnloadBlocker) > +NS_INTERFACE_MAP_END_INHERITING(SVGFEImageElementBase) NS_IMPL_ISUPPORTS_INHERITED7 @@ +284,5 @@ > +SVGFEImageElement::PreserveAspectRatio() > +{ > + nsRefPtr<DOMSVGAnimatedPreserveAspectRatio> ratio; > + mPreserveAspectRatio.ToDOMAnimatedPreserveAspectRatio(getter_AddRefs(ratio), this); > + return ratio.forget(); I'd r+ a better signature here

Comment on attachment 721012 [details] [diff] [review] Stop QIing to nsIDOMSVGFilterPrimitiveStandardAttributes r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/64c624853e92 https://hg.mozilla.org/integration/mozilla-inbound/rev/3710f0b135b8

Wrong bug number in commit message, so backed out and relanded the FEBlend patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/9b7c6a46f1f6

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d3ab7dd1095 https://hg.mozilla.org/integration/mozilla-inbound/rev/96357b85842e https://hg.mozilla.org/integration/mozilla-inbound/rev/808bbc2bc424 https://hg.mozilla.org/integration/mozilla-inbound/rev/cbf9f569d197

https://hg.mozilla.org/mozilla-central/rev/64c624853e92 https://hg.mozilla.org/mozilla-central/rev/5d3ab7dd1095 https://hg.mozilla.org/mozilla-central/rev/96357b85842e https://hg.mozilla.org/mozilla-central/rev/808bbc2bc424 https://hg.mozilla.org/mozilla-central/rev/cbf9f569d197

Comment on attachment 721588 [details] [diff] [review] Remove nsIDOMSVGURIReference Review of attachment 721588 [details] [diff] [review]: ----------------------------------------------------------------- Good riddance, but I'd like an SVG kind of guy to sign off on it :)

Comment on attachment 721593 [details] [diff] [review] WebIDL API for FETile Review of attachment 721593 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/svg/content/src/SVGFETileElement.cpp @@ +13,5 @@ > namespace mozilla { > namespace dom { > > +JSObject* > +SVGFETileElement::WrapNode(JSContext *aCx, JSObject *aScope, bool *aTriedToWrap) * to the left

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/cf778a07299d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d39b5acb8d83

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/72952b8e3e81 because Windows builds were doing the horrible "Error while running startup cache precompilation" that usually means a crash on startup, no, you can't be told where it crashed, but also retriggered after a clobber, because I always suspect a need to clobber these days. If your second builds are fine, then just reland with yet another clobber set, and touch /CLOBBER so people will know to clobber their builds, too.

Relanded with a clobber https://hg.mozilla.org/integration/mozilla-inbound/rev/885607afe05c https://hg.mozilla.org/integration/mozilla-inbound/rev/243f097d4ba5

Comment on attachment 721588 [details] [diff] [review] Remove nsIDOMSVGURIReference I'm not a superreviewer, but here's f=jwatt instead.

https://hg.mozilla.org/mozilla-central/rev/885607afe05c https://hg.mozilla.org/mozilla-central/rev/243f097d4ba5

Comment on attachment 726236 [details] [diff] [review] WebIDL ColorMatrix Review of attachment 726236 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/svg/content/src/SVGFEColorMatrixElement.cpp @@ +56,3 @@ > > +NS_INTERFACE_TABLE_HEAD(SVGFEColorMatrixElement) > + NS_NODE_INTERFACE_TABLE3(SVGFEColorMatrixElement, nsIDOMNode, nsIDOMElement, For nodes that use the new bindings, you don't need the _NODE_ part anymore. Just use the NS_IMPL_NSISUPPORTS_* macro instead? @@ +86,3 @@ > { > + return DOMSVGAnimatedNumberList::GetDOMWrapper(&mNumberListAttributes[VALUES], > + this, VALUES).get(); Don't need the .get() @@ +94,5 @@ > aSources.AppendElement(nsSVGStringInfo(&mStringAttributes[IN1], this)); > } > > nsresult > +SVGFEColorMatrixElement::Filter(nsSVGFilterInstance *instance, * to the left ::: content/svg/content/src/SVGFEColorMatrixElement.h @@ +9,5 @@ > +#include "nsSVGEnum.h" > +#include "nsSVGFilters.h" > +#include "SVGAnimatedNumberList.h" > + > +typedef nsSVGFE SVGFEColorMatrixElementBase; Move the typedef into the namespace if you drop the 'ns'?

Comment on attachment 726238 [details] [diff] [review] WebIDL FEComposite Review of attachment 726238 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/svg/content/src/SVGFECompositeElement.cpp @@ +58,4 @@ > > +NS_INTERFACE_TABLE_HEAD(SVGFECompositeElement) > + NS_NODE_INTERFACE_TABLE3(SVGFECompositeElement, nsIDOMNode, nsIDOMElement, > + nsIDOMSVGElement) Same @@ +121,4 @@ > } > > nsresult > +SVGFECompositeElement::Filter(nsSVGFilterInstance *instance, * to the left

(In reply to :Ms2ger from comment #33) > Comment on attachment 726236 [details] [diff] [review] > WebIDL ColorMatrix > > Review of attachment 726236 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/svg/content/src/SVGFEColorMatrixElement.cpp > @@ +56,3 @@ > > > > +NS_INTERFACE_TABLE_HEAD(SVGFEColorMatrixElement) > > + NS_NODE_INTERFACE_TABLE3(SVGFEColorMatrixElement, nsIDOMNode, nsIDOMElement, > > For nodes that use the new bindings, you don't need the _NODE_ part anymore. > Just use the NS_IMPL_NSISUPPORTS_* macro instead? > Actually there's no point in even having this macro or declaring nsISupports. When I finish moving all the filter elements I'm going to make nsSVGElement implement nsIDOMSVGElement and stop declaring nsISupports in all the subclasses, so I'll just change this then.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1de3ed642b6d https://hg.mozilla.org/integration/mozilla-inbound/rev/1a4518e6fdb8 https://hg.mozilla.org/integration/mozilla-inbound/rev/8b5889e318d6 https://hg.mozilla.org/integration/mozilla-inbound/rev/66362cdd8519

Comment on attachment 726479 [details] [diff] [review] Convert FEGaussianBlur Patch is empty.

https://hg.mozilla.org/mozilla-central/rev/1de3ed642b6d https://hg.mozilla.org/mozilla-central/rev/1a4518e6fdb8 https://hg.mozilla.org/mozilla-central/rev/8b5889e318d6 https://hg.mozilla.org/mozilla-central/rev/66362cdd8519

Weird that the other one was empty...

Comment on attachment 726751 [details] [diff] [review] Convert FEGaussianBlur Review of attachment 726751 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/svg/content/src/SVGFEGaussianBlurElement.cpp @@ +241,5 @@ > } > > void > +SVGFEGaussianBlurElement::GaussianBlur(const Image *aSource, > + const Image *aTarget, * to the left

https://hg.mozilla.org/integration/mozilla-inbound/rev/e23f5058335b https://hg.mozilla.org/integration/mozilla-inbound/rev/f999416ea047

Comment on attachment 727039 [details] [diff] [review] Move FEOffset Review of attachment 727039 [details] [diff] [review]: ----------------------------------------------------------------- Are you missing the Makefile changes here?

https://hg.mozilla.org/mozilla-central/rev/e23f5058335b https://hg.mozilla.org/mozilla-central/rev/f999416ea047

(In reply to :Ms2ger from comment #47) > Comment on attachment 727039 [details] [diff] [review] > Move FEOffset > > Review of attachment 727039 [details] [diff] [review]: > ----------------------------------------------------------------- > > Are you missing the Makefile changes here? Yes, I forgot them. They're in the other patch.

Comment on attachment 727039 [details] [diff] [review] Move FEOffset Review of attachment 727039 [details] [diff] [review]: ----------------------------------------------------------------- r=me if you make the makefile changes in this patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b56cf513f3ce https://hg.mozilla.org/integration/mozilla-inbound/rev/94757fd7a4ca

https://hg.mozilla.org/mozilla-central/rev/b56cf513f3ce https://hg.mozilla.org/mozilla-central/rev/94757fd7a4ca

Comment on attachment 728453 [details] [diff] [review] Convert FESpecularLighting Review of attachment 728453 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/svg/content/src/SVGFESpecularLightingElement.cpp @@ +9,2 @@ > > +NS_IMPL_NS_NEW_NAMESPACED_SVG_ELEMENT(FESpecularLighting) Can you file a followup to make sure we remove the old one at some point? @@ +65,1 @@ > this); Maybe return mNumberPairAttributes[KERNEL_UNIT_LENGTH].ToDOMAnimatedNumber( nsSVGNumberPair::eFirst, this); to avoid the 80-columns limit. @@ +70,2 @@ > { > + return mNumberPairAttributes[KERNEL_UNIT_LENGTH].ToDOMAnimatedNumber(nsSVGNumberPair::eSecond, Same here @@ +75,5 @@ > //---------------------------------------------------------------------- > // nsSVGElement methods > > nsresult > +SVGFESpecularLightingElement::Filter(nsSVGFilterInstance *instance, * to the left ::: content/svg/content/src/nsSVGFilters.cpp @@ -2042,5 @@ > -//------------------------------------------------------------ > - > -typedef nsSVGFE nsSVGFELightingElementBase; > - > -class nsSVGFELightingElement : public nsSVGFELightingElementBase I'm pretty sure this doesn't belong here. ::: content/svg/content/src/nsSVGFilters.h @@ +22,5 @@ > class nsSVGFilterResource; > class nsSVGNumberPair; > > +#define DOT(a,b) (a[0] * b[0] + a[1] * b[1] + a[2] * b[2]) > +#define NORMALIZE(vec) \ Making those two inline functions would make me happy.

Comment on attachment 728469 [details] [diff] [review] Convert FEDiffuseLighting Review of attachment 728469 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/svg/content/src/SVGFEDiffuseLightingElement.cpp @@ +58,2 @@ > { > + return mNumberPairAttributes[KERNEL_UNIT_LENGTH].ToDOMAnimatedNumber(nsSVGNumberPair::eFirst, As before

Comment on attachment 728506 [details] [diff] [review] Move FEDisplacementMap Review of attachment 728506 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/svg/content/src/SVGFEDiffuseLightingElement.cpp @@ +4,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #include "mozilla/dom/SVGFEDiffuseLightingElement.h" > #include "mozilla/dom/SVGFEDiffuseLightingElementBinding.h" > +#include "nsSVGUtils.h" This doesn't belong here.

Comment on attachment 728579 [details] [diff] [review] Convert FEConvolveMatrix Review of attachment 728579 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/svg/content/src/SVGFEConvolveMatrixElement.cpp @@ +120,2 @@ > { > + return DOMSVGAnimatedNumberList::GetDOMWrapper(&mNumberListAttributes[KERNELMATRIX], Hmm, why does this have a different interface than all the others? Maybe file a bug. ::: content/svg/content/src/nsSVGBoolean.cpp @@ +104,5 @@ > aSVGElement->DidAnimateBoolean(mAttrEnum); > } > > +already_AddRefed<SVGAnimatedBoolean> > +nsSVGBoolean::ToDOMAnimatedBoolean(nsSVGElement *aSVGElement) * to the left

Comment on attachment 728583 [details] [diff] [review] Convert FETurbulence Review of attachment 728583 [details] [diff] [review]: ----------------------------------------------------------------- Sometimes I accidentally look at the interface you're dealing with and I wonder if the SVG WG was on crack...

Comment on attachment 728584 [details] [diff] [review] Remove nsIDOMSVGFilterPrimitiveStandardAttributes Review of attachment 728584 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/svg/nsIDOMSVGFilters.idl @@ -1,3 @@ > -/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > -/* This Source Code Form is subject to the terms of the Mozilla Public > - * License, v. 2.0. If a copy of the MPL was not distributed with this \o/ \o/ \o/ \o/ \o/ \o/ \o/

(In reply to :Ms2ger from comment #66) > Comment on attachment 728453 [details] [diff] [review] > Convert FESpecularLighting > > Review of attachment 728453 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/svg/content/src/SVGFESpecularLightingElement.cpp > @@ +9,2 @@ > > > > +NS_IMPL_NS_NEW_NAMESPACED_SVG_ELEMENT(FESpecularLighting) > > Can you file a followup to make sure we remove the old one at some point? > > @@ +65,1 @@ > > this); > > Maybe > > return mNumberPairAttributes[KERNEL_UNIT_LENGTH].ToDOMAnimatedNumber( > nsSVGNumberPair::eFirst, this); > > to avoid the 80-columns limit. > > @@ +70,2 @@ > > { > > + return mNumberPairAttributes[KERNEL_UNIT_LENGTH].ToDOMAnimatedNumber(nsSVGNumberPair::eSecond, > > Same here > > @@ +75,5 @@ > > //---------------------------------------------------------------------- > > // nsSVGElement methods > > > > nsresult > > +SVGFESpecularLightingElement::Filter(nsSVGFilterInstance *instance, > > * to the left > > ::: content/svg/content/src/nsSVGFilters.cpp > @@ -2042,5 @@ > > -//------------------------------------------------------------ > > - > > -typedef nsSVGFE nsSVGFELightingElementBase; > > - > > -class nsSVGFELightingElement : public nsSVGFELightingElementBase > > I'm pretty sure this doesn't belong here. We need that because SpecularLighting inherits from it. I can file a followup to move it somewhere else if you want. > ::: content/svg/content/src/nsSVGFilters.h > @@ +22,5 @@ > > class nsSVGFilterResource; > > class nsSVGNumberPair; > > > > +#define DOT(a,b) (a[0] * b[0] + a[1] * b[1] + a[2] * b[2]) > > +#define NORMALIZE(vec) \ > > Making those two inline functions would make me happy.

Pushing everything except the last patch to avoid bitrot https://hg.mozilla.org/integration/mozilla-inbound/rev/46e4373d6d95 https://hg.mozilla.org/integration/mozilla-inbound/rev/9d3a5915e9e5 https://hg.mozilla.org/integration/mozilla-inbound/rev/240bee22606c https://hg.mozilla.org/integration/mozilla-inbound/rev/4d51bec2d507 https://hg.mozilla.org/integration/mozilla-inbound/rev/4813c1013888 https://hg.mozilla.org/integration/mozilla-inbound/rev/f5d415380052 https://hg.mozilla.org/integration/mozilla-inbound/rev/8bab36d9500f https://hg.mozilla.org/integration/mozilla-inbound/rev/3e25841f15c8 https://hg.mozilla.org/integration/mozilla-inbound/rev/9cbd2f80bd5f https://hg.mozilla.org/integration/mozilla-inbound/rev/e1267e72e21b https://hg.mozilla.org/integration/mozilla-inbound/rev/ee6e761d8186 https://hg.mozilla.org/integration/mozilla-inbound/rev/939f0488b0f7

https://hg.mozilla.org/mozilla-central/rev/46e4373d6d95 https://hg.mozilla.org/mozilla-central/rev/9d3a5915e9e5 https://hg.mozilla.org/mozilla-central/rev/240bee22606c https://hg.mozilla.org/mozilla-central/rev/4d51bec2d507 https://hg.mozilla.org/mozilla-central/rev/4813c1013888 https://hg.mozilla.org/mozilla-central/rev/f5d415380052 https://hg.mozilla.org/mozilla-central/rev/8bab36d9500f https://hg.mozilla.org/mozilla-central/rev/3e25841f15c8 https://hg.mozilla.org/mozilla-central/rev/9cbd2f80bd5f https://hg.mozilla.org/mozilla-central/rev/e1267e72e21b https://hg.mozilla.org/mozilla-central/rev/ee6e761d8186 https://hg.mozilla.org/mozilla-central/rev/939f0488b0f7

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a3aef14dc53a because something in the push caused, precipitated, whatever, bug 854225.

Relanded up to ConvolveMatrix because try said its ok. https://hg.mozilla.org/integration/mozilla-inbound/rev/5548aa2bab63 https://hg.mozilla.org/integration/mozilla-inbound/rev/7b7400c085a4 https://hg.mozilla.org/integration/mozilla-inbound/rev/37aaffbdb6f6 https://hg.mozilla.org/integration/mozilla-inbound/rev/ec645e7f419c https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6d59527eee https://hg.mozilla.org/integration/mozilla-inbound/rev/dea1e46826dc https://hg.mozilla.org/integration/mozilla-inbound/rev/f2af3063ab66 https://hg.mozilla.org/integration/mozilla-inbound/rev/5b7ed1616c62

https://hg.mozilla.org/mozilla-central/rev/5548aa2bab63 https://hg.mozilla.org/mozilla-central/rev/7b7400c085a4 https://hg.mozilla.org/mozilla-central/rev/37aaffbdb6f6 https://hg.mozilla.org/mozilla-central/rev/ec645e7f419c https://hg.mozilla.org/mozilla-central/rev/7f6d59527eee https://hg.mozilla.org/mozilla-central/rev/dea1e46826dc https://hg.mozilla.org/mozilla-central/rev/f2af3063ab66 https://hg.mozilla.org/mozilla-central/rev/5b7ed1616c62

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/cba3d3a781d9 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/da0127725cb0 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/07257e38eb89 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b32f8f514379

https://hg.mozilla.org/mozilla-central/rev/cba3d3a781d9 https://hg.mozilla.org/mozilla-central/rev/da0127725cb0 https://hg.mozilla.org/mozilla-central/rev/07257e38eb89 https://hg.mozilla.org/mozilla-central/rev/b32f8f514379