886416 - Move SVGLength to WebIDL

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[(no longer active)]

Attachment: Move SVGLength to WebIDL; r=bzbarsky

Comment 2

[(no longer active)]

Comment on attachment 8391986 [details] [diff] [review]
Move SVGLength to WebIDL; r=bzbarsky

https://tbpl.mozilla.org/?tree=Try&rev=b3190ccf4d6f

Comment 3

[David Zbarsky (:dzbarsky)]

Comment on attachment 8391986 [details] [diff] [review]
Move SVGLength to WebIDL; r=bzbarsky

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

::: js/xpconnect/tests/mochitest/test_bug790732.html
@@ -43,1 @@
>    is(Ci.nsIDOMNodeFilter, NodeFilter);

I don't think you want to get rid of this - we'd actually want this for the constants even if we got rid of nsIDOMSVGLength

Comment 4

[Masatoshi Kimura [:emk]]

(In reply to David Zbarsky (:dzbarsky) from comment #3)
> I don't think you want to get rid of this - we'd actually want this for the
> constants even if we got rid of nsIDOMSVGLength

Where do we need this? No in-tree users present. You can get the same constants from the global SVGLength property. Also, it will remove constants from the content-exposed Components shim. The real Components will have the constants which addons will use.

Comment 5

[Masatoshi Kimura [:emk]]

FYI I already explained it in bug 893117. Please do not confuse the Components shim with the real Components.

Comment 6

[(no longer active)]

(In reply to Masatoshi Kimura [:emk] from comment #4)
> (In reply to David Zbarsky (:dzbarsky) from comment #3)
> > I don't think you want to get rid of this - we'd actually want this for the
> > constants even if we got rid of nsIDOMSVGLength
> 
> Where do we need this? No in-tree users present. You can get the same
> constants from the global SVGLength property. Also, it will remove constants
> from the content-exposed Components shim. The real Components will have the
> constants which addons will use.

Comment 7

[David Zbarsky (:dzbarsky)]

(In reply to Masatoshi Kimura [:emk] from comment #4)
> (In reply to David Zbarsky (:dzbarsky) from comment #3)
> > I don't think you want to get rid of this - we'd actually want this for the
> > constants even if we got rid of nsIDOMSVGLength
> 
> Where do we need this? No in-tree users present. You can get the same
> constants from the global SVGLength property. Also, it will remove constants
> from the content-exposed Components shim. The real Components will have the
> constants which addons will use.

Currently Components.interfaces.nsIDOMSVGLength.SVG_LENGTHTYPE_PX works in content scopes, so we should probably try to avoid breaking pages.

Comment 8

[Masatoshi Kimura [:emk]]

Do you know a concrete example of breaking pages? Those pages will not work browsers other than Firefox anyway. They should update the page using SVGLength.SVG_LENGTHTYPE_PX (if at all present).

Comment 9

[David Zbarsky (:dzbarsky)]

It's probably safe to remove, but better to do that as a separate patch.

Comment 10

[Masatoshi Kimura [:emk]]

OK, filed bug 984461.

Comment 11

[(no longer active)]

I'm not sure if I follow all of the comments here.  Boris, please let me know what I should do.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I think the proposal is that you leave in that is() bit in the test and the components shim entry for now, and then remove those in bug 984461.

I guess that does minimize risk, though I suspect absolutely no one on the web uses Components.interfaces.nsIDOMSVGLength.

Comment 13

[(no longer active)]

Attachment: Move SVGLength to WebIDL; r=bzbarsky

Comment 14

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #12)
> I guess that does minimize risk, though I suspect absolutely no one on the
> web uses Components.interfaces.nsIDOMSVGLength.

Yeah, seriously!  And I also completely miss why we wouldn't at least try to see if we can remove this.  But I don't want to fight over that here.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8393944 [details] [diff] [review]
Move SVGLength to WebIDL; r=bzbarsky

You could probably have kept the three separate classes and virtual functions if desired... would just need a shared (pure virtual?) base class.  Anyway, I'm fine with having just one class.

>+++ b/content/svg/content/src/DOMSVGLength.cpp
>+DOMSVGLength::~DOMSVGLength()
>+    nsSVGAttrTearoffTable<nsSVGLength2, DOMSVGLength>& table =

This seem like a good use case for "auto"?

>+DOMSVGLength::GetTearOff(nsSVGLength2* aVal, nsSVGElement* aSVGElement,
>+  nsSVGAttrTearoffTable<nsSVGLength2, DOMSVGLength>& table =

And here.

>+  nsRefPtr<DOMSVGLength> element = table.GetTearoff(aVal);

s/element/domLength/?

>+DOMSVGLength::SetValueInSpecifiedUnits(float aValue, ErrorResult& aRv)
>+  if (mVal) {
>+    mVal->SetBaseValueInSpecifiedUnits(aValue, mSVGElement, true);

Need to return after that call, right?

>+DOMSVGLength::SetValueAsString(const nsAString& aValue, ErrorResult& aRv)
>+  if (mVal) {
>+    mVal->SetBaseValueString(aValue, mSVGElement, true);

And here.

>+DOMSVGLength::ConvertToSpecifiedUnits(uint16_t aUnit, ErrorResult& aRv)
>+  if (mVal) {
>+    mVal->ConvertToSpecifiedUnits(aUnit, mSVGElement);

And here.


>+++ b/content/svg/content/src/DOMSVGLength.h
>+  nsISupports* GetParentObject() const {
>+    return nullptr;

Could return Element() if mList, else mSVGElement?  I'm not convinced returning null is right.

>+  JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;

I assume you merged that to the aScope removal.

r=me with the above fixed.  Sorry this took so long...  I _think_ I've convinced myself that the lifetime management here is sufficiently unchanged.

Comment 16

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aac6762777be

Comment 17

[Ed Morley [:emorley]]

Backed out for mochitest failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=37633036&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=37633114&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=37633352&tree=Mozilla-Inbound

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

Comment 18

[(no longer active)]

Ah, I must have used auto&... Sorry.

Comment 19

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=67c09eea8a1f

Comment 20

[Ed Morley [:emorley]]

Bunch of valgrind spew, but presuming same cause:
https://tbpl.mozilla.org/php/getParsedLog.php?id=37635977&tree=Mozilla-Inbound

Comment 21

[(no longer active)]

(In reply to comment #20)
> Bunch of valgrind spew, but presuming same cause:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=37635977&tree=Mozilla-Inbound

Yes I think they're the same thing, but we'll know for sure on my try push.

Comment 22

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c035d35b5eb

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2c035d35b5eb