Closed Bug 529934 Opened 10 years ago Closed 10 years ago

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


(Core :: CSS Parsing and Computation, defect)

Not set





(Reporter: dholbert, Assigned: dholbert)




(2 files)

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.
Attached patch fixSplinter Review
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
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]


>+    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,
  GetStringBufferValue (as it is now)
Also, you should probably add an IsStringUnit() helper function (like
the other Is*Unit functions) and call that in these assertions.


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

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):
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:
Closed: 10 years ago
Resolution: --- → FIXED
Just noticed one more thing (while merging):  it probably would have been good to use IsStringUnit() 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()

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