[SkSVGDevice] Initial text support

[SkSVGDevice] Initial text support

R=reed@google.com,mtklein@google.com

Committed: https://skia.googlesource.com/skia/+/fe3f260ff9b6b3f4c7c33f2b20502b281baab689

[mtklein, 2015-02-03 20:51:09]

https://codereview.chromium.org/899683002/diff/1/experimental/svg/SkSVGDevice...
File experimental/svg/SkSVGDevice.cpp (right):

https://codereview.chromium.org/899683002/diff/1/experimental/svg/SkSVGDevice...
experimental/svg/SkSVGDevice.cpp:61: 
Worth asserting byteLen alignment for each of these cases (2 byte, 1 byte, 2
byte, 4 byte) ?

https://codereview.chromium.org/899683002/diff/1/experimental/svg/SkSVGDevice...
experimental/svg/SkSVGDevice.cpp:402: void SkSVGDevice::AddFontAttributes(const
SkPaint& paint, AutoElement* elem) {
void AutoElement::addFontAttributes(const SkPaint&)? 

Or even do it as part of the AutoElement constructor?  I guess you want it to be
a separate call so that it's only done for text-y draws?

https://codereview.chromium.org/899683002/diff/1/experimental/svg/SkSVGDevice...
experimental/svg/SkSVGDevice.cpp:440: SkScalar x = offset.x() + pos[i *
scalarsPerPos];
Are offset.x() and offset.y() "dx" and "dy"?  If so, seems like you can have SVG
do the deltas from offset for you.

https://codereview.chromium.org/899683002/diff/1/experimental/svg/SkSVGDevice...
experimental/svg/SkSVGDevice.cpp:441: SkScalar y = offset.y() + (scalarsPerPos >
1 ? pos[i * scalarsPerPos + 1] : 0);
When scalarsPerPos == 1, can we just use a single y value, or do we have to have
matching x and y pairs?

[f(malita), 2015-02-03 22:51:25]

https://codereview.chromium.org/899683002/diff/1/experimental/svg/SkSVGDevice...
File experimental/svg/SkSVGDevice.cpp (right):

https://codereview.chromium.org/899683002/diff/1/experimental/svg/SkSVGDevice...
experimental/svg/SkSVGDevice.cpp:61: 
On 2015/02/03 20:51:08, mtklein wrote:
> Worth asserting byteLen alignment for each of these cases (2 byte, 1 byte, 2
> byte, 4 byte) ?

We can probably be even more opinionated and assert that byteLen == the exact
number of bytes consumed. Done.

https://codereview.chromium.org/899683002/diff/1/experimental/svg/SkSVGDevice...
experimental/svg/SkSVGDevice.cpp:402: void SkSVGDevice::AddFontAttributes(const
SkPaint& paint, AutoElement* elem) {
On 2015/02/03 20:51:08, mtklein wrote:
> void AutoElement::addFontAttributes(const SkPaint&)? 

Done.

> Or even do it as part of the AutoElement constructor?  I guess you want it to
be
> a separate call so that it's only done for text-y draws?

Yeah, this is only needed for drawText*.

https://codereview.chromium.org/899683002/diff/1/experimental/svg/SkSVGDevice...
experimental/svg/SkSVGDevice.cpp:440: SkScalar x = offset.x() + pos[i *
scalarsPerPos];
On 2015/02/03 20:51:08, mtklein wrote:
> Are offset.x() and offset.y() "dx" and "dy"?  If so, seems like you can have
SVG
> do the deltas from offset for you.

The syntactic rules are quirky though and this would require either a) dx/dy to
be a list matching the number of glyphs or b) nest the glyphs in a <tspan> while
dx/dy is applied to the parent <text>.

Since the output is quite verbose already, I think we should avoid this.

https://codereview.chromium.org/899683002/diff/1/experimental/svg/SkSVGDevice...
experimental/svg/SkSVGDevice.cpp:441: SkScalar y = offset.y() + (scalarsPerPos >
1 ? pos[i * scalarsPerPos + 1] : 0);
On 2015/02/03 20:51:08, mtklein wrote:
> When scalarsPerPos == 1, can we just use a single y value, or do we have to
have
> matching x and y pairs?

Yeah, I think that will work. Done.

[mtklein, 2015-02-03 23:56:09]

lgtm

https://codereview.chromium.org/899683002/diff/20001/experimental/svg/SkSVGDe...
File experimental/svg/SkSVGDevice.cpp (right):

https://codereview.chromium.org/899683002/diff/20001/experimental/svg/SkSVGDe...
experimental/svg/SkSVGDevice.cpp:453: 
SkASSERT(scalarsPerPos == 1 || scalarsPerPos == 2) somewhere in here?

[f(malita), 2015-02-04 00:45:43]

New patchsets have been uploaded after l-g-t-m from mtklein@google.com

[f(malita), 2015-02-04 00:46:14]

https://codereview.chromium.org/899683002/diff/20001/experimental/svg/SkSVGDe...
File experimental/svg/SkSVGDevice.cpp (right):

https://codereview.chromium.org/899683002/diff/20001/experimental/svg/SkSVGDe...
experimental/svg/SkSVGDevice.cpp:453: 
On 2015/02/03 23:56:08, mtklein wrote:
> SkASSERT(scalarsPerPos == 1 || scalarsPerPos == 2) somewhere in here?

Done.

[f(malita), 2015-02-04 00:46:31]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2015-02-04 00:47:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/899683002/40001

[commit-bot: I haz the power, 2015-02-04 00:47:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-04 00:47:19]

Presubmit check for 899683002-40001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com', 'djsollen@chromium.org', 'djsollen@google.com')

[f(malita), 2015-02-04 00:49:27]

fmalita@chromium.org changed reviewers:
+ bsalomon@google.com

[f(malita), 2015-02-04 00:49:54]

On 2015/02/04 00:47:19, I haz the power (commit-bot) wrote:
> Presubmit check for 899683002-40001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Since the CL is editing public API, you must have an LGTM from one of:
> (mailto:, mailto:, mailto:,
> mailto:, mailto:, mailto:)

Public API owners PTAL.

[reed1, 2015-02-04 01:05:23]

https://codereview.chromium.org/899683002/diff/40001/include/xml/SkXMLWriter.h
File include/xml/SkXMLWriter.h (right):

https://codereview.chromium.org/899683002/diff/40001/include/xml/SkXMLWriter....
include/xml/SkXMLWriter.h:30: void    addText(const SkString& text);
addText(const char text[], size_t length)

That would be a more consistent signature, since all of the others take text/len
instead of skstring.

[f(malita), 2015-02-04 01:35:25]

https://codereview.chromium.org/899683002/diff/40001/include/xml/SkXMLWriter.h
File include/xml/SkXMLWriter.h (right):

https://codereview.chromium.org/899683002/diff/40001/include/xml/SkXMLWriter....
include/xml/SkXMLWriter.h:30: void    addText(const SkString& text);
On 2015/02/04 01:05:23, reed1 wrote:
> addText(const char text[], size_t length)
> 
> That would be a more consistent signature, since all of the others take
text/len
> instead of skstring.

Done. I dropped the 'length' param to avoid converting back to an SkString in
SkXMLStreamWriter::onAddText (SkWStream::writeText expects a null-terminated C
string).

Some of the other methods also use C strings - WDYT?

[reed1, 2015-02-04 01:38:05]

On 2015/02/04 01:35:25, f(malita) wrote:
> https://codereview.chromium.org/899683002/diff/40001/include/xml/SkXMLWriter.h
> File include/xml/SkXMLWriter.h (right):
> 
>
https://codereview.chromium.org/899683002/diff/40001/include/xml/SkXMLWriter....
> include/xml/SkXMLWriter.h:30: void    addText(const SkString& text);
> On 2015/02/04 01:05:23, reed1 wrote:
> > addText(const char text[], size_t length)
> > 
> > That would be a more consistent signature, since all of the others take
> text/len
> > instead of skstring.
> 
> Done. I dropped the 'length' param to avoid converting back to an SkString in
> SkXMLStreamWriter::onAddText (SkWStream::writeText expects a null-terminated C
> string).
> 
> Some of the other methods also use C strings - WDYT?

Hmmm. It is unfortunate that the StreamWriter requires null-termination for,
what could be, large content.
The XML writer was somewhat deliberate in balancing convenience with efficiency:
- attr names are null-terminated for convenience, but these are usually small
- values take an explicit length, to not require nulls since they may be large

[reed1, 2015-02-04 01:39:02]

LGTM

I'll file a bug/request that we consider changing this and streamwriter to take
explicit length for addText.

[reed1, 2015-02-04 01:41:20]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-02-04 01:41:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/899683002/80001

[commit-bot: I haz the power, 2015-02-04 01:47:17]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/fe3f260ff9b6b3f4c7c33f2b20502b281baab689