Add "UnparsedString" unit to nsStyleAnimation, as catch-all for non-interpolatable properties

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

We can extend nsStyleAnimation to support basically all non-interpolatable properties by simply caching the string given to us in ComputeValue, and returning the string in UncomputeValue.

Patch coming up.
Created attachment 413440 [details] [diff] [review]
fix

This patch extends nsStyleAnimation::Value to be able to contain string values, largely mirroring the similar code in nsCSSValue.

This also extends ComputeValue to use this type for any property of type "eStyleAnimType_None", so that we can serialize & de-serialize specified values for all of those properties.

Note that this patch layers on top of "patch 1 v2" from bug 520239 (attachment 413437 [details] [diff] [review]), which splits out most of ComputeValue into a separate helper function.
Attachment #413440 - Flags: review?(dbaron)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
I had another thought, which is that we can actually make this work for both forms of UncomputeValue rather than just the string form relatively easily, either by:
 (a) holding on to an nsCSSCompressedDataBlock (would need the patches in bug 522595), or
 (b) adding a unit type for "uncomputed" values of each of the five units stored in nsCSSCompressedDataBlock (with the patches in my tree, we already have storage code for all of them except nsCSSValue, I think)

However, it seems like SMIL doesn't need that, so this may be fine (although we should add comments pointing to the possible better solution if it's needed in the future, since it's possible, perhaps even likely, that further optimization to the SMIL code in the future might make us want the other UncomputeValue for it).


(More later when I review the whole thing.)
Comment on attachment 413440 [details] [diff] [review]
fix

nsStyleAnimation.h:

>+    const PRUnichar* GetStringBufferValue() const {
>+      NS_ASSERTION(mUnit == eUnit_UnparsedString, "unit mismatch");
>+      return GetBufferValue(mValue.mString);
>+    }
>+
>+    void GetUnparsedStringValue(nsAString& aBuffer) const {
>+      NS_ASSERTION(mUnit == eUnit_UnparsedString, "unit mismatch");
>+      aBuffer.Truncate();
>+      PRUint32 len = NS_strlen(GetBufferValue(mValue.mString));
>+      mValue.mString->ToString(len, aBuffer);
>+    }

I think these functions ought to have more similar names, in particular,
probably:
  GetStringBufferValue (as it is now)
  GetStringValue
Also, you should probably add an IsStringUnit() helper function (like
the other Is*Unit functions) and call that in these assertions.

nsStyleAnimation.cpp:

In nsStyleAnimation::Value::SetUnparsedStringValue, your seccond
assignment to mUnit should be assigning mUnit = eUnit_Null, not
eUnit_UnparsedString!

In operator==:
>+    case eUnit_UnparsedString:
>+      return (NS_strcmp(GetBufferValue(mValue.mString),
>+                        GetBufferValue(aOther.mValue.mString)) == 0);
I think it would be slightly clearer to use GetStringBufferValue rather
than the static method GetBufferValue.

r=dbaron with that; sorry for the delay
Attachment #413440 - Flags: review?(dbaron) → review+
Thanks for the fixes! Addressed them in my patch queue (including a s/SetUnparsedStringValue/SetStringValue/, to match the "GetStringValue" change that you suggested):
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/a83bff4279ea
Hmm.  I actually though SetUnparsedStringValue should stay as it was; if it's SetStringValue then it ought to take an aUnit argument, but that's silly (now, anyway) given that there's only one string unit.
Right, I had the same thought -- we can add a unit argument to SetStringValue if & when we add more string units.
Re-reading comment 5, I think I misunderstood you in comment 6 -- sounds like you'd prefer to keep it as SetUnparsedStringValue.  Done:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/f2143b75351b
Landed: http://hg.mozilla.org/mozilla-central/rev/abcb86186d79
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Just noticed one more thing (while merging):  it probably would have been good to use IsStringUnit() in FreeValue().
Created attachment 416971 [details] [diff] [review]
minor followup: Use IsStringValue in FreeValue()

Agreed - thanks for that.  Here's a followup patch to make that change.
Attachment #416971 - Flags: review?(dbaron)
Comment on attachment 416971 [details] [diff] [review]
minor followup: Use IsStringValue in FreeValue()

r=dbaron
Attachment #416971 - Flags: review?(dbaron) → review+
You need to log in before you can comment on or make changes to this bug.