Remove SVG fonts

Remove SVG fonts

This patch removes SVG fonts in all platforms, as was discussed in
https://groups.google.com/a/chromium.org/d/msg/blink-dev/pYbbUcYvlYY/LQvFvM8KZZEJ

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

[pdr., 2014-10-23 21:26:37]

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

[eae, 2014-10-23 22:02:34]

This is awesome!

https://codereview.chromium.org/656913006/diff/1/Source/config.gni
File Source/config.gni (right):

https://codereview.chromium.org/656913006/diff/1/Source/config.gni#newcode53
Source/config.gni:53: feature_defines_list = [
We might want to remove the list entirely (and the pointless comment above) as
this was the last feature using it.

https://codereview.chromium.org/656913006/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElement.cpp (right):

https://codereview.chromium.org/656913006/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElement.cpp:77: // FIXME: We should be able to
remove this.
Could you file and link to a bug here please?

https://codereview.chromium.org/656913006/diff/1/Source/core/fetch/FontResour...
File Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/656913006/diff/1/Source/core/fetch/FontResour...
Source/core/fetch/FontResource.cpp:83: void FontResource::trace(Visitor*
visitor)
Remove the entire overloaded function?

https://codereview.chromium.org/656913006/diff/1/Source/core/rendering/svg/SV...
File Source/core/rendering/svg/SVGTextLayoutEngineSpacing.cpp (right):

https://codereview.chromium.org/656913006/diff/1/Source/core/rendering/svg/SV...
Source/core/rendering/svg/SVGTextLayoutEngineSpacing.cpp:38: float
SVGTextLayoutEngineSpacing::calculateSVGKerning(bool /*isVerticalText*/, Glyph
/*currentGlyph*/)
Can we remove this method entirely now (or in the future)?

[jbroman, 2014-10-23 22:09:15]

https://codereview.chromium.org/656913006/diff/1/Source/config.gni
File Source/config.gni (right):

https://codereview.chromium.org/656913006/diff/1/Source/config.gni#newcode53
Source/config.gni:53: feature_defines_list = [
On 2014/10/23 22:02:33, eae wrote:
> We might want to remove the list entirely (and the pointless comment above) as
> this was the last feature using it.

What about the ones below?

[pdr., 2014-10-23 22:15:34]

Thanks for the review! The code should be updated now, though I am still working
through ~50 test failures.

https://codereview.chromium.org/656913006/diff/1/Source/config.gni
File Source/config.gni (right):

https://codereview.chromium.org/656913006/diff/1/Source/config.gni#newcode53
Source/config.gni:53: feature_defines_list = [
On 2014/10/23 22:02:33, eae wrote:
> We might want to remove the list entirely (and the pointless comment above) as
> this was the last feature using it.

Unfortunately we have to leave this, as the lines below add to it.

https://codereview.chromium.org/656913006/diff/1/Source/core/fetch/FontResour...
File Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/656913006/diff/1/Source/core/fetch/FontResour...
Source/core/fetch/FontResource.cpp:83: void FontResource::trace(Visitor*
visitor)
On 2014/10/23 22:02:34, eae wrote:
> Remove the entire overloaded function?

Done.

https://codereview.chromium.org/656913006/diff/1/Source/core/rendering/svg/SV...
File Source/core/rendering/svg/SVGTextLayoutEngineSpacing.cpp (right):

https://codereview.chromium.org/656913006/diff/1/Source/core/rendering/svg/SV...
Source/core/rendering/svg/SVGTextLayoutEngineSpacing.cpp:38: float
SVGTextLayoutEngineSpacing::calculateSVGKerning(bool /*isVerticalText*/, Glyph
/*currentGlyph*/)
On 2014/10/23 22:02:34, eae wrote:
> Can we remove this method entirely now (or in the future)?

Done.

[eae, 2014-10-23 22:16:45]

On 2014/10/23 22:15:34, pdr wrote:
> Thanks for the review! The code should be updated now, though I am still
working
> through ~50 test failures.
> 
> https://codereview.chromium.org/656913006/diff/1/Source/config.gni
> File Source/config.gni (right):
> 
> https://codereview.chromium.org/656913006/diff/1/Source/config.gni#newcode53
> Source/config.gni:53: feature_defines_list = [
> On 2014/10/23 22:02:33, eae wrote:
> > We might want to remove the list entirely (and the pointless comment above)
as
> > this was the last feature using it.
> 
> Unfortunately we have to leave this, as the lines below add to it.

Oh yeah, just saw that. Sadness.


>
https://codereview.chromium.org/656913006/diff/1/Source/core/fetch/FontResour...
> File Source/core/fetch/FontResource.cpp (right):
> 
>
https://codereview.chromium.org/656913006/diff/1/Source/core/fetch/FontResour...
> Source/core/fetch/FontResource.cpp:83: void FontResource::trace(Visitor*
> visitor)
> On 2014/10/23 22:02:34, eae wrote:
> > Remove the entire overloaded function?
> 
> Done.
> 
>
https://codereview.chromium.org/656913006/diff/1/Source/core/rendering/svg/SV...
> File Source/core/rendering/svg/SVGTextLayoutEngineSpacing.cpp (right):
> 
>
https://codereview.chromium.org/656913006/diff/1/Source/core/rendering/svg/SV...
> Source/core/rendering/svg/SVGTextLayoutEngineSpacing.cpp:38: float
> SVGTextLayoutEngineSpacing::calculateSVGKerning(bool /*isVerticalText*/, Glyph
> /*currentGlyph*/)
> On 2014/10/23 22:02:34, eae wrote:
> > Can we remove this method entirely now (or in the future)?
> 
> Done.

Thanks!

[eae, 2014-10-23 22:17:25]

LGTM

[pdr., 2014-10-23 23:53:27]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-10-23 23:54:36]

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

[commit-bot: I haz the power, 2014-10-24 01:11:25]

Committed patchset #5 (id:70001) as 184321

[Erik Dahlström (inactive), 2014-10-24 08:05:22]

ed@opera.com changed reviewers:
+ ed@opera.com

[Erik Dahlström (inactive), 2014-10-24 08:05:25]

https://codereview.chromium.org/656913006/diff/70001/LayoutTests/svg/W3C-SVG-...
File LayoutTests/svg/W3C-SVG-1.1-SE/text-intro-09-b.svg (left):

https://codereview.chromium.org/656913006/diff/70001/LayoutTests/svg/W3C-SVG-...
LayoutTests/svg/W3C-SVG-1.1-SE/text-intro-09-b.svg:1: <svg version="1.1"
baseProfile="basic" id="svg-root"
why was this test removed? it isn't meant to test svg fonts specifically, it's
for testing correct textshaping and RTL and such in svg text content.

[pdr., 2014-10-24 20:34:44]

On 2014/10/24 at 08:05:25, ed wrote:
>
https://codereview.chromium.org/656913006/diff/70001/LayoutTests/svg/W3C-SVG-...
> File LayoutTests/svg/W3C-SVG-1.1-SE/text-intro-09-b.svg (left):
> 
>
https://codereview.chromium.org/656913006/diff/70001/LayoutTests/svg/W3C-SVG-...
> LayoutTests/svg/W3C-SVG-1.1-SE/text-intro-09-b.svg:1: <svg version="1.1"
baseProfile="basic" id="svg-root"
> why was this test removed? it isn't meant to test svg fonts specifically, it's
for testing correct textshaping and RTL and such in svg text content.

I think you're right. I'll upload a separate patch to restore this test.