Shrink SVG paint-order to take less bits

Shrink SVG paint-order to take less bits

This patch is heavily inspired from WebKit.
http://trac.webkit.org/changeset/165595

dominant-baseline property will be moved from non-inherited to inherited.
To save the space for it, paint-order needs to be shrink.

BUG=142215
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199879

[Shanmuga Pandi, 2015-07-28 10:01:44]

shanmuga.m@samsung.com changed reviewers:
+ rob.buis@samsung.com

[Shanmuga Pandi, 2015-07-28 10:03:29]

Please check this patch.
Thank you.

[rwlbuis, 2015-07-28 16:05:30]

On 2015/07/28 10:03:29, Shanmuga Pandi wrote:
> Please check this patch.
> Thank you.

Looks good to me. I am not sure that you have to mention the last paragraph in
the Description, the change seems valuable standalone. Maybe pdr will have an
opinion on that...

[rwlbuis, 2015-07-28 16:06:27]

On 2015/07/28 16:05:30, rwlbuis wrote:
> On 2015/07/28 10:03:29, Shanmuga Pandi wrote:
> > Please check this patch.
> > Thank you.
> 
> Looks good to me. I am not sure that you have to mention the last paragraph in
> the Description, the change seems valuable standalone. Maybe pdr will have an
> opinion on that...

Related, you may need to create a new bug for this, but since it is just a
supporting patch maybe the one you used is ok.

[rwlbuis, 2015-07-28 22:52:35]

rob.buis@samsung.com changed reviewers:
+ pdr@chromium.org

[rwlbuis, 2015-07-28 22:52:51]

On 2015/07/28 16:06:27, rwlbuis wrote:
> On 2015/07/28 16:05:30, rwlbuis wrote:
> > On 2015/07/28 10:03:29, Shanmuga Pandi wrote:
> > > Please check this patch.
> > > Thank you.
> > 
> > Looks good to me. I am not sure that you have to mention the last paragraph
in
> > the Description, the change seems valuable standalone. Maybe pdr will have
an
> > opinion on that...
> 
> Related, you may need to create a new bug for this, but since it is just a
> supporting patch maybe the one you used is ok.

+pdr for proper review.

[pdr., 2015-07-29 05:18:35]

pdr@chromium.org changed reviewers:
+ ed@chromium.org, fs@opera.com

[pdr., 2015-07-29 05:20:50]

Overall I like the direction of this patch. Mostly high-level concerns below as
the code looks good.

+cc ed@chromium.org as well, as he was the original author of this code (his
first big patch, iirc)

https://codereview.chromium.org/1252933003/diff/1/LayoutTests/svg/css/script-...
File LayoutTests/svg/css/script-tests/svg-paint-order.js (right):

https://codereview.chromium.org/1252933003/diff/1/LayoutTests/svg/css/script-...
LayoutTests/svg/css/script-tests/svg-paint-order.js:33: test("fill stroke
markers", "fill");
Is this change correct?

This seems to make us match both Gecko and WebKit but the spec seems to say we
should return the computed value "as specified":
https://svgwg.org/svg2-draft/single-page.html#painting-PaintOrder

I made a little demo to test this with: http://jsfiddle.net/Lpxsuto8

I think it makes sense to change our behavior, just want to be sure to update
the spec if we do.

https://codereview.chromium.org/1252933003/diff/1/Source/core/style/SVGComput...
File Source/core/style/SVGComputedStyle.cpp (right):

https://codereview.chromium.org/1252933003/diff/1/Source/core/style/SVGComput...
Source/core/style/SVGComputedStyle.cpp:251: Vector<EPaintOrderType, 3>
SVGComputedStyle::paintTypesForPaintOrder() const
Returning a Vector seems a bit heavy for this code since painting is quite
performance sensitive. The WebKit code was very careful to use move semantics to
avoid copies but we can't yet use them in blink. I think it may be best to keep
using bit twiddling tricks here. This won't be too hard and I think we can even
make the code a little cleaner.

My recommendation would be to try to re-use the existing (unsigned pt =
(paintOrder() >> (kPaintOrderBitwidth*index)) & ((1u << kPaintOrderBitwidth) -
1)) code here.

It's also very possible that I'm optimizing too early here. If you think so,
could you run the svg perf tests with and without your change to see what the
difference is?

https://codereview.chromium.org/1252933003/diff/1/Source/core/style/SVGComput...
File Source/core/style/SVGComputedStyleDefs.h (right):

https://codereview.chromium.org/1252933003/diff/1/Source/core/style/SVGComput...
Source/core/style/SVGComputedStyleDefs.h:117: // These are all minimized
combinations of paint-order.
Using minimized names was a bit confusing to me at first. What do you think
about switching this to be:
PaintOrderFillStrokeMarkers
PaintOrderFillMarkersStroke
PaintOrderStrokeFillMarkers
PaintOrderStrokeMarkersFill
PaintOrderMarkersFillStroke
PaintOrderMarkersStrokeFill

[Shanmuga Pandi, 2015-07-29 06:55:04]

https://codereview.chromium.org/1252933003/diff/1/LayoutTests/svg/css/script-...
File LayoutTests/svg/css/script-tests/svg-paint-order.js (right):

https://codereview.chromium.org/1252933003/diff/1/LayoutTests/svg/css/script-...
LayoutTests/svg/css/script-tests/svg-paint-order.js:33: test("fill stroke
markers", "fill");
On 2015/07/29 05:20:50, pdr wrote:
> Is this change correct?
> 
> This seems to make us match both Gecko and WebKit but the spec seems to say we
> should return the computed value "as specified":
> https://svgwg.org/svg2-draft/single-page.html#painting-PaintOrder
> 
> I made a little demo to test this with: http://jsfiddle.net/Lpxsuto8
> 
> I think it makes sense to change our behavior, just want to be sure to update
> the spec if we do.

 Is it ok to change the code as like old approach. 
For ex :
 test("fill stroke", "fill stroke markers") instead of  test("fill stroke",
"fill");  ? 
Even then we are not achieving the spec item , computed value "as specified".

https://codereview.chromium.org/1252933003/diff/1/Source/core/style/SVGComput...
File Source/core/style/SVGComputedStyle.cpp (right):

https://codereview.chromium.org/1252933003/diff/1/Source/core/style/SVGComput...
Source/core/style/SVGComputedStyle.cpp:251: Vector<EPaintOrderType, 3>
SVGComputedStyle::paintTypesForPaintOrder() const
On 2015/07/29 05:20:50, pdr wrote:
> Returning a Vector seems a bit heavy for this code since painting is quite
> performance sensitive. The WebKit code was very careful to use move semantics
to
> avoid copies but we can't yet use them in blink. I think it may be best to
keep
> using bit twiddling tricks here. This won't be too hard and I think we can
even
> make the code a little cleaner.
> 
> My recommendation would be to try to re-use the existing (unsigned pt =
> (paintOrder() >> (kPaintOrderBitwidth*index)) & ((1u << kPaintOrderBitwidth) -
> 1)) code here.
> 
> It's also very possible that I'm optimizing too early here. If you think so,
> could you run the svg perf tests with and without your change to see what the
> difference is?

Ok. I will try to change the code as you suggested.

https://codereview.chromium.org/1252933003/diff/1/Source/core/style/SVGComput...
File Source/core/style/SVGComputedStyleDefs.h (right):

https://codereview.chromium.org/1252933003/diff/1/Source/core/style/SVGComput...
Source/core/style/SVGComputedStyleDefs.h:117: // These are all minimized
combinations of paint-order.
On 2015/07/29 05:20:50, pdr wrote:
> Using minimized names was a bit confusing to me at first. What do you think
> about switching this to be:
> PaintOrderFillStrokeMarkers
> PaintOrderFillMarkersStroke
> PaintOrderStrokeFillMarkers
> PaintOrderStrokeMarkersFill
> PaintOrderMarkersFillStroke
> PaintOrderMarkersStrokeFill

Ok. I will change.

[Shanmuga Pandi, 2015-07-29 13:02:43]

I have updated the code.
Please review it.
Thanks

[fs, 2015-07-30 14:39:37]

On 2015/07/29 06:55:04, Shanmuga Pandi wrote:
>
https://codereview.chromium.org/1252933003/diff/1/LayoutTests/svg/css/script-...
> File LayoutTests/svg/css/script-tests/svg-paint-order.js (right):
> 
>
https://codereview.chromium.org/1252933003/diff/1/LayoutTests/svg/css/script-...
> LayoutTests/svg/css/script-tests/svg-paint-order.js:33: test("fill stroke
> markers", "fill");
> On 2015/07/29 05:20:50, pdr wrote:
> > Is this change correct?
> > 
> > This seems to make us match both Gecko and WebKit but the spec seems to say
we
> > should return the computed value "as specified":
> > https://svgwg.org/svg2-draft/single-page.html#painting-PaintOrder

I suspect this "as specified" is the result of being the "default" when the
"Computed value" field was added - spec archeology would confirm I suppose.
Keeping 'normal' as a computed value doesn't make a whole lot of sense to me,
but if that what all impls does (after this patch) then I guess we shouldn't
cause a stir over it... (I also wonder if "as specified" allows this kind of
"minimization" - which, I should note, is also being done in the
.style.paintOrder = ...; console.log(...style.paintOrder) case).

https://codereview.chromium.org/1252933003/diff/40001/Source/core/css/Compute...
File Source/core/css/ComputedStyleCSSValueMapping.cpp (right):

https://codereview.chromium.org/1252933003/diff/40001/Source/core/css/Compute...
Source/core/css/ComputedStyleCSSValueMapping.cpp:1194: case
PaintOrderFillStrokeMarkers:
Eep! This looks like large (code) footprint =( - could pass const
SVGComputedStyle& and essentially reuse the existing code I think (maybe a tad
ugly, but...). Various data-driven schemes ought to work too.

https://codereview.chromium.org/1252933003/diff/40001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1252933003/diff/40001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:7874: Vector<CSSValueID>
paintTypeList;
Giving this inline-capacity (3) seems reasonable.

https://codereview.chromium.org/1252933003/diff/40001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:7875:
RefPtrWillBeRawPtr<CSSPrimitiveValue> fill;
IIRC, when these are RawPtr, they won't initialize to nullptr (like if they're
RefPtr), so these likely need "= nullptr" aded to them.

https://codereview.chromium.org/1252933003/diff/40001/Source/core/style/SVGCo...
File Source/core/style/SVGComputedStyle.cpp (right):

https://codereview.chromium.org/1252933003/diff/40001/Source/core/style/SVGCo...
Source/core/style/SVGComputedStyle.cpp:259: pt = (((PT_MARKERS <<
kPaintOrderBitwidth) | PT_STROKE) << kPaintOrderBitwidth) | PT_FILL;
This doesn't read very well IMO, either a helper:

unsigned paintOrderSequence(... first, ... second, ... third)
{
    // pack somehow
}
...
pt = paintOrderSequence(PT_FILL, PT_STROKE, PT_MARKERS);

or data-driven:

static const unsigned paintOrderSequence[] = {
    <packing helper>,
    ...
};
unsigned pt = paintOrderSequence[paintOrder()];

Hopefully the generated code is similar though. Making for lower visibility of
kPaintOrderBitwidth and make the sequence read in the "right" order would be the
goal IMO.

[Shanmuga Pandi, 2015-07-31 08:44:50]

Done the changes as per comments. 
Please check.
Thanks

https://codereview.chromium.org/1252933003/diff/40001/Source/core/css/Compute...
File Source/core/css/ComputedStyleCSSValueMapping.cpp (right):

https://codereview.chromium.org/1252933003/diff/40001/Source/core/css/Compute...
Source/core/css/ComputedStyleCSSValueMapping.cpp:1194: case
PaintOrderFillStrokeMarkers:
On 2015/07/30 14:39:36, fs (ooo) wrote:
> Eep! This looks like large (code) footprint =( - could pass const
> SVGComputedStyle& and essentially reuse the existing code I think (maybe a tad
> ugly, but...). Various data-driven schemes ought to work too.

Done.

https://codereview.chromium.org/1252933003/diff/40001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1252933003/diff/40001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:7874: Vector<CSSValueID>
paintTypeList;
On 2015/07/30 14:39:37, fs (ooo) wrote:
> Giving this inline-capacity (3) seems reasonable.

Done.

https://codereview.chromium.org/1252933003/diff/40001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:7875:
RefPtrWillBeRawPtr<CSSPrimitiveValue> fill;
On 2015/07/30 14:39:37, fs (ooo) wrote:
> IIRC, when these are RawPtr, they won't initialize to nullptr (like if they're
> RefPtr), so these likely need "= nullptr" aded to them.

Done.

https://codereview.chromium.org/1252933003/diff/40001/Source/core/style/SVGCo...
File Source/core/style/SVGComputedStyle.cpp (right):

https://codereview.chromium.org/1252933003/diff/40001/Source/core/style/SVGCo...
Source/core/style/SVGComputedStyle.cpp:259: pt = (((PT_MARKERS <<
kPaintOrderBitwidth) | PT_STROKE) << kPaintOrderBitwidth) | PT_FILL;
On 2015/07/30 14:39:37, fs (ooo) wrote:
> This doesn't read very well IMO, either a helper:
> 
> unsigned paintOrderSequence(... first, ... second, ... third)
> {
>     // pack somehow
> }
> ...
> pt = paintOrderSequence(PT_FILL, PT_STROKE, PT_MARKERS);
> 
> or data-driven:
> 
> static const unsigned paintOrderSequence[] = {
>     <packing helper>,
>     ...
> };
> unsigned pt = paintOrderSequence[paintOrder()];
> 
> Hopefully the generated code is similar though. Making for lower visibility of
> kPaintOrderBitwidth and make the sequence read in the "right" order would be
the
> goal IMO.

Done.

[fs, 2015-07-31 13:27:38]

lgtm

[Shanmuga Pandi, 2015-08-03 05:14:06]

The CQ bit was checked by shanmuga.m@samsung.com

[commit-bot: I haz the power, 2015-08-03 05:14:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1252933003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1252933003/60001

[commit-bot: I haz the power, 2015-08-03 07:01:16]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199879