653571 - Add IsIdentity() methods to SVGXxxListAndInfo classes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

This cset:
 http://hg.mozilla.org/mozilla-central/rev/a80424c4b0a8
added an "IsIdentity()" method to SVGPathDataAndInfo, as a clearer / more consistent way to check whether a given SVGPathDataAndInfo object is a just-initialized "identity" (zero) value, vs. whether it's actually got a meaningful value.

We should do that for the other SVGXxxListAndInfo classes (at least SVGLengthListAndInfo and SVGPointListAndInfo), so that we don't confusingly rely on "Is Element() null?" anymore to provide this information.

This is particularly important now that bug 653497 is making those classes' mElement pointers into nsWeakPtrs, which has a more multifaceted definition of what 'null' is.  The nsWeakPtr itself can be null/uninitialized, or its internal nsIWeakReference can have a cleared-out reference to its target once its target has gone away.  The former case should be considered 'identity' -- the latter should not. (and the latter case isn't actually something we expect to encounter when we're doing math, anyway, as I mused in bug 653497 comment 4).

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> This cset:
>  http://hg.mozilla.org/mozilla-central/rev/a80424c4b0a8
> added an "IsIdentity()" method to SVGPathDataAndInfo

er I meant "SVGPathDataAndOwner" (that's the one with a different name than the rest. We should probably rename that, too. :))

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> We should do that for the other SVGXxxListAndInfo classes (at least
> SVGLengthListAndInfo and SVGPointListAndInfo)

Those classes are defined here:
 http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGLengthList.h#172
 http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGPointList.h#160

Comment 3

[Biru Mohanathas [:poiru]]

Attachment: Add IsIdentity() function to SVG{Length,Point}ListAndInfo

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=6317c0979e90

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> This cset:
>  http://hg.mozilla.org/mozilla-central/rev/a80424c4b0a8
> added an "IsIdentity()" method to SVGPathDataAndInfo, as a clearer / more
> consistent way to check whether a given SVGPathDataAndInfo object is a
> just-initialized "identity" (zero) value, vs. whether it's actually got a
> meaningful value.

I'm not sure that I like that particularly. The whole "is identity" concept is a SMIL thing, and I think we should be trying to make the basic data classes that SMIL works on more SMIL agnostic, not less so. It would seem better that the *SMILType classes keep the knowledge to act as the shims here. Adding an IsEmpty method would be more 

If you are going to add this, please add a decent comment to the declaration of IsIdentity. To someone looking at SVGLengthList it's absolutely unclear what this IsIdentity thing is for, especially as it checks for mElement rather than zero-ness.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Watt [:jwatt] from comment #4)
> I'm not sure that I like that particularly. The whole "is identity" concept
> is a SMIL thing, and I think we should be trying to make the basic data
> classes that SMIL works on more SMIL agnostic, not less so.

Note that the header comment for SVGLengthListAndInfo indicates that it's specifically a "SVGLengthList" variant *for SMIL* to use. So, this is happening in a SMIL-specific abstraction.

> It would seem
> better that the *SMILType classes keep the knowledge to act as the shims
> here.

It's nice to keep this logic outside of the *SMILType classes ::Add() methods, because those classes' arithmetic functions have traditionally been hard for us to reason about correctly, edge-case-wise.  It's useful to hide some of the complexity behind mathematical abstractions like "identity values", and enforce invariants accordingly.

I do agree that it makes sense for this to be in SMIL-specific code, though. (and it is, per above, since these "AndInfo" classes are explicitly for use in SMIL.)

> Adding an IsEmpty method would be more 

Note that this class already exposes an IsEmpty() method (inherited), which only checks that our length list is empty. That's not quite what we want -- we *really* want to check whether we're a freshly-initialized, never-populated value. (which we treat as an identity value in arithmetic for SMIL types)  I think we could conceivably have one of these objects that has an Element set but has an empty length list, which would not count as an identity value. So the existing IsEmpty() isn't quite what we want.

> If you are going to add this, please add a decent comment to the declaration
> of IsIdentity.

Agreed -- we should add comments for these IsIdentity methods.

Comment 6

[Daniel Holbert [:dholbert]]

(Note also that the SMILType classes can't access mElement -- they can only access Element(), which returns null in both of the cases I describe in the last paragraph of comment 0, when we really only want to consider the first case as an "identity" value.  The only place we can distinguish between those two cases is inside of the *AndInfo classes, because that's the only code with access to the mElement weak pointer.)

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8337357 [details] [diff] [review]
Add IsIdentity() function to SVG{Length,Point}ListAndInfo

># HG changeset patch
># Parent db7f67f7a517f4c0cf03a73ab4f4768c5c08a577
># User Birunthan Mohanathas <[e-mail]>
>Bug 653571 - Add IsIdentity() function to SVG{Length,Point}ListAndInfo
>
>diff --git a/content/svg/content/src/SVGLengthList.h b/content/svg/content/src/SVGLengthList.h
>--- a/content/svg/content/src/SVGLengthList.h
>+++ b/content/svg/content/src/SVGLengthList.h
>+  bool IsIdentity() const {
>+    if (!mElement) {
>+      NS_ABORT_IF_FALSE(IsEmpty(), "target element propagation failure");
>+      return true;
>+    }

Per above, please add a comment above both IsIdentity declarations saying that this method returns true if this object is an "identity" value, from the perspective of SMIL -- i.e. if it's a value that was set up in SVGLengthListSMILType::Init() but has never received a call to SetInfo. Or something like that.

(mentioning SVGPointListSMILType::Init in the pointlist version of the comment, of course)

r=me with that, as long as jwatt doesn't object too strongly. :) (It sounds like he's OK with it as long as there's a comment, per end of comment 4)

Thanks!

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> Note that the header comment for SVGLengthListAndInfo indicates that it's
> specifically a "SVGLengthList" variant *for SMIL* to use. So, this is
> happening in a SMIL-specific abstraction.

Doh, yes indeed. Sorry for the noise.

Comment 9

[Biru Mohanathas [:poiru]]

Attachment: Add IsIdentity() function to SVG{Length,Point}ListAndInfo

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Per above, please add a comment above both IsIdentity declarations saying
> that this method returns true if this object is an "identity" value, from
> the perspective of SMIL -- i.e. if it's a value that was set up in
> SVGLengthListSMILType::Init() but has never received a call to SetInfo. Or
> something like that.

Done.

Comment 10

[Robert Longson [:longsonr]]

What happened to the comment 1 renaming? If that's not going to happen here can you raise a follow-up bug please?

Comment 11

[Biru Mohanathas [:poiru]]

(In reply to Robert Longson from comment #10)
> What happened to the comment 1 renaming? If that's not going to happen here
> can you raise a follow-up bug please?

Opened bug 943446 for that. Thanks for the reminder.

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8338538 [details] [diff] [review]
Add IsIdentity() function to SVG{Length,Point}ListAndInfo

>+++ b/content/svg/content/src/SVGPathData.h
>+   * Returns true if this object is an "identity" value, from the perspective
>+   * of SMIL. In other words, returns true until the initial value set up in
>+   * SVGLengthListSMILType::Init() has been changed with a SetInfo() call.

Nit: it looks like you have "SVGLengthListSMILType" into all of the IsIdentity comments, but they need to be adjusted to be the correct type for each class.

(e.g. this one should be SVGPointListSMILType)

Otherwise, still looks great. Let's fix this comment-nit before landing, though.

Comment 13

[Biru Mohanathas [:poiru]]

Attachment: Add IsIdentity() function to SVG{Length,Point}ListAndInfo

(In reply to Daniel Holbert [:dholbert] from comment #12)
> Nit: it looks like you have "SVGLengthListSMILType" into all of the
> IsIdentity comments, but they need to be adjusted to be the correct type for
> each class.

Thanks, fixed!

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8338650 [details] [diff] [review]
Add IsIdentity() function to SVG{Length,Point}ListAndInfo

>+++ b/content/svg/content/src/SVGPathData.h
>+  /**
>+   * Returns true if this object is an "identity" value, from the perspective
>+   * of SMIL. In other words, returns true until the initial value set up in
>+   * SVGPathSegListSMILType::Init() has been changed with a SetInfo() call.

Sorry, one more nit: s/SetInfo/SetElement/ in this comment

http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGPathData.h#242

(SVGPathDataAndOwner only tracks one piece of info -- Element -- so its setup method is called SetElement instead of SetInfo)

r=me with that. :)

Comment 15

[Daniel Holbert [:dholbert]]

Ah, looks like this just landed as-is -- no big deal, I'll push a followup to fix the code-comment issue from comment 14.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/288be55b74aa

Comment 17

[Daniel Holbert [:dholbert]]

Pushed a followup to address comment 14:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/c81e58d7c31d

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/288be55b74aa
https://hg.mozilla.org/mozilla-central/rev/c81e58d7c31d