Initial OpenType Font Variations Support

Initial OpenType Font Variations Support

Adding support for instantiating OpenType Font Variations from web fonts
through font-variation-settings on Linux and Mac. Linux currenly
requires a system FreeType at version 2.6.5 or newer and at least macOS
10.11, but support is still experimental.

With this CL, in addition to web fonts Mac supports instantiating system
font variations.

Some known issues, see meta bug crbug.com/669453:
* Correct mark positioning requires new HarfBuzz API to pass axis
  parameters from Skia to HarfBuzz
* Incorrect advance width scaling on Linux when hinting is enabled (as
  it is by default), discussed and almost solved in
  https://bugs.chromium.org/p/skia/issues/detail?id=5917

TEST=fast/text/variable-fonts/variable-box-font.html,
fast/text/variable-fonts/variable-mac-system-font.html,
http/tests/webfont/variable-box-font-arraybuffer.html
FontSettingsTest.cpp
BUG=669459, 670246
Committed: https://crrev.com/d5cec19a655dbb722f75bfccdaa44f69c0827e64
Cr-Commit-Position: refs/heads/master@{#440455}

[drott, 2016-12-16 14:26:35]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 14:26:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[drott, 2016-12-16 14:31:33]

Adding missing test expectation, clean up ambiguous test CSS

[drott, 2016-12-16 14:31:34]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 14:31:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[drott, 2016-12-16 14:32:04]

drott@chromium.org changed reviewers:
+ behdad@chromium.org, eae@chromium.org, kojii@chromium.org

[drott, 2016-12-16 14:32:20]

Ready for review, yeyh.

[commit-bot: I haz the power, 2016-12-16 15:07:28]

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

[commit-bot: I haz the power, 2016-12-16 15:07:29]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[drott, 2016-12-16 15:30:41]

Fix Windows compilation by avoiding VLA

[drott, 2016-12-16 15:30:41]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 15:30:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 17:00:00]

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

[commit-bot: I haz the power, 2016-12-16 17:00:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[eae, 2016-12-16 18:36:27]

Starting review.

[eae, 2016-12-16 18:48:27]

This is great! Have a couple of questions and suggestions but overall this looks
really good.

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/text/variable-fonts/variable-box-font-inline.html
(right):

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/text/variable-fonts/variable-box-font-inline.html:6:
src:url(data:font/opentype;base64,AAEAAAAOAIAAAwBgRFNJRwAAAAEAAAigAAAACEdTVUIAAQAAAAAIJAAAAApPUy8yaByMpAAAAWgAAABgY21hcCYEJfEAAAHYAAAATGZ2YXJ6Q3iCAAAIMAAAADRnbHlmy/PKewAAAjAAAAFaZ3ZhcsYlqCQAAAhkAAAAOmhlYWQKWWH6AAAA7AAAADZoaGVhBkIBkwAAASQAAAAkaG10eARMAF0AAAHIAAAADmxvY2EA9QGIAAACJAAAAAxtYXhwAHoEUAAAAUgAAAAgbmFtZczowUYAAAOMAAAEU3Bvc3QdShcBAAAH4AAAAEIAAQAAAAEAABXxStdfDzz1AAMD6AAAAADUcIMTAAAAANR5m6sAAP84AlgDIAAAAAcAAgAAAAAAAAABAAAD6P84AAACWAAAAAACWAABAAAAAAAAAAAAAAAAAAAAAgABAAAABQBJAAoAAAAAAAEAAgAeAAQAAABkA+gAAAAAAAMCNgGQAAUACAKKAlgAAABLAooCWAAAAV4AMgEsAAAAAAUAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFVLV04AQAAgJYQDIP84AMgD6ADIAAAAAQAAAAAB9AK8AAAAIAAAAfQAXQJYAAAAAAAAAAAAAAAAAAIAAAADAAAAFAADAAEAAAAUAAQAOAAAAAoACAACAAIAIABBJYAlhP//AAAAIABBJYAlhP///+H/wdqD2oAAAQAAAAAAAAAAAAAAAABhAGEAegCUAK0ACgBd/zgBmgMgAAMABwALABcAHQAnACsAMQA9AEgAAAERIRETIxUzJxUjNTcjFTM1MxUjNSMVMzUjFSMVMzUjFTMVIxUzNTMnFSM1NyMVMzUjNyMVMxUjFTM1IzUzESMVMwcVMzUjNzMBmv7D8KamIWSFZCIhZCGmIYWmpkJCZEJkIUNkpkJCpUFCpkJCpkZGpmZGIAMg/BgD6P1WcVAvL844Fy9QcetCIqAhJSFGgiQkImgixyElISEl/TIhLyEhLwADAAAAAAJYArwAAwAHAAsAABEhESExETMRIREzEQJY/agUAjAUAV7+ogK8/UQCvP1EAAMAAAAAAlgCvAADAAcACwAAESERIRERMxEhETMRAlj9qBQCMBQCvP6i/qICvP1EArz9RAADAAAAAAJYArwAAwAHAAsAABEhESExETMRIREzEQJY/agUAjAUAV7+ogK8/UQCvP1EAAAAAAAVAQIAAQAAAAAAAAA5AAAAAQAAAAAAAQAQADkAAQAAAAAAAgAHAEkAAQAAAAAAAwAjAFAAAQAAAAAABAAYAHMAAQAAAAAABQANAIsAAQAAAAAABgAYAJgAAQAAAAAADQAiALAAAQAAAAAADgA/ANIAAwABBAkAAAByAREAAwABBAkAAQAgAYMAAwABBAkAAgAOAaMAAwABBAkAAwBGAbEAAwABBAkABAAwAfcAAwABBAkABQAaAicAAwABBAkABgAwAkEAAwABBAkADQBEAnEAAwABBAkADgB+ArUAAwABBAkBAAAMAzMAAwABBAkBAQAMAz8AAwABBAkBAgAGA0tDb3B5cmlnaHQgMjAxNiBUaGUgQ2hyb21pdW0gQXV0aG9ycy4gQWxsIHJpZ2h0cyByZXNlcnZlZC52YXJpYWJsZXRlc3RfYm94UmVndWxhcjEuMDAwO1VLV047dmFyaWFibGV0ZXN0X2JveC1SZWd1bGFydmFyaWFibGV0ZXN0X2JveCBSZWd1bGFyVmVyc2lvbiAxLjAwMHZhcmlhYmxldGVzdF9ib3gtUmVndWxhclNJTCBPcGVuIEZvbnQgTGljZW5zZSwgVmVyc2lvbiAxLjFodHRwOi8vc2NyaXB0cy5zaWwub3JnL2Ntcy9zY3JpcHRzL3BhZ2UucGhwP3NpdGVfaWQ9bnJzaSZpZD1PRkwAQwBvAHAAeQByAGkAZwBoAHQAIAAyADAAMQA2ACAAVABoAGUAIABDAGgAcgBvAG0AaQB1AG0AIABBAHUAdABoAG8AcgBzAC4AIABBAGwAbAAgAHIAaQBnAGgAdABzACAAcgBlAHMAZQByAHYAZQBkAC4AdgBhAHIAaQBhAGIAbABlAHQAZQBzAHQAXwBiAG8AeABSAGUAZwB1AGwAYQByADEALgAwADAAMAA7AFUASwBXAE4AOwB2AGEAcgBpAGEAYgBsAGUAdABlAHMAdABfAGIAbwB4AC0AUgBlAGcAdQBsAGEAcgB2AGEAcgBpAGEAYgBsAGUAdABlAHMAdABfAGIAbwB4ACAAUgBlAGcAdQBsAGEAcgBWAGUAcgBzAGkAbwBuACAAMQAuADAAMAAwAHYAYQByAGkAYQBiAGwAZQB0AGUAcwB0AF8AYgBvAHgALQBSAGUAZwB1AGwAYQByAFMASQBMACAATwBwAGUAbgAgAEYAbwBuAHQAIABMAGkAYwBlAG4AcwBlACwAIABWAGUAcgBzAGkAbwBuACAAMQAuADEAaAB0AHQAcAA6AC8ALwBzAGMAcgBpAHAAdABzAC4AcwBpAGwALgBvAHIAZwAvAGMAbQBzAC8AcwBjAHIAaQBwAHQAcwAvAHAAYQBnAGUALgBwAGgAcAA/AHMAaQB0AGUAXwBpAGQAPQBuAHIAcwBpACYAaQBkAD0ATwBGAEwAVQBwAHcAYQByAGQAQgBvAHQAdABvAG0AVABvAHAAAAIAAAAAAAD/tQAyAAAAAAAAAAAAAAAAAAAAAAAAAAAABQAAAAMAJAECAQMKdXBwZXJfaGFsZgpsb3dlcl9oYWxmAAAAAQAAAAAAAAAAAAAAAQAAABAAAgABABQAAgAIdXB3ZAAAAAAAAAAAAV4AAAAAAQABAQAAAAAAAAECAAABXgAAAAEAAAABAAAAAAAgAAUAAAAAACAAAAAAAAAADQANAA0AAQAKABCgAEAABAMAAQEBg0MBXgFeAV4BXgAAAAAAAQAAAAA=);
Why do you have both an inline and an external-resource version of the same
test? Is the logic different between the two?

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/text/variable-fonts/variable-mac-system-font.html
(right):

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/text/variable-fonts/variable-mac-system-font.html:7:
window.onload = () => {
I don't think you need to use an onload handler here. If you move the script
block down to the end of the body it should work as a regular synchronous test.
That'll make it faster and a little easier to debug.

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/resource/FontResource.h (right):

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/resource/FontResource.h:71:
FontVariationSettings* = nullptr);
Can this be a const pointer?

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

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontCustomPlatformData.cpp:55: return
SkSetFourByteTag(tag[0], tag[1], tag[2], tag[3]);
DCHECK(tag.size() == 4);

actually, as the tags can be user supplied this should be a proper if-check if
we haven't validated that already. If we have then a dcheck is fine.

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontCustomPlatformData.cpp:68: if
(variationSettings && variationSettings->size() < UINT16_MAX) {
I'm guessing the size < UINT16_MAX check is a security check for malicious
content? Regardless, please add a comment explaining the limit.

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

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/linux/FontPlatformDataLinux.cpp:49: //
TODO: Due to Skia bug 5917
TODO(drott) or TODO(layout-dev)

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/mac/FontPlatformDataMac.mm
(right):

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/mac/FontPlatformDataMac.mm:163: static
SkFourByteTag atomicStringToFourByteTag(AtomicString tag) {
Given that we probably want a couple of checks in here it might make sense to
share this function across implementations.

[drott, 2016-12-19 13:36:09]

Review comments addressed

[drott, 2016-12-19 13:41:53]

Hash collision fixes

[drott, 2016-12-19 13:41:54]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 13:42:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 13:55:52]

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

[commit-bot: I haz the power, 2016-12-19 13:55:52]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[drott, 2016-12-19 14:10:05]

DCHECK build fix

[drott, 2016-12-19 14:10:06]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 14:10:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[drott, 2016-12-19 14:12:34]

Thanks for the detailed review and positive feedback, Emil. I am quite excited
about getting this landed. Replied inline with explanations and comments in the
code.

Disabled Linux tests for now, looks like the bots do required the newer version
of FreeType - also we need to work out why it only works when hinting is off.
Ben has a Skia fix proposal for that in the relevant bug.

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/text/variable-fonts/variable-box-font-inline.html
(right):

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/text/variable-fonts/variable-box-font-inline.html:6:
src:url(data:font/opentype;base64,AAEAAAAOAIAAAwBgRFNJRwAAAAEAAAigAAAACEdTVUIAAQAAAAAIJAAAAApPUy8yaByMpAAAAWgAAABgY21hcCYEJfEAAAHYAAAATGZ2YXJ6Q3iCAAAIMAAAADRnbHlmy/PKewAAAjAAAAFaZ3ZhcsYlqCQAAAhkAAAAOmhlYWQKWWH6AAAA7AAAADZoaGVhBkIBkwAAASQAAAAkaG10eARMAF0AAAHIAAAADmxvY2EA9QGIAAACJAAAAAxtYXhwAHoEUAAAAUgAAAAgbmFtZczowUYAAAOMAAAEU3Bvc3QdShcBAAAH4AAAAEIAAQAAAAEAABXxStdfDzz1AAMD6AAAAADUcIMTAAAAANR5m6sAAP84AlgDIAAAAAcAAgAAAAAAAAABAAAD6P84AAACWAAAAAACWAABAAAAAAAAAAAAAAAAAAAAAgABAAAABQBJAAoAAAAAAAEAAgAeAAQAAABkA+gAAAAAAAMCNgGQAAUACAKKAlgAAABLAooCWAAAAV4AMgEsAAAAAAUAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFVLV04AQAAgJYQDIP84AMgD6ADIAAAAAQAAAAAB9AK8AAAAIAAAAfQAXQJYAAAAAAAAAAAAAAAAAAIAAAADAAAAFAADAAEAAAAUAAQAOAAAAAoACAACAAIAIABBJYAlhP//AAAAIABBJYAlhP///+H/wdqD2oAAAQAAAAAAAAAAAAAAAABhAGEAegCUAK0ACgBd/zgBmgMgAAMABwALABcAHQAnACsAMQA9AEgAAAERIRETIxUzJxUjNTcjFTM1MxUjNSMVMzUjFSMVMzUjFTMVIxUzNTMnFSM1NyMVMzUjNyMVMxUjFTM1IzUzESMVMwcVMzUjNzMBmv7D8KamIWSFZCIhZCGmIYWmpkJCZEJkIUNkpkJCpUFCpkJCpkZGpmZGIAMg/BgD6P1WcVAvL844Fy9QcetCIqAhJSFGgiQkImgixyElISEl/TIhLyEhLwADAAAAAAJYArwAAwAHAAsAABEhESExETMRIREzEQJY/agUAjAUAV7+ogK8/UQCvP1EAAMAAAAAAlgCvAADAAcACwAAESERIRERMxEhETMRAlj9qBQCMBQCvP6i/qICvP1EArz9RAADAAAAAAJYArwAAwAHAAsAABEhESExETMRIREzEQJY/agUAjAUAV7+ogK8/UQCvP1EAAAAAAAVAQIAAQAAAAAAAAA5AAAAAQAAAAAAAQAQADkAAQAAAAAAAgAHAEkAAQAAAAAAAwAjAFAAAQAAAAAABAAYAHMAAQAAAAAABQANAIsAAQAAAAAABgAYAJgAAQAAAAAADQAiALAAAQAAAAAADgA/ANIAAwABBAkAAAByAREAAwABBAkAAQAgAYMAAwABBAkAAgAOAaMAAwABBAkAAwBGAbEAAwABBAkABAAwAfcAAwABBAkABQAaAicAAwABBAkABgAwAkEAAwABBAkADQBEAnEAAwABBAkADgB+ArUAAwABBAkBAAAMAzMAAwABBAkBAQAMAz8AAwABBAkBAgAGA0tDb3B5cmlnaHQgMjAxNiBUaGUgQ2hyb21pdW0gQXV0aG9ycy4gQWxsIHJpZ2h0cyByZXNlcnZlZC52YXJpYWJsZXRlc3RfYm94UmVndWxhcjEuMDAwO1VLV047dmFyaWFibGV0ZXN0X2JveC1SZWd1bGFydmFyaWFibGV0ZXN0X2JveCBSZWd1bGFyVmVyc2lvbiAxLjAwMHZhcmlhYmxldGVzdF9ib3gtUmVndWxhclNJTCBPcGVuIEZvbnQgTGljZW5zZSwgVmVyc2lvbiAxLjFodHRwOi8vc2NyaXB0cy5zaWwub3JnL2Ntcy9zY3JpcHRzL3BhZ2UucGhwP3NpdGVfaWQ9bnJzaSZpZD1PRkwAQwBvAHAAeQByAGkAZwBoAHQAIAAyADAAMQA2ACAAVABoAGUAIABDAGgAcgBvAG0AaQB1AG0AIABBAHUAdABoAG8AcgBzAC4AIABBAGwAbAAgAHIAaQBnAGgAdABzACAAcgBlAHMAZQByAHYAZQBkAC4AdgBhAHIAaQBhAGIAbABlAHQAZQBzAHQAXwBiAG8AeABSAGUAZwB1AGwAYQByADEALgAwADAAMAA7AFUASwBXAE4AOwB2AGEAcgBpAGEAYgBsAGUAdABlAHMAdABfAGIAbwB4AC0AUgBlAGcAdQBsAGEAcgB2AGEAcgBpAGEAYgBsAGUAdABlAHMAdABfAGIAbwB4ACAAUgBlAGcAdQBsAGEAcgBWAGUAcgBzAGkAbwBuACAAMQAuADAAMAAwAHYAYQByAGkAYQBiAGwAZQB0AGUAcwB0AF8AYgBvAHgALQBSAGUAZwB1AGwAYQByAFMASQBMACAATwBwAGUAbgAgAEYAbwBuAHQAIABMAGkAYwBlAG4AcwBlACwAIABWAGUAcgBzAGkAbwBuACAAMQAuADEAaAB0AHQAcAA6AC8ALwBzAGMAcgBpAHAAdABzAC4AcwBpAGwALgBvAHIAZwAvAGMAbQBzAC8AcwBjAHIAaQBwAHQAcwAvAHAAYQBnAGUALgBwAGgAcAA/AHMAaQB0AGUAXwBpAGQAPQBuAHIAcwBpACYAaQBkAD0ATwBGAEwAVQBwAHcAYQByAGQAQgBvAHQAdABvAG0AVABvAHAAAAIAAAAAAAD/tQAyAAAAAAAAAAAAAAAAAAAAAAAAAAAABQAAAAMAJAECAQMKdXBwZXJfaGFsZgpsb3dlcl9oYWxmAAAAAQAAAAAAAAAAAAAAAQAAABAAAgABABQAAgAIdXB3ZAAAAAAAAAAAAV4AAAAAAQABAQAAAAAAAAECAAABXgAAAAEAAAABAAAAAAAgAAUAAAAAACAAAAAAAAAADQANAA0AAQAKABCgAEAABAMAAQEBg0MBXgFeAV4BXgAAAAAAAQAAAAA=);
On 2016/12/16 at 18:48:27, eae (ooo until 2017-01-03) wrote:
> Why do you have both an inline and an external-resource version of the same
test? Is the logic different between the two?

There are two changes to add variation parameters in RemoteFontFaceSource and
BinaryDataFontFaceSource. I was assuming, inline base64 data would test the
latter, but upon your good catch here, I double checked this. Created a new test
in http/tests/webfonts/variable-box-font-arraybuffer.html which uses the Fetch
API, then creates a JS ArrayBuffer from the downloaded font blob and inserts it
into the set of fonts in the DOM. This path tests variation parameters on
BinaryDataFontFaceSource.

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/text/variable-fonts/variable-mac-system-font.html
(right):

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/text/variable-fonts/variable-mac-system-font.html:7:
window.onload = () => {
On 2016/12/16 at 18:48:27, eae (ooo until 2017-01-03) wrote:
> I don't think you need to use an onload handler here. If you move the script
block down to the end of the body it should work as a regular synchronous test.
That'll make it faster and a little easier to debug.

Moved to end, no onload handler.

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/resource/FontResource.h (right):

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/resource/FontResource.h:71:
FontVariationSettings* = nullptr);
On 2016/12/16 at 18:48:27, eae (ooo until 2017-01-03) wrote:
> Can this be a const pointer?

Yes, modified accordingly.

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

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontCustomPlatformData.cpp:55: return
SkSetFourByteTag(tag[0], tag[1], tag[2], tag[3]);
On 2016/12/16 at 18:48:27, eae (ooo until 2017-01-03) wrote:
> DCHECK(tag.size() == 4);
> 
> actually, as the tags can be user supplied this should be a proper if-check if
we haven't validated that already. If we have then a dcheck is fine.

Added the DCHECK. Generally the feature tags can only come in through
CSSPropertyParser, which has length-checks in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/parse...

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontCustomPlatformData.cpp:68: if
(variationSettings && variationSettings->size() < UINT16_MAX) {
On 2016/12/16 at 18:48:27, eae (ooo until 2017-01-03) wrote:
> I'm guessing the size < UINT16_MAX check is a security check for malicious
content? Regardless, please add a comment explaining the limit.

Explained in new comment.

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

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/linux/FontPlatformDataLinux.cpp:49: //
TODO: Due to Skia bug 5917
On 2016/12/16 at 18:48:27, eae (ooo until 2017-01-03) wrote:
> TODO(drott) or TODO(layout-dev)

Done.

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/mac/FontPlatformDataMac.mm
(right):

https://codereview.chromium.org/2581083003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/mac/FontPlatformDataMac.mm:163: static
SkFourByteTag atomicStringToFourByteTag(AtomicString tag) {
On 2016/12/16 at 18:48:27, eae (ooo until 2017-01-03) wrote:
> Given that we probably want a couple of checks in here it might make sense to
share this function across implementations.

Moved to FontSettings.h and cleared of Skia types in order not to pollute our
includes.

[commit-bot: I haz the power, 2016-12-19 14:28:43]

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

[commit-bot: I haz the power, 2016-12-19 14:28:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[drott, 2016-12-19 14:47:29]

Description was changed from

==========
Initial OpenType Font Variations Support

Adding support for instantiating OpenType Font Variations from web fonts
through font-variation-settings on Linux and Mac. Linux currenly
requires a system FreeType at version 2.6.5 or newer.

With this CL, in addition to web fonts Mac supports instantiating system
font variations.

Some known issues, see meta bug crbug.com/669453:
* Correct mark positioning requires new HarfBuzz API to pass axis
  parameters from Skia to HarfBuzz
* Incorrect advance width scaling on Linux when hinting is enabled (as
  it is by default)

TEST=fast/text/variable-fonts/variable-box-font.html,
fast/text/variable-fonts/variable-box-font-inline.html,
fast/text/variable-fonts/variable-mac-system-font.html,
FontSettingsTest.cpp
BUG=669459, 670246
==========

to

==========
Initial OpenType Font Variations Support

Adding support for instantiating OpenType Font Variations from web fonts
through font-variation-settings on Linux and Mac. Linux currenly
requires a system FreeType at version 2.6.5 or newer.

With this CL, in addition to web fonts Mac supports instantiating system
font variations.

Some known issues, see meta bug crbug.com/669453:
* Correct mark positioning requires new HarfBuzz API to pass axis
  parameters from Skia to HarfBuzz
* Incorrect advance width scaling on Linux when hinting is enabled (as
  it is by default)

TEST=fast/text/variable-fonts/variable-box-font.html,
fast/text/variable-fonts/variable-mac-system-font.html,
http/tests/webfont/variable-box-font-arraybuffer.html
FontSettingsTest.cpp
BUG=669459, 670246
==========

[drott, 2016-12-19 15:15:35]

CHECK_NE literal

[drott, 2016-12-19 15:15:36]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 15:15:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 15:36:45]

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

[commit-bot: I haz the power, 2016-12-19 15:36:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[drott, 2016-12-19 20:50:55]

Avoid duplicate definitions

[drott, 2016-12-19 20:50:55]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 20:51:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[drott, 2016-12-19 20:55:46]

Remove leftover logging in new test

[drott, 2016-12-19 20:55:47]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 20:56:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 22:27:18]

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

[commit-bot: I haz the power, 2016-12-19 22:27:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[drott, 2016-12-20 09:44:54]

Fix hash collision tests, adjust test expectations

[drott, 2016-12-20 09:44:55]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 09:45:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 11:50:35]

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

[commit-bot: I haz the power, 2016-12-20 11:50:36]

Dry run: This issue passed the CQ dry run.

[drott, 2016-12-20 12:10:45]

Okay, I had to disable tests on below macOS 10.11 as well - this is really still
experimental, but I believe it's ready for final review now, Emil - thanks in
advance for taking another look.

[drott, 2016-12-20 12:11:26]

Description was changed from

==========
Initial OpenType Font Variations Support

Adding support for instantiating OpenType Font Variations from web fonts
through font-variation-settings on Linux and Mac. Linux currenly
requires a system FreeType at version 2.6.5 or newer.

With this CL, in addition to web fonts Mac supports instantiating system
font variations.

Some known issues, see meta bug crbug.com/669453:
* Correct mark positioning requires new HarfBuzz API to pass axis
  parameters from Skia to HarfBuzz
* Incorrect advance width scaling on Linux when hinting is enabled (as
  it is by default)

TEST=fast/text/variable-fonts/variable-box-font.html,
fast/text/variable-fonts/variable-mac-system-font.html,
http/tests/webfont/variable-box-font-arraybuffer.html
FontSettingsTest.cpp
BUG=669459, 670246
==========

to

==========
Initial OpenType Font Variations Support

Adding support for instantiating OpenType Font Variations from web fonts
through font-variation-settings on Linux and Mac. Linux currenly
requires a system FreeType at version 2.6.5 or newer and at least macOS
10.11, but support is still experimental.

With this CL, in addition to web fonts Mac supports instantiating system
font variations.

Some known issues, see meta bug crbug.com/669453:
* Correct mark positioning requires new HarfBuzz API to pass axis
  parameters from Skia to HarfBuzz
* Incorrect advance width scaling on Linux when hinting is enabled (as
  it is by default)

TEST=fast/text/variable-fonts/variable-box-font.html,
fast/text/variable-fonts/variable-mac-system-font.html,
http/tests/webfont/variable-box-font-arraybuffer.html
FontSettingsTest.cpp
BUG=669459, 670246
==========

[drott, 2016-12-20 14:21:29]

Behdad and Koji, could you take a look as well after eae@'s earlier review? I
think he was mostly happy and now he's ooo. Ideally, I'd like to land this
before going on holidays for a few days on Friday. Thanks in advance.

[kojii, 2016-12-21 14:53:12]

lgtm w/a few nits, do you have someone you want to ask review for core? There
might be someone you know in Tokyo web font loading team?

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontCustomPlatformData.cpp
(right):

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontCustomPlatformData.cpp:71:
RefPtr<SkFontMgr> fm = adoptRef(SkFontMgr::RefDefault());
I think we prefer 
  sk_sp<SkFontMgr> fm(SkFontMgr::RefDefault());
since crrev.com/2290903002.

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right):

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:286:
WTF::wrapUnique(new FontPlatformData(
nit: nice to change to makeUnique in this opportunity?
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer-...

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/mac/FontPlatformDataMac.mm
(right):

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/mac/FontPlatformDataMac.mm:191:
RefPtr<SkFontMgr> fm = adoptRef(SkFontMgr::RefDefault());
Same (sk_sp)

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/opentype/FontSettings.cpp (right):

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/opentype/FontSettings.cpp:23:
addToHash(hash, StringHasher::hashMemory(&value, sizeof(value)));
Not important but just from curiosity, why don't we use DefaultHash<float>?

[drott, 2016-12-21 14:57:44]

Description was changed from

==========
Initial OpenType Font Variations Support

Adding support for instantiating OpenType Font Variations from web fonts
through font-variation-settings on Linux and Mac. Linux currenly
requires a system FreeType at version 2.6.5 or newer and at least macOS
10.11, but support is still experimental.

With this CL, in addition to web fonts Mac supports instantiating system
font variations.

Some known issues, see meta bug crbug.com/669453:
* Correct mark positioning requires new HarfBuzz API to pass axis
  parameters from Skia to HarfBuzz
* Incorrect advance width scaling on Linux when hinting is enabled (as
  it is by default)

TEST=fast/text/variable-fonts/variable-box-font.html,
fast/text/variable-fonts/variable-mac-system-font.html,
http/tests/webfont/variable-box-font-arraybuffer.html
FontSettingsTest.cpp
BUG=669459, 670246
==========

to

==========
Initial OpenType Font Variations Support

Adding support for instantiating OpenType Font Variations from web fonts
through font-variation-settings on Linux and Mac. Linux currenly
requires a system FreeType at version 2.6.5 or newer and at least macOS
10.11, but support is still experimental.

With this CL, in addition to web fonts Mac supports instantiating system
font variations.

Some known issues, see meta bug crbug.com/669453:
* Correct mark positioning requires new HarfBuzz API to pass axis
  parameters from Skia to HarfBuzz
* Incorrect advance width scaling on Linux when hinting is enabled (as
  it is by default), discussed and almost solved in 
  https://bugs.chromium.org/p/skia/issues/detail?id=5917

TEST=fast/text/variable-fonts/variable-box-font.html,
fast/text/variable-fonts/variable-mac-system-font.html,
http/tests/webfont/variable-box-font-arraybuffer.html
FontSettingsTest.cpp
BUG=669459, 670246
==========

[drott, 2016-12-21 15:19:52]

Addressing review comments from Koji

[drott, 2016-12-21 15:22:00]

Thanks for the review, Koji. I appreciate your diving into this subject and the
CL is a little large.

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontCustomPlatformData.cpp
(right):

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontCustomPlatformData.cpp:71:
RefPtr<SkFontMgr> fm = adoptRef(SkFontMgr::RefDefault());
On 2016/12/21 at 14:53:12, kojii wrote:
> I think we prefer 
>   sk_sp<SkFontMgr> fm(SkFontMgr::RefDefault());
> since crrev.com/2290903002.

Good point, thanks, done.

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right):

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:286:
WTF::wrapUnique(new FontPlatformData(
On 2016/12/21 at 14:53:12, kojii wrote:
> nit: nice to change to makeUnique in this opportunity?
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer-...

Done.

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/mac/FontPlatformDataMac.mm
(right):

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/mac/FontPlatformDataMac.mm:191:
RefPtr<SkFontMgr> fm = adoptRef(SkFontMgr::RefDefault());
On 2016/12/21 at 14:53:12, kojii wrote:
> Same (sk_sp)

Done.

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/opentype/FontSettings.cpp (right):

https://codereview.chromium.org/2581083003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/opentype/FontSettings.cpp:23:
addToHash(hash, StringHasher::hashMemory(&value, sizeof(value)));
On 2016/12/21 at 14:53:12, kojii wrote:
> Not important but just from curiosity, why don't we use DefaultHash<float>?

Good point, took this from similar code in FontDescription or FontCacheKey and
could have update this. Changed to FloatHash<float>.

[drott, 2016-12-21 15:22:07]

The CQ bit was checked by drott@chromium.org

[drott, 2016-12-21 15:22:08]

The patchset sent to the CQ was uploaded after l-g-t-m from kojii@chromium.org
Link to the patchset: https://codereview.chromium.org/2581083003/#ps200001
(title: "Addressing review comments from Koji")

[commit-bot: I haz the power, 2016-12-21 15:22:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 15:31:08]

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

[commit-bot: I haz the power, 2016-12-21 15:31:09]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[drott, 2016-12-21 15:34:00]

drott@chromium.org changed reviewers:
+ jochen@chromium.org, kouhei@chromium.org, pdr@chromium.org, yutak@chromium.org

[drott, 2016-12-21 15:34:38]

kouhei@chromium.org, yutak@chromium.org, jochen@chromium.org, pdr@chromium.org,
could you please review changes in core/, thank you and happy holidays!

[drott, 2016-12-22 09:12:14]

Fix makeUnique syntax

[drott, 2016-12-22 09:12:15]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 09:12:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 10:47:30]

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

[commit-bot: I haz the power, 2016-12-22 10:47:32]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-12-22 16:01:53]

lgtm

[drott, 2016-12-22 18:27:37]

The CQ bit was checked by drott@chromium.org

[drott, 2016-12-22 18:27:39]

The patchset sent to the CQ was uploaded after l-g-t-m from kojii@chromium.org
Link to the patchset: https://codereview.chromium.org/2581083003/#ps220001
(title: "Fix makeUnique syntax")

[commit-bot: I haz the power, 2016-12-22 18:27:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 18:32:49]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1482431257575890,
"parent_rev": "1caeb253ebe752bcb16c47b64fe7c7ba8b27b26d", "commit_rev":
"39318fdb6ec11382bc74c45bfb415263167af324"}

[commit-bot: I haz the power, 2016-12-22 18:33:18]

Description was changed from

==========
Initial OpenType Font Variations Support

Adding support for instantiating OpenType Font Variations from web fonts
through font-variation-settings on Linux and Mac. Linux currenly
requires a system FreeType at version 2.6.5 or newer and at least macOS
10.11, but support is still experimental.

With this CL, in addition to web fonts Mac supports instantiating system
font variations.

Some known issues, see meta bug crbug.com/669453:
* Correct mark positioning requires new HarfBuzz API to pass axis
  parameters from Skia to HarfBuzz
* Incorrect advance width scaling on Linux when hinting is enabled (as
  it is by default), discussed and almost solved in 
  https://bugs.chromium.org/p/skia/issues/detail?id=5917

TEST=fast/text/variable-fonts/variable-box-font.html,
fast/text/variable-fonts/variable-mac-system-font.html,
http/tests/webfont/variable-box-font-arraybuffer.html
FontSettingsTest.cpp
BUG=669459, 670246
==========

to

==========
Initial OpenType Font Variations Support

Adding support for instantiating OpenType Font Variations from web fonts
through font-variation-settings on Linux and Mac. Linux currenly
requires a system FreeType at version 2.6.5 or newer and at least macOS
10.11, but support is still experimental.

With this CL, in addition to web fonts Mac supports instantiating system
font variations.

Some known issues, see meta bug crbug.com/669453:
* Correct mark positioning requires new HarfBuzz API to pass axis
  parameters from Skia to HarfBuzz
* Incorrect advance width scaling on Linux when hinting is enabled (as
  it is by default), discussed and almost solved in 
  https://bugs.chromium.org/p/skia/issues/detail?id=5917

TEST=fast/text/variable-fonts/variable-box-font.html,
fast/text/variable-fonts/variable-mac-system-font.html,
http/tests/webfont/variable-box-font-arraybuffer.html
FontSettingsTest.cpp
BUG=669459, 670246

Review-Url: https://codereview.chromium.org/2581083003
==========

[commit-bot: I haz the power, 2016-12-22 18:33:20]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-12-22 18:35:22]

Description was changed from

==========
Initial OpenType Font Variations Support

Adding support for instantiating OpenType Font Variations from web fonts
through font-variation-settings on Linux and Mac. Linux currenly
requires a system FreeType at version 2.6.5 or newer and at least macOS
10.11, but support is still experimental.

With this CL, in addition to web fonts Mac supports instantiating system
font variations.

Some known issues, see meta bug crbug.com/669453:
* Correct mark positioning requires new HarfBuzz API to pass axis
  parameters from Skia to HarfBuzz
* Incorrect advance width scaling on Linux when hinting is enabled (as
  it is by default), discussed and almost solved in 
  https://bugs.chromium.org/p/skia/issues/detail?id=5917

TEST=fast/text/variable-fonts/variable-box-font.html,
fast/text/variable-fonts/variable-mac-system-font.html,
http/tests/webfont/variable-box-font-arraybuffer.html
FontSettingsTest.cpp
BUG=669459, 670246

Review-Url: https://codereview.chromium.org/2581083003
==========

to

==========
Initial OpenType Font Variations Support

Adding support for instantiating OpenType Font Variations from web fonts
through font-variation-settings on Linux and Mac. Linux currenly
requires a system FreeType at version 2.6.5 or newer and at least macOS
10.11, but support is still experimental.

With this CL, in addition to web fonts Mac supports instantiating system
font variations.

Some known issues, see meta bug crbug.com/669453:
* Correct mark positioning requires new HarfBuzz API to pass axis
  parameters from Skia to HarfBuzz
* Incorrect advance width scaling on Linux when hinting is enabled (as
  it is by default), discussed and almost solved in
  https://bugs.chromium.org/p/skia/issues/detail?id=5917

TEST=fast/text/variable-fonts/variable-box-font.html,
fast/text/variable-fonts/variable-mac-system-font.html,
http/tests/webfont/variable-box-font-arraybuffer.html
FontSettingsTest.cpp
BUG=669459, 670246

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

[commit-bot: I haz the power, 2016-12-22 18:35:24]

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