Move SVGLength to WebIDL

RESOLVED FIXED in mozilla31

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Ms2ger, Assigned: Away for a while)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 8391986 [details] [diff] [review]
Move SVGLength to WebIDL; r=bzbarsky
(Assignee)

Updated

4 years ago
Assignee: nobody → ehsan
(Assignee)

Comment 2

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

https://tbpl.mozilla.org/?tree=Try&rev=b3190ccf4d6f
Attachment #8391986 - Flags: review?(bzbarsky)
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
(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.
Keywords: site-compat
FYI I already explained it in bug 893117. Please do not confuse the Components shim with the real Components.
(Assignee)

Comment 6

4 years ago
(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.
Flags: needinfo?(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.
Flags: needinfo?(dzbarsky)
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).
It's probably safe to remove, but better to do that as a separate patch.
Blocks: 984461
OK, filed bug 984461.
Keywords: site-compat
(Assignee)

Comment 11

4 years ago
I'm not sure if I follow all of the comments here.  Boris, please let me know what I should do.
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.
(Assignee)

Comment 13

4 years ago
Created attachment 8393944 [details] [diff] [review]
Move SVGLength to WebIDL; r=bzbarsky
(Assignee)

Updated

4 years ago
Attachment #8391986 - Attachment is obsolete: true
Attachment #8391986 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Attachment #8393944 - Flags: review?(bzbarsky)
(Assignee)

Comment 14

4 years ago
(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 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.
Attachment #8393944 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 18

4 years ago
Ah, I must have used auto&... Sorry.
(Assignee)

Comment 21

4 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/2c035d35b5eb
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
(Assignee)

Updated

4 years ago
Depends on: 1018524
Depends on: 1158551
Depends on: 1284690
You need to log in before you can comment on or make changes to this bug.