Last Comment Bug 520239 - SVG SMIL: Add support for animating CSS shorthand properties
: SVG SMIL: Add support for animating CSS shorthand properties
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Daniel Holbert [:dholbert]
:
: Jet Villegas (:jet)
Mentors:
Depends on: 474049 529934 530199
Blocks: 537142 520487
  Show dependency treegraph
 
Reported: 2009-10-02 12:11 PDT by Daniel Holbert [:dholbert]
Modified: 2009-12-29 12:24 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1: Split out nsStyleAnimation::BuildCustomSiblingStyleContext (5.61 KB, patch)
2009-11-13 16:32 PST, Daniel Holbert [:dholbert]
dbaron: review+
Details | Diff | Splinter Review
patch 2: make "BuildStyleRule" work with shorthands (3.96 KB, patch)
2009-11-13 16:43 PST, Daniel Holbert [:dholbert]
dbaron: review+
Details | Diff | Splinter Review
patch 3: add support on SMIL side (33.71 KB, patch)
2009-11-13 17:04 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
patch 3 v1: add support on SMIL side for 'marker', 'overflow' (24.84 KB, patch)
2009-11-16 15:47 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
patch 4 WIP: add support for 'font' (8.60 KB, patch)
2009-11-16 15:51 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
patch 3 v2: add support on SMIL side for 'marker', 'overflow' (24.90 KB, patch)
2009-11-16 16:06 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review
patch 1 v2: addresses review comments (13.39 KB, patch)
2009-11-19 14:13 PST, Daniel Holbert [:dholbert]
dholbert: review+
Details | Diff | Splinter Review
patch 5: [REPLACES 3 & 4] treat shorthands like any other property (with eUnit_UnparsedString) (21.05 KB, patch)
2009-11-20 16:24 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2009-10-02 12:11:57 PDT
Bug 474049's main patch will be landing soon, but it doesn't include support for shorthand properties mentioned in the SVG property index ("font", "overflow", and "marker").

This bug is on supporting those shorthand properties.
Comment 1 Daniel Holbert [:dholbert] 2009-11-13 16:32:30 PST
Created attachment 412337 [details] [diff] [review]
patch 1: Split out nsStyleAnimation::BuildCustomSiblingStyleContext

This is the first patch needed here.

Some background -- currently, nsStyleAnimation::ComputeValue does this:
   A) Parses the given value for the given property.
   B) Creates a temporary nsStyleContext, which we can use to access the result of (A).
   C) Calls ExtractComputedValue to query that context for the computed value of our property.

The above pathway assumes that steps (A) and (C) are interested in the same property.  However, that's not what we want for shorthand properties.

e.g. with "font", we **parse** a value for the "font" property, but we then want to **query** the computed values of multiple different sub-properties.  

Similarly, for the single-valued shorthands "overflow" and "marker", we can **parse** a specified value for the shorthand property, but we still can't directly **query** that shorthand property's computed value -- we have to query (any of) its subproperties instead.

So, this patch splits out steps (A)/(B) from step (C), so that we can e.g. do (A)/(B) _once_ to parse a "font" value, and then re-use the result in multiple iterations of (C). (for each subproperty)
Comment 2 Daniel Holbert [:dholbert] 2009-11-13 16:43:42 PST
Created attachment 412342 [details] [diff] [review]
patch 2: make "BuildStyleRule" work with shorthands

This patch does two things:
 (1) Turn on animation support for "overflow-x" and "overflow-y" in nsCSSPropList.h, so that the upcoming "patch 3" can query nsStyleAnimation for computed values of these properties.
 (2) Makes the "BuildStyleRule" helper function in nsStyleAnimation work for shorthand properties. (Explanation below)

Explanation:
In current mozilla-central, BuildStyleRule uses this call:
>    declaration->SlotForValue(aProperty)
to check whether parsing succeeded.  But that doesn't work for shorthands, because there's no single slot for a shorthand's value.  Instead, for a shorthand, we can just pass one of its subproperties to SlotForValue.

This patch reworks the logic in this method, to make this change a bit easier.  It breaks up what previously was a giant 11-line "if" statement into a few nested "if" clauses, so that I can declare a local variable in one clause to simplify things.
Comment 3 Daniel Holbert [:dholbert] 2009-11-13 17:04:09 PST
Created attachment 412348 [details] [diff] [review]
patch 3: add support on SMIL side

This patch handles animation of shorthand properties on the SMIL side.  Most of this is done in the class "nsSMILCSSShorthandType" (which is analogous to nsSMILCSSValueType).

This patch currently works for both "overflow" and "marker" (layering on top of the URI patches in bug 520487).

This currently has some "font" chunks commented out, because we can't yet animate the subproperties "font-stretch", "font-weight" (bug 528234) and "font-family" (bug 528656).  Once we've got those, though, this patch should be fine with those chunks un-commented.

Requesting review on the work here so far, with the understanding that minimal changes will be required here after bug 528234/bug 528656 are fixed.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-13 18:47:46 PST
+  // Just using NS_ASSERTION here because this assertion could fail when called
+  // from the cleanup "Destroy()" call in OOM or no-PresContext conditions.

Then it should be a WARNING, right?

+      // Crap! We lack a PresContext or a PresShell
+      success = PR_FALSE;
+      NS_NOTREACHED("Not parsing animation value; unable to get PresContext");

If we can get into this state, then this shouldn't be NS_NOTREACHED.

+struct ShorthandValue {
+  ShorthandValue() : mPropID(eCSSProperty_UNKNOWN), mPresContext(nsnull),
+                     mIsInherit(PR_FALSE), mData(nsnull) {}
+  nsCSSProperty   mPropID;
+  nsPresContext*  mPresContext;
+  PRPackedBool    mIsInherit;
+  GenericWrapper* mData;  // Contents depends on the shorthand property.
+                          // (Or, if mIsInherit is true, mData should be null)
+};

Would it be possible to simply create subclasses of ShorthandValue instead of using this mData field?
Comment 5 Daniel Holbert [:dholbert] 2009-11-16 14:11:19 PST
(In reply to comment #4)
> Would it be possible to simply create subclasses of ShorthandValue instead of
> using this mData field?

Yeah, and that's probably cleaner.  Reworking the patch to do that...
Comment 6 Daniel Holbert [:dholbert] 2009-11-16 15:47:52 PST
Created attachment 412708 [details] [diff] [review]
patch 3 v1: add support on SMIL side for 'marker', 'overflow'

This patch uses subclasses of ShorthandValue instead of the generic "mData" pointer, as roc suggests in comment 4.

This fixes the inappropriate-warning-level issues roc mentioned in comment 4, as well.

I've split off 'font' support into a different patch, which I'll attach separately.
Comment 7 Daniel Holbert [:dholbert] 2009-11-16 15:51:50 PST
Created attachment 412712 [details] [diff] [review]
patch 4 WIP: add support for 'font'

Here's a patch with the added code for 'font'.  We still need 'font-family' (bug 528656) before this one will work.
Comment 8 Daniel Holbert [:dholbert] 2009-11-16 16:06:15 PST
Created attachment 412715 [details] [diff] [review]
patch 3 v2: add support on SMIL side for 'marker', 'overflow'

Sorry, the last version of "patch 3" still had an XXXdholbert about needing a header-comment for the nsSMILCSSShorthandValueType class.  Added that in this version.
Comment 9 David Baron :dbaron: ⌚️UTC-10 2009-11-18 14:09:01 PST
Comment on attachment 412337 [details] [diff] [review]
patch 1: Split out nsStyleAnimation::BuildCustomSiblingStyleContext

I think you should call the new method StyleWithDeclarationAdded or something like that, since what it does is return a style context for the element's style plus the given declaration overriding it.

You should document that the method also returns null if the given value does not parse correctly (and probably document that clearly for ComputeValue too).

You should change the |return PR_FALSE| to |return nsnull| inside the function.

Finally, you should make ComputeValue return false when this new method returns null.  Right now it looks like it will crash.  (And if we don't have a test that would have caught that, we should add one.)

r=dbaron with those things fixed
Comment 10 David Baron :dbaron: ⌚️UTC-10 2009-11-18 14:13:37 PST
Comment on attachment 412342 [details] [diff] [review]
patch 2: make "BuildStyleRule" work with shorthands

I'd prefer if you keep the code structure so that the error case is the early return and the success case goes to the end of the function (like code with exceptions works).  That's relatively easy in this case: just compute |propertyToCheck| before the if statement.

Also, it looked like part of the nsCSSPropList.h changes from patch 3 crept into this patch; those should be in patch 3 instead.

Also, rather than using:
*nsCSSProps::SubpropertyEntryFor(aProperty)
I'd prefer:
nsCSSProps::SubpropertyEntryFor(aProperty)[0]
which is technically equivalent but more logically correct.
r=dbaron with that
Comment 11 Daniel Holbert [:dholbert] 2009-11-19 14:13:28 PST
Created attachment 413437 [details] [diff] [review]
patch 1 v2: addresses review comments

This addresses review comments from comment #9.  carrying forward r+.

> Finally, you should make ComputeValue return false when this new method returns
> null.  Right now it looks like it will crash.  (And if we don't have a test
> that would have caught that, we should add one.)

FWIW, this was correct -- with the old patch version, we did crash when the CSS parser failed to parse a value.  This version adds a mochitest to catch this sort of thing in the future.
Comment 12 Daniel Holbert [:dholbert] 2009-11-20 16:24:16 PST
Created attachment 413737 [details] [diff] [review]
patch 5: [REPLACES 3 & 4] treat shorthands like any other property (with eUnit_UnparsedString)

So -- now that we have bug 529934 (which lets us just serialize to a string-value rather than to a CSS-computed-value), we can actually just handle Shorthand properties like any other CSS property -- serializing them to a single string-value, rather than to a bunch of computed values.

This patch is a replacement for "patch 3" & "patch 4 WIP".  It removes a couple of "no shorthands allowed" assumptions in nsSMILCSSValueType, and it removes the nsISMILCSSValueType interface, since that's no longer needed.

(This also layers on top of the patch in bug 530199.)

Note You need to log in before you can comment on or make changes to this bug.