Enable layout testing support for font fallback

Enable layout testing support for font fallback

crbug.com/285303 describes how family name based font fallback
does not work in the case of ttf files with identical family name.
The two added files, GardinerMod(Bug|Cat).ttf are derivatives from
the public domain Gardiner font. For license information, see
the LayoutTests/third_party/GardinerMod directory in Blink.
They have the same family name, but different available glyphs,
and are used in the family-fallback-gardiner.html test case.

BUG=381684
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281738

[Dominik Röttsches, 2014-06-25 14:16:56]

Variable name case fix.

[Dominik Röttsches, 2014-06-25 14:19:35]

Variable name case fix.

[piman, 2014-06-25 18:43:31]

https://codereview.chromium.org/356733003/diff/20001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/356733003/diff/20001/content/content_shell.gy...
content/content_shell.gypi:412:
'shell/renderer/test_runner/resources/fonts/GardinerModCat.ttf',
third-party data needs to go into third_party, where the origin, license and
modification are documented - see
http://www.chromium.org/developers/adding-3rd-party-libraries

https://codereview.chromium.org/356733003/diff/20001/content/shell/app/webkit...
File content/shell/app/webkit_test_platform_support_linux.cc (right):

https://codereview.chromium.org/356733003/diff/20001/content/shell/app/webkit...
content/shell/app/webkit_test_platform_support_linux.cc:42: const
base::FilePath& base_path, FcConfig* font_config) {
indent looks wrong. Run "git cl format" ?

[Dominik Röttsches, 2014-06-26 11:21:55]

https://codereview.chromium.org/356733003/diff/20001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/356733003/diff/20001/content/content_shell.gy...
content/content_shell.gypi:412:
'shell/renderer/test_runner/resources/fonts/GardinerModCat.ttf',
On 2014/06/25 18:43:31, piman (slow to review) wrote:
> third-party data needs to go into third_party, where the origin, license and
> modification are documented - see
> http://www.chromium.org/developers/adding-3rd-party-libraries

How about referencing them from
third_party/WebKit/LayoutTests/third_party/GardinerMod. I had a longer
discussion with dpranke about what to do with fonts needed for layout testing.
He suggested to put them in this folder.

The fonts I am adding are not relevant to Chrome's UI or in any way essential to
Chrome. They are relevant for Blink layout testing using content_shell. That's
why I think this might actually be the right place for them.

If you see better fit for them on the Chrome side - i can move them to the top
level third_party directory.

https://codereview.chromium.org/356733003/diff/20001/content/shell/app/webkit...
File content/shell/app/webkit_test_platform_support_linux.cc (right):

https://codereview.chromium.org/356733003/diff/20001/content/shell/app/webkit...
content/shell/app/webkit_test_platform_support_linux.cc:42: const
base::FilePath& base_path, FcConfig* font_config) {
On 2014/06/25 18:43:31, piman (slow to review) wrote:
> indent looks wrong. Run "git cl format" ?

Yep, sorry about that. Tried with git cl format now.

[piman, 2014-06-26 19:46:25]

On 2014/06/26 11:21:55, Dominik Röttsches wrote:
>
https://codereview.chromium.org/356733003/diff/20001/content/content_shell.gypi
> File content/content_shell.gypi (right):
> 
>
https://codereview.chromium.org/356733003/diff/20001/content/content_shell.gy...
> content/content_shell.gypi:412:
> 'shell/renderer/test_runner/resources/fonts/GardinerModCat.ttf',
> On 2014/06/25 18:43:31, piman (slow to review) wrote:
> > third-party data needs to go into third_party, where the origin, license and
> > modification are documented - see
> > http://www.chromium.org/developers/adding-3rd-party-libraries
> 
> How about referencing them from
> third_party/WebKit/LayoutTests/third_party/GardinerMod.

That's ok with me if the third_party OWNERs vetted that.

> I had a longer
> discussion with dpranke about what to do with fonts needed for layout testing.
> He suggested to put them in this folder.
> 
> The fonts I am adding are not relevant to Chrome's UI or in any way essential
to
> Chrome. They are relevant for Blink layout testing using content_shell. That's
> why I think this might actually be the right place for them.
> 
> If you see better fit for them on the Chrome side - i can move them to the top
> level third_party directory.

Either location is fine with me. It's mostly about making sure third-party
code/data is in the right place so that it's properly tracked by third_party
OWNERs re: license etc.

>
https://codereview.chromium.org/356733003/diff/20001/content/shell/app/webkit...
> File content/shell/app/webkit_test_platform_support_linux.cc (right):
> 
>
https://codereview.chromium.org/356733003/diff/20001/content/shell/app/webkit...
> content/shell/app/webkit_test_platform_support_linux.cc:42: const
> base::FilePath& base_path, FcConfig* font_config) {
> On 2014/06/25 18:43:31, piman (slow to review) wrote:
> > indent looks wrong. Run "git cl format" ?
> 
> Yep, sorry about that. Tried with git cl format now.

[piman, 2014-06-26 19:46:30]

lgtm

[Dominik Röttsches, 2014-06-26 20:01:01]

The CQ bit was checked by dominik.rottsches@intel.com

[commit-bot: I haz the power, 2014-06-26 20:01:55]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/356733003/...

[Dirk Pranke, 2014-06-26 20:26:20]

The CQ bit was unchecked by dpranke@chromium.org

[jamesr, 2014-06-26 20:26:24]

https://codereview.chromium.org/356733003/diff/40001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/356733003/diff/40001/content/content_shell.gy...
content/content_shell.gypi:412:
'../third_party/WebKit/LayoutTests/third_party/GardinerMod/GardinerModBug.ttf',
this isn't good - things in the chromium repo should not be referencing paths in
third_party/WebKit/. this makes it hard/impossible to change the directory
structure inside the blink repository since it is impossible to atomically
change this file and the file/path in the blink repository.

[jamesr, 2014-06-26 20:32:30]

We have targets in third_party/WebKit/public/ that you can use from chromium if
you need to do some work that's internal to the blink repository. Please use or
add one of those if you need to induce this logic from the chromium side

https://codereview.chromium.org/356733003/diff/40001/content/shell/app/webkit...
File content/shell/app/webkit_test_platform_support_linux.cc (right):

https://codereview.chromium.org/356733003/diff/40001/content/shell/app/webkit...
content/shell/app/webkit_test_platform_support_linux.cc:52: std::cerr << "Failed
to load test font resource "
why doesn't this use base's loggig facilities? i suspect the other use of cerr
dates back from when this code live in a TestRunner limbo between the blink and
chromium repositories and couldn't use WTF or base/, but that hasn't been the
situation for a while now

[Dominik Röttsches, 2014-06-26 20:51:56]

james, isn't this going to be less of a problem since we plan to merge the repos
anyway? 

This is only about adding some fonts and making them available as side loaded
fonts in content shell. What do you suggest doing instead? It's not that I am
doing "work internal to blink", i.e. any functional code or so. It's about
referencing layout test font files. As mentioned in my previous comment - these
files don't really belong in the Chromium repo since they're only meant for
layout testing. Where would you prefer placing them?

Re logging, I can update that, I was just copying how the rest of the code in
that file is doing logging.

[jamesr, 2014-06-26 20:54:16]

On 2014/06/26 20:51:56, Dominik Röttsches wrote:
> james, isn't this going to be less of a problem since we plan to merge the
repos
> anyway? 

Yes, once we merge repos this will be less of a problem.  We have not merged
repos yet so this is still a problem until we do.

> This is only about adding some fonts and making them available as side loaded
> fonts in content shell. What do you suggest doing instead? It's not that I am
> doing "work internal to blink", i.e. any functional code or so. It's about
> referencing layout test font files. As mentioned in my previous comment -
these
> files don't really belong in the Chromium repo since they're only meant for
> layout testing. Where would you prefer placing them?

The rule is simple - things in the chromium repo can only refer to paths inside
blink that are in the third_party/WebKit/public/ directory.  If you want to have
a target in the chromium repo that copies a file around that is inside the blink
repo, you can add a target in the blink side to do the copy and reference it
from third_party/WebKit/public/.

We bundle up the other resources for layout tests on the chromium side, though,
so I'm not sure what use it is to check these files in to the blink repository.

> Re logging, I can update that, I was just copying how the rest of the code in
> that file is doing logging.

[Dominik Röttsches, 2014-06-26 20:54:31]

I mean - the content shell gypi on the chromium side uses local rules to copy
font files into place. Do you suggest I should replicate such copying on the
blink side, to copy files into destinations that content  shell needs?

[jamesr, 2014-06-26 20:55:31]

Will anything other than content shell load these files up?  If not, then I'd
put the files closer to the content shell code.

[Dominik Röttsches, 2014-06-26 20:58:10]

> We bundle up the other resources for layout tests on the chromium side,
though,
> so I'm not sure what use it is to check these files in to the blink
repository.

Okay, so then do I understand correctly top level third_party and not
referencing blink or adding gyp rules in blink for the font is the better choice
in this case? I can move the font to Chromium, sure.

[Dominik Röttsches, 2014-06-26 20:58:41]

On 2014/06/26 20:55:31, jamesr wrote:
> Will anything other than content shell load these files up?  If not, then I'd
> put the files closer to the content shell code.

Alright - thanks.

[Dominik Röttsches, 2014-07-02 14:40:33]

I changed the logging code and moved the font file to third_party/gardiner_mod.

@eae, @behdad, are you okay with being listed as owner for the font file? The
rules in http://www.chromium.org/developers/adding-3rd-party-libraries say it
needs to be a @chromium owner.

Thanks in advance for reviewing this again, jamesr.

[eae, 2014-07-02 15:34:00]

On 2014/07/02 14:40:33, Dominik Röttsches wrote:
> I changed the logging code and moved the font file to
third_party/gardiner_mod.
> 
> @eae, @behdad, are you okay with being listed as owner for the font file? The
> rules in http://www.chromium.org/developers/adding-3rd-party-libraries say it
> needs to be a @chromium owner.

Sure

[Dominik Röttsches, 2014-07-03 07:11:51]

brettw, cpu, ben, darin: Could you review the addition of the font in
src/third_party? Thanks.

[cpu_(ooo_6.6-7.5), 2014-07-03 23:12:53]

lgtm but you need review from the open source license folks

http://www.chromium.org/developers/adding-3rd-party-libraries

[Dominik Röttsches, 2014-07-04 05:49:01]

On 2014/07/03 23:12:53, cpu wrote:
> lgtm but you need review from the open source license folks
> 
> http://www.chromium.org/developers/adding-3rd-party-libraries

I pinged the list and CC'ed it on this bug, but I don't get a response. Who
could I ping specifically? Thank you.

[jschuh, 2014-07-07 17:08:45]

Rubberstamp lgtm for security, since this is just data. However, in the future
please email security with the link to the CL rather than a CC on the CL
(because that tends to get filtered out and ignored).

[Dominik Röttsches, 2014-07-07 21:40:08]

The CQ bit was checked by dominik.rottsches@intel.com

[commit-bot: I haz the power, 2014-07-07 21:40:29]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/356733003/...

[Dominik Röttsches, 2014-07-07 21:49:47]

The CQ bit was unchecked by dominik.rottsches@intel.com

[Dominik Röttsches, 2014-07-08 11:19:33]

The CQ bit was checked by dominik.rottsches@intel.com

[commit-bot: I haz the power, 2014-07-08 11:21:05]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/356733003/...

[Dominik Röttsches, 2014-07-08 11:43:45]

The CQ bit was checked by dominik.rottsches@intel.com

[commit-bot: I haz the power, 2014-07-08 11:45:04]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/356733003/...

[commit-bot: I haz the power, 2014-07-08 13:32:13]

Change committed as 281738

[Dominik Röttsches, 2014-07-08 13:34:25]

On 2014/07/07 17:08:45, Justin Schuh wrote:
> Rubberstamp lgtm for security, since this is just data. However, in the future
> please email security with the link to the CL rather than a CC on the CL
> (because that tends to get filtered out and ignored).

Thanks for the review, Justin. 

I did indeed sent a mail already on 7/2 to
open-source-third-party-reviews@google.com, security@chromium.org with subject
"Review requested for adding modified font file".

Are these still the right mailing lists?

[jschuh, 2014-07-09 13:38:56]

Yeah, security@chromium.org is the right list. Sorry, it must have gone
missed over the holiday.



On Tue, Jul 8, 2014 at 6:34 AM, <dominik.rottsches@intel.com> wrote:

> On 2014/07/07 17:08:45, Justin Schuh wrote:
>
>> Rubberstamp lgtm for security, since this is just data. However, in the
>> future
>> please email security with the link to the CL rather than a CC on the CL
>> (because that tends to get filtered out and ignored).
>>
>
> Thanks for the review, Justin.
>
> I did indeed sent a mail already on 7/2 to
> open-source-third-party-reviews@google.com, security@chromium.org with
> subject
> "Review requested for adding modified font file".
>
> Are these still the right mailing lists?
>
>
> https://codereview.chromium.org/356733003/
>
> --
> --
> -----
> security@chromium.org is for discussing vulnerabilities and fixes in
> Chromium code.Please protect Chromium users: DO NOT FORWARD this email or
> disclose its contents to third parties.
>
> http://groups.google.com/a/chromium.org/group/security
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.