Last Comment Bug 828805 - implement paint-order property from SVG 2
: implement paint-order property from SVG 2
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: Cameron McCormack (:heycam) (away Sep 28)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-09 21:26 PST by Cameron McCormack (:heycam) (away Sep 28)
Modified: 2013-02-20 11:00 PST (History)
8 users (show)
cam: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
WIP (v0.1) (35.24 KB, patch)
2013-01-09 21:26 PST, Cameron McCormack (:heycam) (away Sep 28)
no flags Details | Diff | Splinter Review
patch (v1.0) (47.41 KB, patch)
2013-01-10 22:13 PST, Cameron McCormack (:heycam) (away Sep 28)
roc: review+
bzbarsky: review+
Details | Diff | Splinter Review

Description Cameron McCormack (:heycam) (away Sep 28) 2013-01-09 21:26:58 PST
Created attachment 700203 [details] [diff] [review]
WIP (v0.1)

Here's a WIP.  This works, but I haven't written any tests yet.  Also the property probably needs to go behind a pref.
Comment 1 Cameron McCormack (:heycam) (away Sep 28) 2013-01-10 22:13:32 PST
Created attachment 700863 [details] [diff] [review]
patch (v1.0)

r?bz for adding the property (the content/, layout/style/ and modules/ bits);
r?roc for the rendering (the layout/svg/ and gfx/ bits)
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-01-10 22:20:53 PST
Comment on attachment 700863 [details] [diff] [review]
patch (v1.0)

Review of attachment 700863 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsStyleConsts.h
@@ +868,5 @@
> +#define NS_STYLE_PAINT_ORDER_FILL               1
> +#define NS_STYLE_PAINT_ORDER_STROKE             2
> +#define NS_STYLE_PAINT_ORDER_MARKERS            3
> +#define NS_STYLE_PAINT_ORDER_LAST_VALUE NS_STYLE_PAINT_ORDER_MARKERS
> +#define NS_STYLE_PAINT_ORDER_BITWIDTH           2

Some documentation is needed here
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2013-01-11 18:26:41 PST
Comment on attachment 700863 [details] [diff] [review]
patch (v1.0)

>+          // Already seen this component.
>+          return false;

Shouldn't there be some error-reporting here?

>+        position += NS_STYLE_PAINT_ORDER_BITWIDTH;

There need to be some static asserts about the usable bits in "order" being enough for the maximal possible number of copies of BITWIDTH we might need.

In any case, please make seen and order unsigned?

>+       // Can't have "normal" in the middle of the list of paint components.
>+       return false;

Again, error reporting.

>+    value.SetIntValue(order, eCSSUnit_Enumerated);

Either this needs to live inside the "component != NS_STYLE_PAINT_ORDER_NORMAL" block or you need a static assert that 0 == NS_STYLE_PAINT_ORDER_NORMAL (which is what this code is assuming).  That assert might be a good idea anyway.

In nsCSSValue::AppendToString, should we be going for minimal serializations?  That is, given "paint-order: stroke", should we reserialize it as "stroke" or as "stroke fill markers" (as the patch does)?

The code in nsComputedDOMStyle::DoGetPaintOrder is basically identical to the code in nsCSSValue::AppendToString.  Can we share that code via some utility function as needed?

r=me with those fixed
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2013-01-11 18:54:16 PST
To be clear, the minimal serialization question is an open one...  "No" is an OK answer, as long as we understand why.  But normally we'd aim for minimal serialization...
Comment 5 Cameron McCormack (:heycam) (away Sep 28) 2013-01-11 19:07:57 PST
Hmm, well in the spec at the moment I've written "Computed value: as specified", which would mean minimal serialisation, assuming we don't have a separate "Serialize as:" line.

I can think of one potential advantage of maximal serialisation: it allows the author to determine the set of possible values for the property, in case we ever extend it.  Still, the main reason for filling in the unspecified components in their default order in the first place was to avoid the author having to know the complete set of components, so maybe it's not that necessary.

I think I'll adjust it to serialise minimally.


(In reply to Boris Zbarsky (:bz) from comment #3)
> Shouldn't there be some error-reporting here?

Will add some.

> >+        position += NS_STYLE_PAINT_ORDER_BITWIDTH;
> 
> There need to be some static asserts about the usable bits in "order" being
> enough for the maximal possible number of copies of BITWIDTH we might need.

There is assert in nsRuleNode.cpp, which isn't about "order" being big enough but nsStyleSVG::mPaintOrder.  I'll add one here for "order" too.

> In any case, please make seen and order unsigned?

OK, though CSSValue::SetIntValue takes a signed integer, so I'll need to cast it later anyway.

> >+    value.SetIntValue(order, eCSSUnit_Enumerated);
> 
> Either this needs to live inside the "component !=
> NS_STYLE_PAINT_ORDER_NORMAL" block or you need a static assert that 0 ==
> NS_STYLE_PAINT_ORDER_NORMAL (which is what this code is assuming).  That
> assert might be a good idea anyway.

Yes, good idea.
Comment 6 Cameron McCormack (:heycam) (away Sep 28) 2013-01-12 15:47:12 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfef9b308f92

Docs folks: note that this is behind a pref svg.paint-order.enabled.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2013-01-12 16:38:49 PST
Note that docs folks are swamped with b2g for the last many months and foreseeable future.  I recommend just documenting this yourself.
Comment 8 Cameron McCormack (:heycam) (away Sep 28) 2013-01-12 16:45:16 PST
Ah yep, fair enough.
Comment 9 Cameron McCormack (:heycam) (away Sep 28) 2013-01-12 16:46:27 PST
Landed a couple of followups:

https://hg.mozilla.org/integration/mozilla-inbound/rev/761992365553
https://hg.mozilla.org/integration/mozilla-inbound/rev/991938589ebe
Comment 10 Cameron McCormack (:heycam) (away Sep 28) 2013-01-12 17:58:11 PST
I added a page on MDN (browser compatibility stating that the property is available in Nightly should be valid once the next inbound -> central merge and nightly build happens!):

https://developer.mozilla.org/en-US/docs/SVG/Attribute/paint-order
Comment 13 Robert Longson 2013-01-13 09:49:25 PST
mayankleoboy1 please raise a new bug.
Comment 14 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-01-13 09:51:14 PST
(In reply to Cameron McCormack (:heycam) from comment #5)
> Hmm, well in the spec at the moment I've written "Computed value: as
> specified", which would mean minimal serialisation, assuming we don't have a
> separate "Serialize as:" line.

Just one comment here; I don't see the relationship between the computed value lines and serialization at all; see http://lists.w3.org/Archives/Public/www-style/2009Mar/0329.html
Comment 15 Cameron McCormack (:heycam) (away Sep 28) 2013-01-13 14:44:03 PST
OK, I had kinda remembered that people were inferring serialisation from the "Computed value" line and that some new specs might have been adding explicit serialisations for properties, but didn't realise that inferring the serialisation this way was incorrect.
Comment 16 Jesse Ruderman 2013-02-19 01:01:24 PST
This is listed on https://developer.mozilla.org/en-US/docs/Firefox_21_for_developers.  I don't think this feature is big enough for a relnote, but I filed bug 842480 to have the relnote page link to the MDN page.

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