Avoid one temporary copy on web fonts decoding

Avoid one temporary copy on web fonts decoding

Before this CL, we allocated a temporary SharedBuffer to store decoded
font data. Since the decoded data is only used to create SkTypeface we don't
need to have such abstraction. Create SkTypeface directly from the decoded
data. This CL also renamed and moved platform/fonts/opentype/OpenTypeSanitizer
to platform/fonts/WebFontDecoder for better naming.

BUG=616969
Committed: https://crrev.com/f4e652e1603fdc1c59afc235261648cac4d3ba1e
Cr-Commit-Position: refs/heads/master@{#397648}

[bashi, 2016-06-02 03:47:07]

Description was changed from

==========
Avoid one temporary copy on web fonts decoding

Before this CL, we allocated a temporary SharedBuffer to store decoded
font data. Since the decoded data is only used to create SkTypeface we don't
need to have such abstraction. Create SkTypeface directly from the decoded
data. This CL also renamed and removed platform/fonts/opentype/OpenTypeSanitizer
to platform/fonts/WebFontDecoder for better naming.
==========

to

==========
Avoid one temporary copy on web fonts decoding

Before this CL, we allocated a temporary SharedBuffer to store decoded
font data. Since the decoded data is only used to create SkTypeface we don't
need to have such abstraction. Create SkTypeface directly from the decoded
data. This CL also renamed and moved platform/fonts/opentype/OpenTypeSanitizer
to platform/fonts/WebFontDecoder for better naming.
==========

[bashi, 2016-06-02 04:54:07]

bashi@chromium.org changed reviewers:
+ ksakamoto@chromium.org

[bashi, 2016-06-02 04:54:07]

Sakamoto-san, PTAL?

I don't expect this CL contributes to reduce memory consumption and/or
performance improvement but I think avoiding unnecessary copy is a good thing :)

[Kunihiko Sakamoto, 2016-06-02 05:50:34]

lgtm

https://codereview.chromium.org/2032943002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp (right):

https://codereview.chromium.org/2032943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp:196: return nullptr;
Not directly related to this CL, but can we set some error string that indicates
the decoded font was not usable by platform?

[bashi, 2016-06-02 07:22:26]

bashi@chromium.org changed reviewers:
+ eae@chromium.org

[bashi, 2016-06-02 07:22:27]

Thanks for review, sakamoto-san!

eae@, could you do owner's review?

https://codereview.chromium.org/2032943002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp (right):

https://codereview.chromium.org/2032943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp:196: return nullptr;
On 2016/06/02 05:50:34, Kunihiko Sakamoto wrote:
> Not directly related to this CL, but can we set some error string that
indicates
> the decoded font was not usable by platform?

Done.

[eae, 2016-06-02 14:51:18]

LGTM

https://codereview.chromium.org/2032943002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp (right):

https://codereview.chromium.org/2032943002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp:163: static const
size_t maxWebFontSize = 30 * 1024 * 1024; // 30 MB
Is this limit documented somewhere? 30 MB seems pretty low given that there are
fonts that cover the entire unicode baseplane.

[bashi, 2016-06-02 23:06:56]

Thanks for review!

https://codereview.chromium.org/2032943002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp (right):

https://codereview.chromium.org/2032943002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp:163: static const
size_t maxWebFontSize = 30 * 1024 * 1024; // 30 MB
On 2016/06/02 14:51:17, eae wrote:
> Is this limit documented somewhere? 30 MB seems pretty low given that there
are
> fonts that cover the entire unicode baseplane.

I don't think this limit is documented somewhere. Probably it's an arbitrary
limit someone added in the past. We may want to increase (or just remove) the
limit but I'll keep this limit in this CL.

[eae, 2016-06-02 23:13:57]

On 2016/06/02 23:06:56, bashi1 wrote:
> Thanks for review!
> 
>
https://codereview.chromium.org/2032943002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp (right):
> 
>
https://codereview.chromium.org/2032943002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp:163: static const
> size_t maxWebFontSize = 30 * 1024 * 1024; // 30 MB
> On 2016/06/02 14:51:17, eae wrote:
> > Is this limit documented somewhere? 30 MB seems pretty low given that there
> are
> > fonts that cover the entire unicode baseplane.
> 
> I don't think this limit is documented somewhere. Probably it's an arbitrary
> limit someone added in the past. We may want to increase (or just remove) the
> limit but I'll keep this limit in this CL.

Sounds good. At the very least we should make sure it's documented somewhere. No
point in blocking this CL on it though.

[bashi, 2016-06-03 00:28:52]

Description was changed from

==========
Avoid one temporary copy on web fonts decoding

Before this CL, we allocated a temporary SharedBuffer to store decoded
font data. Since the decoded data is only used to create SkTypeface we don't
need to have such abstraction. Create SkTypeface directly from the decoded
data. This CL also renamed and moved platform/fonts/opentype/OpenTypeSanitizer
to platform/fonts/WebFontDecoder for better naming.
==========

to

==========
Avoid one temporary copy on web fonts decoding

Before this CL, we allocated a temporary SharedBuffer to store decoded
font data. Since the decoded data is only used to create SkTypeface we don't
need to have such abstraction. Create SkTypeface directly from the decoded
data. This CL also renamed and moved platform/fonts/opentype/OpenTypeSanitizer
to platform/fonts/WebFontDecoder for better naming.

BUG=616969
==========

[bashi, 2016-06-03 00:33:19]

On 2016/06/02 23:13:57, eae wrote:
> On 2016/06/02 23:06:56, bashi1 wrote:
> > Thanks for review!
> > 
> >
>
https://codereview.chromium.org/2032943002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2032943002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp:163: static
const
> > size_t maxWebFontSize = 30 * 1024 * 1024; // 30 MB
> > On 2016/06/02 14:51:17, eae wrote:
> > > Is this limit documented somewhere? 30 MB seems pretty low given that
there
> > are
> > > fonts that cover the entire unicode baseplane.
> > 
> > I don't think this limit is documented somewhere. Probably it's an arbitrary
> > limit someone added in the past. We may want to increase (or just remove)
the
> > limit but I'll keep this limit in this CL.
> 
> Sounds good. At the very least we should make sure it's documented somewhere.
No
> point in blocking this CL on it though.

Added a TODO comment in WebFontDecoder.cpp. Let me know if there is an
appropriate place to document the limit. I'm happy to put some explanations.

[eae, 2016-06-03 00:37:37]

On 2016/06/03 00:33:19, bashi1 wrote:
> On 2016/06/02 23:13:57, eae wrote:
> > On 2016/06/02 23:06:56, bashi1 wrote:
> > > Thanks for review!
> > > 
> > >
> >
>
https://codereview.chromium.org/2032943002/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2032943002/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp:163: static
> const
> > > size_t maxWebFontSize = 30 * 1024 * 1024; // 30 MB
> > > On 2016/06/02 14:51:17, eae wrote:
> > > > Is this limit documented somewhere? 30 MB seems pretty low given that
> there
> > > are
> > > > fonts that cover the entire unicode baseplane.
> > > 
> > > I don't think this limit is documented somewhere. Probably it's an
arbitrary
> > > limit someone added in the past. We may want to increase (or just remove)
> the
> > > limit but I'll keep this limit in this CL.
> > 
> > Sounds good. At the very least we should make sure it's documented
somewhere.
> No
> > point in blocking this CL on it though.
> 
> Added a TODO comment in WebFontDecoder.cpp. Let me know if there is an
> appropriate place to document the limit. I'm happy to put some explanations.

Thank you.
Showing a warning in the developer console would be a good place for it.

[bashi, 2016-06-03 00:42:04]

On 2016/06/03 00:37:37, eae wrote:
> On 2016/06/03 00:33:19, bashi1 wrote:
> > On 2016/06/02 23:13:57, eae wrote:
> > > On 2016/06/02 23:06:56, bashi1 wrote:
> > > > Thanks for review!
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2032943002/diff/40001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp
(right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2032943002/diff/40001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp:163: static
> > const
> > > > size_t maxWebFontSize = 30 * 1024 * 1024; // 30 MB
> > > > On 2016/06/02 14:51:17, eae wrote:
> > > > > Is this limit documented somewhere? 30 MB seems pretty low given that
> > there
> > > > are
> > > > > fonts that cover the entire unicode baseplane.
> > > > 
> > > > I don't think this limit is documented somewhere. Probably it's an
> arbitrary
> > > > limit someone added in the past. We may want to increase (or just
remove)
> > the
> > > > limit but I'll keep this limit in this CL.
> > > 
> > > Sounds good. At the very least we should make sure it's documented
> somewhere.
> > No
> > > point in blocking this CL on it though.
> > 
> > Added a TODO comment in WebFontDecoder.cpp. Let me know if there is an
> > appropriate place to document the limit. I'm happy to put some explanations.
> 
> Thank you.
> Showing a warning in the developer console would be a good place for it.

Thanks. It seems we are already showing a warning message in the developer
console :)

[bashi, 2016-06-03 06:35:06]

The CQ bit was checked by bashi@chromium.org

[bashi, 2016-06-03 06:35:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
ksakamoto@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2032943002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-06-03 06:35:25]

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

[commit-bot: I haz the power, 2016-06-03 06:39:45]

Description was changed from

==========
Avoid one temporary copy on web fonts decoding

Before this CL, we allocated a temporary SharedBuffer to store decoded
font data. Since the decoded data is only used to create SkTypeface we don't
need to have such abstraction. Create SkTypeface directly from the decoded
data. This CL also renamed and moved platform/fonts/opentype/OpenTypeSanitizer
to platform/fonts/WebFontDecoder for better naming.

BUG=616969
==========

to

==========
Avoid one temporary copy on web fonts decoding

Before this CL, we allocated a temporary SharedBuffer to store decoded
font data. Since the decoded data is only used to create SkTypeface we don't
need to have such abstraction. Create SkTypeface directly from the decoded
data. This CL also renamed and moved platform/fonts/opentype/OpenTypeSanitizer
to platform/fonts/WebFontDecoder for better naming.

BUG=616969
==========

[commit-bot: I haz the power, 2016-06-03 06:39:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-03 06:41:23]

Description was changed from

==========
Avoid one temporary copy on web fonts decoding

Before this CL, we allocated a temporary SharedBuffer to store decoded
font data. Since the decoded data is only used to create SkTypeface we don't
need to have such abstraction. Create SkTypeface directly from the decoded
data. This CL also renamed and moved platform/fonts/opentype/OpenTypeSanitizer
to platform/fonts/WebFontDecoder for better naming.

BUG=616969
==========

to

==========
Avoid one temporary copy on web fonts decoding

Before this CL, we allocated a temporary SharedBuffer to store decoded
font data. Since the decoded data is only used to create SkTypeface we don't
need to have such abstraction. Create SkTypeface directly from the decoded
data. This CL also renamed and moved platform/fonts/opentype/OpenTypeSanitizer
to platform/fonts/WebFontDecoder for better naming.

BUG=616969

Committed: https://crrev.com/f4e652e1603fdc1c59afc235261648cac4d3ba1e
Cr-Commit-Position: refs/heads/master@{#397648}
==========

[commit-bot: I haz the power, 2016-06-03 06:41:24]

Patchset 4 (id:??) landed as
https://crrev.com/f4e652e1603fdc1c59afc235261648cac4d3ba1e
Cr-Commit-Position: refs/heads/master@{#397648}

[drott, 2016-06-07 09:01:46]

drott@chromium.org changed reviewers:
+ drott@chromium.org

[drott, 2016-06-07 09:01:47]

https://codereview.chromium.org/2032943002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2032943002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:1370: crbug.com/616969
fast/text/emoji-web-font.html [ NeedsRebaseline ]
How is the emoji-web-font result affected?

[bashi, 2016-06-07 09:14:34]

On 2016/06/07 09:01:47, drott wrote:
>
https://codereview.chromium.org/2032943002/diff/60001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/2032943002/diff/60001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/TestExpectations:1370: crbug.com/616969
> fast/text/emoji-web-font.html [ NeedsRebaseline ]
> How is the emoji-web-font result affected?

In this CL I added a warning message when Skia failed to create SkTypeface. And
the test has been failing for some reason (even before the CL is landed) on some
bots so I needed to rebaseline the expectations. There might be a bug but I
don't think this causes the regression.

[drott, 2016-06-07 09:16:27]

On 2016/06/07 at 09:14:34, bashi wrote:
> On 2016/06/07 09:01:47, drott wrote:
> >
https://codereview.chromium.org/2032943002/diff/60001/third_party/WebKit/Layo...
> > File third_party/WebKit/LayoutTests/TestExpectations (right):
> > 
> >
https://codereview.chromium.org/2032943002/diff/60001/third_party/WebKit/Layo...
> > third_party/WebKit/LayoutTests/TestExpectations:1370: crbug.com/616969
> > fast/text/emoji-web-font.html [ NeedsRebaseline ]
> > How is the emoji-web-font result affected?
> 
> In this CL I added a warning message when Skia failed to create SkTypeface.
And the test has been failing for some reason (even before the CL is landed) on
some bots so I needed to rebaseline the expectations. There might be a bug but I
don't think this causes the regression.

I see, thanks. The different behavior on the bots might be differing versions of
Freetype?

[bashi, 2016-06-07 09:23:55]

On 2016/06/07 09:16:27, drott wrote:
> On 2016/06/07 at 09:14:34, bashi wrote:
> > On 2016/06/07 09:01:47, drott wrote:
> > >
>
https://codereview.chromium.org/2032943002/diff/60001/third_party/WebKit/Layo...
> > > File third_party/WebKit/LayoutTests/TestExpectations (right):
> > > 
> > >
>
https://codereview.chromium.org/2032943002/diff/60001/third_party/WebKit/Layo...
> > > third_party/WebKit/LayoutTests/TestExpectations:1370: crbug.com/616969
> > > fast/text/emoji-web-font.html [ NeedsRebaseline ]
> > > How is the emoji-web-font result affected?
> > 
> > In this CL I added a warning message when Skia failed to create SkTypeface.
> And the test has been failing for some reason (even before the CL is landed)
on
> some bots so I needed to rebaseline the expectations. There might be a bug but
I
> don't think this causes the regression.
> 
> I see, thanks. The different behavior on the bots might be differing versions
of
> Freetype?

Yeah, it's likely as I wasn't able to repro :(