Support 'paint-order' from SVG2.

Support 'paint-order' from SVG2.

R=pdr@chromium.org
BUG=223766
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156571

[Stephen Chennney, 2013-05-03 12:55:50]

This needs Intent to Implement email and approval. I don't see any problem at
all with the feature, but we need to run through the process.

[f(malita), 2013-05-03 14:36:25]

Thanks for implementing this Erik.

https://codereview.chromium.org/14907011/diff/1/Source/core/rendering/style/S...
File Source/core/rendering/style/SVGRenderStyle.cpp (right):

https://codereview.chromium.org/14907011/diff/1/Source/core/rendering/style/S...
Source/core/rendering/style/SVGRenderStyle.cpp:305: return PO_MARKERS;
Is there a better way to encode these internally?

For example: a base 4 3-digit number, where each digit is 0/NONE, 1/FILL,
2/STROKE, 3/MARKERS. Then we can avoid testing the whole value space and instead
test one digit at a time and shift. And if we assign EPaintOrder values
carefully, we might even be able to cast this number directly to that enum.

[pdr., 2013-05-03 17:58:29]

I like where this is going! Really top-notch work, thank you for tackling it.

https://codereview.chromium.org/14907011/diff/1/LayoutTests/svg/W3C-SVG2/pain...
File LayoutTests/svg/W3C-SVG2/paintorder.svg (right):

https://codereview.chromium.org/14907011/diff/1/LayoutTests/svg/W3C-SVG2/pain...
LayoutTests/svg/W3C-SVG2/paintorder.svg:1: <svg viewBox="-10 -10 280 220"
xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
Typically the tests in the W3C-*/ directories are official tests from the W3C,
but that's not a fixed rule by any means. If you plan to upstream this test to
the W3C, this is a great location. Otherwise, I would suggest putting this in
LayoutTests/svg/paintorder, but this is up to your judgement.

Could we add an additional test for dynamic values, to show that changing the
paint order causes the appropriate repaint to occur? There should be examples of
this type of test in LayoutTests/svg/repaint.

https://codereview.chromium.org/14907011/diff/1/Source/core/css/CSSParser.h
File Source/core/css/CSSParser.h (right):

https://codereview.chromium.org/14907011/diff/1/Source/core/css/CSSParser.h#n...
Source/core/css/CSSParser.h:221: bool parsePaintOrderParameter(CSSValueList*
parsedValues, CSSParserValue* value, PaintOrderState& paint_order);
Can you make these two functions const?

PassRefPtr<CSSValue> parsePaintOrder() const;
bool parsePaintOrderParameter(CSSValueList* parsedValues, CSSParserValue* value,
PaintOrderState
     & paint_order) const;

https://codereview.chromium.org/14907011/diff/1/Source/core/css/SVGCSSParser.cpp
File Source/core/css/SVGCSSParser.cpp (right):

https://codereview.chromium.org/14907011/diff/1/Source/core/css/SVGCSSParser....
Source/core/css/SVGCSSParser.cpp:224: case CSSPropertyPaintOrder:
When adding a property, you need to add an entry to the big switch statement in
UseCounter.cpp as well.

https://codereview.chromium.org/14907011/diff/1/Source/core/css/SVGCSSParser....
Source/core/css/SVGCSSParser.cpp:227: else if (parsedValue = parsePaintOrder())
You'll need to use extra parenthesis here to make clang happy:

"else if ((parsedValue = parsePaintOrder()))"

https://codereview.chromium.org/14907011/diff/1/Source/core/css/SVGCSSParser....
Source/core/css/SVGCSSParser.cpp:390: for ( ; valid && value; value =
m_valueList->next())
Style nit (here, and elsewhere): paintOrder instead of paint_order.

Suggestion: I think this loop would be cleaner written as:
do {
    if (valid = parsePaintOrderParameter(parsedValues.get(), value,
paint_order))
        parsedValues->append(CSSPrimitiveValue::createIdentifier(value->id));
} while (valid && (value = m_valueList->next()));

https://codereview.chromium.org/14907011/diff/1/Source/core/css/SVGCSSStyleSe...
File Source/core/css/SVGCSSStyleSelector.cpp (right):

https://codereview.chromium.org/14907011/diff/1/Source/core/css/SVGCSSStyleSe...
Source/core/css/SVGCSSStyleSelector.cpp:113: //if (item->isPrimitiveValue())
Oops :)

https://codereview.chromium.org/14907011/diff/1/Source/core/css/SVGCSSStyleSe...
Source/core/css/SVGCSSStyleSelector.cpp:717: if (value->isValueList())
Style nit: no braces needed for single-line if/else bodies.

https://codereview.chromium.org/14907011/diff/1/Source/core/rendering/style/S...
File Source/core/rendering/style/SVGRenderStyle.cpp (right):

https://codereview.chromium.org/14907011/diff/1/Source/core/rendering/style/S...
Source/core/rendering/style/SVGRenderStyle.cpp:305: return PO_MARKERS;
On 2013/05/03 14:36:25, Florin Malita wrote:
> Is there a better way to encode these internally?
> 
> For example: a base 4 3-digit number, where each digit is 0/NONE, 1/FILL,
> 2/STROKE, 3/MARKERS. Then we can avoid testing the whole value space and
instead
> test one digit at a time and shift. And if we assign EPaintOrder values
> carefully, we might even be able to cast this number directly to that enum.

+1 to Florin's idea.
Using an int here is not easy to understand, especially at the callsites (e.g.,
in RenderSVGShape). We should design this to make the code in RenderSVGShape
easy to understand without having to refer here to see how it works. It would
also be nice to not have to iterate over every permutation.

https://codereview.chromium.org/14907011/diff/1/Source/core/rendering/style/S...
File Source/core/rendering/style/SVGRenderStyleDefs.h (right):

https://codereview.chromium.org/14907011/diff/1/Source/core/rendering/style/S...
Source/core/rendering/style/SVGRenderStyleDefs.h:97: enum EPaintOrder {
The style checker will complain about these names not following the style guide.
Just ignore the checker in this case; these names are fine.

[pdr., 2013-05-03 22:59:08]

Two more quick notes:

Because we aren't shipping this quite yet, we will need to add this feature
behind a flag.

Do you mind updating the description on the issue to be more detailed? Our style
is to not use many code comments so I like putting enough information there so
when we come back a year from now it will be clear what we were thinking :)

[pdr., 2013-05-06 04:52:14]

On 2013/05/03 22:59:08, pdr wrote:
> Because we aren't shipping this quite yet, we will need to add this feature
> behind a flag.

This feature flag business used to be a dreary process but eseidel is about to
make it a thousand times easier with crbug.com/237740. If you can wait a couple
days, you should be able to use his new and easy-to-use feature flag feature.

[pdr., 2013-05-08 18:44:10]

On 2013/05/06 04:52:14, pdr wrote:
> On 2013/05/03 22:59:08, pdr wrote:
> > Because we aren't shipping this quite yet, we will need to add this feature
> > behind a flag.
> 
> This feature flag business used to be a dreary process but eseidel is about to
> make it a thousand times easier with crbug.com/237740. If you can wait a
couple
> days, you should be able to use his new and easy-to-use feature flag feature.

This just landed, for more information see:
https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/blink-dev/...

[pdr., 2013-05-31 18:40:49]

Erik, this was such an awesome patch but there hasn't been any activity in a
month. Is there anything I can do to help you land it? Alternatively, may I take
this and land it myself?

[Erik Dahlström (inactive), 2013-06-03 08:05:51]

On 2013/05/31 18:40:49, pdr wrote:
> Erik, this was such an awesome patch but there hasn't been any activity in a
> month. Is there anything I can do to help you land it? Alternatively, may I
take
> this and land it myself?

Just haven't had the time for it, I'll try to get it sorted this week.

[Erik Dahlström (inactive), 2013-06-03 15:58:24]

https://codereview.chromium.org/14907011/diff/1/Source/core/css/SVGCSSParser.cpp
File Source/core/css/SVGCSSParser.cpp (right):

https://codereview.chromium.org/14907011/diff/1/Source/core/css/SVGCSSParser....
Source/core/css/SVGCSSParser.cpp:224: case CSSPropertyPaintOrder:
On 2013/05/03 17:58:29, pdr wrote:
> When adding a property, you need to add an entry to the big switch statement
in
> UseCounter.cpp as well.

is that really useful? it's a new feature, and UseCounter seems to be more for
deprecation. What I mean is, it's not going to be used much yet, so I'd think
it's kind of pointless to add it at this stage.

https://codereview.chromium.org/14907011/diff/1/Source/core/rendering/style/S...
File Source/core/rendering/style/SVGRenderStyle.cpp (right):

https://codereview.chromium.org/14907011/diff/1/Source/core/rendering/style/S...
Source/core/rendering/style/SVGRenderStyle.cpp:305: return PO_MARKERS;
On 2013/05/03 17:58:29, pdr wrote:
> On 2013/05/03 14:36:25, Florin Malita wrote:
> > Is there a better way to encode these internally?
> > 
> > For example: a base 4 3-digit number, where each digit is 0/NONE, 1/FILL,
> > 2/STROKE, 3/MARKERS. Then we can avoid testing the whole value space and
> instead
> > test one digit at a time and shift. And if we assign EPaintOrder values
> > carefully, we might even be able to cast this number directly to that enum.
> 
> +1 to Florin's idea.
> Using an int here is not easy to understand, especially at the callsites
(e.g.,
> in RenderSVGShape). We should design this to make the code in RenderSVGShape
> easy to understand without having to refer here to see how it works. It would
> also be nice to not have to iterate over every permutation.

If we can live with normalizing the paintorder here then that becomes possible.
By normalizing I mean computing the finally used value such that it will always
consist of three parts (instead of e.g one, like "fill" or "normal"). That does
mean that it becomes impossible to map an EPaintOrder value back to the exact
CSSValue(s) that generated it. The computed value should be "as specified"
according to the spec, and if I look at what Firefox nightly is doing it does
return something different than what was specified if I call getComputedStyle
and fetch the paint-order value. It would naturally simplify the implementation
to have a normalized representation here, but the question is if it would be
correct. Thoughts?

[pdr., 2013-06-04 05:47:35]

On 2013/06/03 15:58:24, Erik Dahlström wrote:
>
https://codereview.chromium.org/14907011/diff/1/Source/core/css/SVGCSSParser.cpp
> File Source/core/css/SVGCSSParser.cpp (right):
> 
> is that really useful? it's a new feature, and UseCounter seems to be more for
> deprecation. What I mean is, it's not going to be used much yet, so I'd think
> it's kind of pointless to add it at this stage.

Good catch, I have no idea what I was thinking.

> 
>
https://codereview.chromium.org/14907011/diff/1/Source/core/rendering/style/S...
> File Source/core/rendering/style/SVGRenderStyle.cpp (right):
> 
>
https://codereview.chromium.org/14907011/diff/1/Source/core/rendering/style/S...
> Source/core/rendering/style/SVGRenderStyle.cpp:305: return PO_MARKERS;
> On 2013/05/03 17:58:29, pdr wrote:
> > On 2013/05/03 14:36:25, Florin Malita wrote:
> > > Is there a better way to encode these internally?
> > > 
> > > For example: a base 4 3-digit number, where each digit is 0/NONE, 1/FILL,
> > > 2/STROKE, 3/MARKERS. Then we can avoid testing the whole value space and
> > instead
> > > test one digit at a time and shift. And if we assign EPaintOrder values
> > > carefully, we might even be able to cast this number directly to that
enum.
> > 
> > +1 to Florin's idea.
> > Using an int here is not easy to understand, especially at the callsites
> (e.g.,
> > in RenderSVGShape). We should design this to make the code in RenderSVGShape
> > easy to understand without having to refer here to see how it works. It
would
> > also be nice to not have to iterate over every permutation.
> 
> If we can live with normalizing the paintorder here then that becomes
possible.
> By normalizing I mean computing the finally used value such that it will
always
> consist of three parts (instead of e.g one, like "fill" or "normal"). That
does
> mean that it becomes impossible to map an EPaintOrder value back to the exact
> CSSValue(s) that generated it. The computed value should be "as specified"
> according to the spec, and if I look at what Firefox nightly is doing it does
> return something different than what was specified if I call getComputedStyle
> and fetch the paint-order value. It would naturally simplify the
implementation
> to have a normalized representation here, but the question is if it would be
> correct. Thoughts?

We're behind a flag so I think this is very reasonable.

Because it's a violation of the spec, I'd like to have some resolution on
www-svg before we turn this on, but I don't think we need to gate landing this
patch on that.

[Erik Dahlström (inactive), 2013-06-25 14:48:21]

On 2013/06/04 05:47:35, pdr wrote:
> On 2013/06/03 15:58:24, Erik Dahlström wrote:
> >
>
https://codereview.chromium.org/14907011/diff/1/Source/core/css/SVGCSSParser.cpp
> > File Source/core/css/SVGCSSParser.cpp (right):
> > 
> > is that really useful? it's a new feature, and UseCounter seems to be more
for
> > deprecation. What I mean is, it's not going to be used much yet, so I'd
think
> > it's kind of pointless to add it at this stage.
> 
> Good catch, I have no idea what I was thinking.
> 
> > 
> >
>
https://codereview.chromium.org/14907011/diff/1/Source/core/rendering/style/S...
> > File Source/core/rendering/style/SVGRenderStyle.cpp (right):
> > 
> >
>
https://codereview.chromium.org/14907011/diff/1/Source/core/rendering/style/S...
> > Source/core/rendering/style/SVGRenderStyle.cpp:305: return PO_MARKERS;
> > On 2013/05/03 17:58:29, pdr wrote:
> > > On 2013/05/03 14:36:25, Florin Malita wrote:
> > > > Is there a better way to encode these internally?
> > > > 
> > > > For example: a base 4 3-digit number, where each digit is 0/NONE,
1/FILL,
> > > > 2/STROKE, 3/MARKERS. Then we can avoid testing the whole value space and
> > > instead
> > > > test one digit at a time and shift. And if we assign EPaintOrder values
> > > > carefully, we might even be able to cast this number directly to that
> enum.
> > > 
> > > +1 to Florin's idea.
> > > Using an int here is not easy to understand, especially at the callsites
> > (e.g.,
> > > in RenderSVGShape). We should design this to make the code in
RenderSVGShape
> > > easy to understand without having to refer here to see how it works. It
> would
> > > also be nice to not have to iterate over every permutation.
> > 
> > If we can live with normalizing the paintorder here then that becomes
> possible.
> > By normalizing I mean computing the finally used value such that it will
> always
> > consist of three parts (instead of e.g one, like "fill" or "normal"). That
> does
> > mean that it becomes impossible to map an EPaintOrder value back to the
exact
> > CSSValue(s) that generated it. The computed value should be "as specified"
> > according to the spec, and if I look at what Firefox nightly is doing it
does
> > return something different than what was specified if I call
getComputedStyle
> > and fetch the paint-order value. It would naturally simplify the
> implementation
> > to have a normalized representation here, but the question is if it would be
> > correct. Thoughts?
> 
> We're behind a flag so I think this is very reasonable.
> 
> Because it's a violation of the spec, I'd like to have some resolution on
> www-svg before we turn this on, but I don't think we need to gate landing this
> patch on that.

Ping. The second patchset hopefully addresses most of the issues, and has been
waiting for review for a couple of weeks now.

[pdr., 2013-06-25 15:30:44]

Love it!

Lets do one more round before the final LGTM.

Question: can you add a test of the order that matches the conclusion on
www-svg?

By the way, please make lots of noise if you aren't getting a review in even 24
hours. This is cool code and waiting for a review is a buzz-kill (not to
mention, patch merge hell). It's not at all obvious, but when you upload a new
patch the reviewers are not emailed; you have to click "Publish and Mail
comments" and then click "Publish All My Drafts".

https://codereview.chromium.org/14907011/diff/19001/Source/core/css/SVGCSSCom...
File Source/core/css/SVGCSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/14907011/diff/19001/Source/core/css/SVGCSSCom...
Source/core/css/SVGCSSComputedStyleDeclaration.cpp:67: switch (paintOrderType) {
Nit: indent inside the switch statement.

https://codereview.chromium.org/14907011/diff/19001/Source/core/css/SVGCSSCom...
Source/core/css/SVGCSSComputedStyleDeclaration.cpp:78: }
Nit: Our style is to leave the while loop on the same line as the ending brace.

https://codereview.chromium.org/14907011/diff/19001/Source/core/css/SVGCSSPar...
File Source/core/css/SVGCSSParser.cpp (right):

https://codereview.chromium.org/14907011/diff/19001/Source/core/css/SVGCSSPar...
Source/core/css/SVGCSSParser.cpp:395: bool seen[] = { false, false, false };
Style nit (fix at your discretion)

I think this function would be cleaner with 3 bools instead of an array:
seenFill, seenStroke, and seenMarkers. This way, your for loop at the end would
turn into 3 if statements.

https://codereview.chromium.org/14907011/diff/19001/Source/core/css/SVGCSSPar...
Source/core/css/SVGCSSParser.cpp:427: for (int i = 0; i < 3; i++)
Nit: if you choose to keep this, because the body of the for loop is multi-line,
can you add braces?

https://codereview.chromium.org/14907011/diff/19001/Source/core/page/RuntimeE...
File Source/core/page/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/14907011/diff/19001/Source/core/page/RuntimeE...
Source/core/page/RuntimeEnabledFeatures.in:64: SVG2 status=experimental
Do you mind calling this just PaintOrder or similar? We have lots of SVG2
features already and I worry people will get confused.

https://codereview.chromium.org/14907011/diff/19001/Source/core/rendering/sty...
File Source/core/rendering/style/SVGRenderStyle.cpp (right):

https://codereview.chromium.org/14907011/diff/19001/Source/core/rendering/sty...
Source/core/rendering/style/SVGRenderStyle.cpp:232: ASSERT(index < 3);
Can you turn this 3 into a constant? I worry that we'll forget what it's for in
a years time.

[Erik Dahlström (inactive), 2013-07-19 15:33:58]

Added some more tests, and discovered a few issues in the process.

Just to let you know, I'll be on vacation for a few weeks starting today.

[pdr., 2013-07-19 17:35:48]

LGTM

https://codereview.chromium.org/14907011/diff/32001/Source/core/css/SVGCSSVal...
File Source/core/css/SVGCSSValueKeywords.in (right):

https://codereview.chromium.org/14907011/diff/32001/Source/core/css/SVGCSSVal...
Source/core/css/SVGCSSValueKeywords.in:281: // markers
Did you mean to comment this out?

[commit-bot: I haz the power, 2013-07-19 17:36:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ed@opera.com/14907011/32001

[commit-bot: I haz the power, 2013-07-19 17:36:32]

Can't process patch for file LayoutTests/svg/paintorder/paintorder-expected.svg.
Binary file is empty. Maybe the file wasn't uploaded in the first place?

[pdr., 2013-07-19 17:59:27]

On 2013/07/19 17:36:32, I haz the power (commit-bot) wrote:
> Can't process patch for file
LayoutTests/svg/paintorder/paintorder-expected.svg.
> Binary file is empty. Maybe the file wasn't uploaded in the first place?

Is paintorder-expected.svg marked as binary? The commit queue should be able to
process this patch.

[Erik Dahlström (inactive), 2013-08-19 07:10:22]

On 2013/07/19 17:35:48, pdr wrote:
> LGTM
> 
>
https://codereview.chromium.org/14907011/diff/32001/Source/core/css/SVGCSSVal...
> File Source/core/css/SVGCSSValueKeywords.in (right):
> 
>
https://codereview.chromium.org/14907011/diff/32001/Source/core/css/SVGCSSVal...
> Source/core/css/SVGCSSValueKeywords.in:281: // markers
> Did you mean to comment this out?

Yes, only to mark that it is a value keyword of 'paint-order' (similar to other
props). Unfortunately the separate SVGCSSValueKeywords.in file didn't work
fully, so the keyword had to be put in CSSValueKeywords.in file instead (some
processing script didn't work properly, caused a compile error IIRC).

[Erik Dahlström (inactive), 2013-08-19 07:16:46]

On 2013/07/19 17:59:27, pdr wrote:
> On 2013/07/19 17:36:32, I haz the power (commit-bot) wrote:
> > Can't process patch for file
> LayoutTests/svg/paintorder/paintorder-expected.svg.
> > Binary file is empty. Maybe the file wasn't uploaded in the first place?
> 
> Is paintorder-expected.svg marked as binary? The commit queue should be able
to
> process this patch.

I have not intentionally marked it as binary. It was a textfile before, and I
have only moved it to a new directory (in patchset #2). Maybe that's why svn got
confused?

[pdr., 2013-08-21 05:39:20]

On 2013/08/19 07:16:46, Erik Dahlström wrote:
> I have not intentionally marked it as binary. It was a textfile before, and I
> have only moved it to a new directory (in patchset #2). Maybe that's why svn
got
> confused?

You were only gone for a month but our infrastructure has teched up quite a bit
(still not acceptable, but closer). I think the bots should be happy if you
reupload :)

[commit-bot: I haz the power, 2013-08-21 08:21:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ed@opera.com/14907011/32001

[commit-bot: I haz the power, 2013-08-21 08:22:05]

Can't process patch for file LayoutTests/svg/paintorder/paintorder-expected.svg.
Binary file is empty. Maybe the file wasn't uploaded in the first place?

[commit-bot: I haz the power, 2013-08-21 08:26:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ed@opera.com/14907011/32001

[commit-bot: I haz the power, 2013-08-21 08:26:28]

Can't process patch for file LayoutTests/svg/paintorder/paintorder-expected.svg.
Binary file is empty. Maybe the file wasn't uploaded in the first place?

[Erik Dahlström (inactive), 2013-08-21 09:30:20]

On 2013/08/21 05:39:20, pdr wrote:
> On 2013/08/19 07:16:46, Erik Dahlström wrote:
> > I have not intentionally marked it as binary. It was a textfile before, and
I
> > have only moved it to a new directory (in patchset #2). Maybe that's why svn
> got
> > confused?
> 
> You were only gone for a month but our infrastructure has teched up quite a
bit
> (still not acceptable, but closer). I think the bots should be happy if you
> reupload :)

The file is most definitely in patchset #3, see e.g
https://codereview.chromium.org/download/issue14907011_32001.diff.

I've retried the "commit" checkbox in the patchset twice now, same result. The
wiki page for the commit queue didn't have any info regarding this type of
issue.

Please help me out here? :)

[pdr., 2013-08-21 19:18:47]

On 2013/08/21 09:30:20, Erik Dahlström wrote:
> On 2013/08/21 05:39:20, pdr wrote:
> > On 2013/08/19 07:16:46, Erik Dahlström wrote:
> > > I have not intentionally marked it as binary. It was a textfile before,
and
> I
> > > have only moved it to a new directory (in patchset #2). Maybe that's why
svn
> > got
> > > confused?
> > 
> > You were only gone for a month but our infrastructure has teched up quite a
> bit
> > (still not acceptable, but closer). I think the bots should be happy if you
> > reupload :)
> 
> The file is most definitely in patchset #3, see e.g
> https://codereview.chromium.org/download/issue14907011_32001.diff.
> 
> I've retried the "commit" checkbox in the patchset twice now, same result. The
> wiki page for the commit queue didn't have any info regarding this type of
> issue.
> 
> Please help me out here? :)

Looks like I have to land this for you. I'll land this today.

[pdr., 2013-08-22 05:16:46]

On 2013/08/21 19:18:47, pdr wrote:
> On 2013/08/21 09:30:20, Erik Dahlström wrote:
> > On 2013/08/21 05:39:20, pdr wrote:
> > > On 2013/08/19 07:16:46, Erik Dahlström wrote:
> > > > I have not intentionally marked it as binary. It was a textfile before,
> and
> > I
> > > > have only moved it to a new directory (in patchset #2). Maybe that's why
> svn
> > > got
> > > > confused?
> > > 
> > > You were only gone for a month but our infrastructure has teched up quite
a
> > bit
> > > (still not acceptable, but closer). I think the bots should be happy if
you
> > > reupload :)
> > 
> > The file is most definitely in patchset #3, see e.g
> > https://codereview.chromium.org/download/issue14907011_32001.diff.
> > 
> > I've retried the "commit" checkbox in the patchset twice now, same result.
The
> > wiki page for the commit queue didn't have any info regarding this type of
> > issue.
> > 
> > Please help me out here? :)
> 
> Looks like I have to land this for you. I'll land this today.

This turned out to not be the easiest patch to rebase.

I've rebased the patch but there were real changes involved:
1) I needed to rewrite most of the property wiring due to a move to
StyleBuilderCustom.cpp, the removal of the identifier value, and the removal of
SVGStyledElement (per the SVG2 spec).
2) The repaint test in the original patch was not correct; repaint tests should
have platform-specific expectations. I've added an entry to TestExpectations to
automagically generate these expectations, and removed
repaint-paintorder-expected.svg.
3) Lots of minor merge failures.

I'm not comfortable landing this under your name with big changes like this.
Can you review the changes I made? (review the reviewers :)
If you like the updated patch, add a comment saying "LGTM" then check the
commit checkbox to land the patch.

[Erik Dahlström (inactive), 2013-08-22 13:24:59]

On 2013/08/22 05:16:46, pdr wrote:
> On 2013/08/21 19:18:47, pdr wrote:
> > On 2013/08/21 09:30:20, Erik Dahlström wrote:
> > > On 2013/08/21 05:39:20, pdr wrote:
> > > > On 2013/08/19 07:16:46, Erik Dahlström wrote:
> > > > > I have not intentionally marked it as binary. It was a textfile
before,
> > and
> > > I
> > > > > have only moved it to a new directory (in patchset #2). Maybe that's
why
> > svn
> > > > got
> > > > > confused?
> > > > 
> > > > You were only gone for a month but our infrastructure has teched up
quite
> a
> > > bit
> > > > (still not acceptable, but closer). I think the bots should be happy if
> you
> > > > reupload :)
> > > 
> > > The file is most definitely in patchset #3, see e.g
> > > https://codereview.chromium.org/download/issue14907011_32001.diff.
> > > 
> > > I've retried the "commit" checkbox in the patchset twice now, same result.
> The
> > > wiki page for the commit queue didn't have any info regarding this type of
> > > issue.
> > > 
> > > Please help me out here? :)
> > 
> > Looks like I have to land this for you. I'll land this today.
> 
> This turned out to not be the easiest patch to rebase.
> 
> I've rebased the patch but there were real changes involved:
> 1) I needed to rewrite most of the property wiring due to a move to
> StyleBuilderCustom.cpp, the removal of the identifier value, and the removal
of
> SVGStyledElement (per the SVG2 spec).
> 2) The repaint test in the original patch was not correct; repaint tests
should
> have platform-specific expectations. I've added an entry to TestExpectations
to
> automagically generate these expectations, and removed
> repaint-paintorder-expected.svg.
> 3) Lots of minor merge failures.
> 
> I'm not comfortable landing this under your name with big changes like this.
> Can you review the changes I made? (review the reviewers :)
> If you like the updated patch, add a comment saying "LGTM" then check the
> commit checkbox to land the patch.

I rebased to fix a minor merge mistake in patchset #4, an off-by-one in
UseCounter.cpp.

LGTM.

[commit-bot: I haz the power, 2013-08-22 13:25:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ed@opera.com/14907011/71001

[commit-bot: I haz the power, 2013-08-22 13:26:02]

Can't process patch for file LayoutTests/svg/paintorder/paintorder-expected.svg.
Binary file is empty. Maybe the file wasn't uploaded in the first place?

[pdr., 2013-08-22 19:20:34]

Committed patchset #5 manually as r156571 (presubmit successful).