Cameron McCormack (:heycam) Assignee
	Description • 11 years ago

Here's a WIP.  This works, but I haven't written any tests yet.  Also the property probably needs to go behind a pref.

	Cameron McCormack (:heycam) Assignee
	Comment 1 • 11 years ago

r?bz for adding the property (the content/, layout/style/ and modules/ bits);
r?roc for the rendering (the layout/svg/ and gfx/ bits)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 2 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 11 years ago

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...

	Cameron McCormack (:heycam) Assignee
	Comment 5 • 11 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 6 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfef9b308f92

Docs folks: note that this is behind a pref svg.paint-order.enabled.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 11 years ago

Note that docs folks are swamped with b2g for the last many months and foreseeable future.  I recommend just documenting this yourself.

	Cameron McCormack (:heycam) Assignee
	Comment 8 • 11 years ago

Ah yep, fair enough.

	Cameron McCormack (:heycam) Assignee
	Comment 9 • 11 years ago

Landed a couple of followups:

https://hg.mozilla.org/integration/mozilla-inbound/rev/761992365553
https://hg.mozilla.org/integration/mozilla-inbound/rev/991938589ebe

	Cameron McCormack (:heycam) Assignee
	Comment 10 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/bfef9b308f92
https://hg.mozilla.org/mozilla-central/rev/761992365553
https://hg.mozilla.org/mozilla-central/rev/991938589ebe

	Mayank Bansal
	Comment 12 • 11 years ago

dont know where to report this, but  https://developer.mozilla.org/en-US/docs/SVG/Attribute/paint-order  page uses high CPU when scrolled.

http://people.mozilla.com/~bgirard/cleopatra/#report=2ea8407db63deb9b5c6b005e129abe3b2d2eb9bb

	Robert Longson [:longsonr]
	Comment 13 • 11 years ago

mayankleoboy1 please raise a new bug.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 14 • 11 years ago

(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

	Cameron McCormack (:heycam) Assignee
	Comment 15 • 11 years ago

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.

	Jesse Ruderman
	Comment 16 • 11 years ago

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.