Updating to new OTS repo from https://github.com/khaledhosny/ots.git

Updating with to new OTS repo from https://github.com/khaledhosny/ots.git
Hash tag imported: c24a839b1c66c4de09e58fabaacb82bf3bd692a4 

Some important differences between new and old version of OTS:
1. Support for MATH table
2. Implementation changes related to new WOFF 2.0
3. Bug fixes related to OTF tables (GDEF, GSUB etc) parsing
4. Accept platform 0 encoding 1 cmap subtables
5. Allow fonts with CMAP format-12 or format-13 table

A dummy version of EnableWOFF() is added to get Blink compiled.
After the following CL is landed in Blink, it'll be removed.

Blink CL: https://codereview.chromium.org/774253008

TEST: Blink layout tests: fast/css/font-face*
BUG=339857

[h.joshi, 2014-10-16 10:02:33]

h.joshi@samsung.com changed reviewers:
+ behdad@chromium.org

[h.joshi, 2014-10-16 10:02:34]

Moving to latest code set from https://github.com/khaledhosny/ots.git which
include
Khaled changes for compatibility changes for Old API.

Once this is submitted will make DEPS changes

[behdad_google, 2014-10-16 21:18:01]

behdad@google.com changed reviewers:
+ behdad@google.com

[behdad_google, 2014-10-16 21:18:02]

Generally looks good.  The file permission changes from 644 to 755 looks wrong
though.  Can you fix that please?

https://codereview.chromium.org/658573004/diff/20001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/658573004/diff/20001/.gitignore#newcode1
.gitignore:1: /Makefile
Would be fine if we don't import this.

https://codereview.chromium.org/658573004/diff/20001/.gitmodules
File .gitmodules (right):

https://codereview.chromium.org/658573004/diff/20001/.gitmodules#newcode1
.gitmodules:1: [submodule "third_party/brotli/src"]
I don't think we want to import this file.

[h.joshi, 2014-10-17 05:02:16]

Pls review.

Changed file permission back to 644, file permissions were set to 755 in the
original code which I synced from base code from /external/ots.git.

https://codereview.chromium.org/658573004/diff/20001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/658573004/diff/20001/.gitignore#newcode1
.gitignore:1: /Makefile
On 2014/10/16 21:18:02, behdad_google wrote:
> Would be fine if we don't import this.

Done.

https://codereview.chromium.org/658573004/diff/20001/.gitmodules
File .gitmodules (right):

https://codereview.chromium.org/658573004/diff/20001/.gitmodules#newcode1
.gitmodules:1: [submodule "third_party/brotli/src"]
On 2014/10/16 21:18:02, behdad_google wrote:
> I don't think we want to import this file.

Done.

[h.joshi, 2014-10-18 18:52:25]

@Behdad: Pls review

[jungshik at Google, 2014-10-20 18:13:18]

On 2014/10/18 18:52:25, h.joshi wrote:
> @Behdad: Pls review

Pls, move README.chromium to this CL.

[jungshik at Google, 2014-10-20 18:40:08]

jshin@chromium.org changed reviewers:
+ agl@chromium.org, jshin@chromium.org

[jungshik at Google, 2014-10-20 18:40:10]

Adam,  can you take a look (as the original author of OTS and as a chrome
security team member)? Is there any procedure to follow for importing a 3rd
party library (that is security-critical) to Chromium tree?

[jungshik at Google, 2014-10-20 18:42:27]

In addition to adding README.chromium from another CL of yours, please specify
what version (git hash) you're importing in the CL description. 

Moreover, it'd be nice to list important differences between the old version
(from code.google.com) and this version in the CL description for the future
reference. 

Thanks.

[agl, 2014-10-20 18:42:31]

Not a member of security team and I no longer handle OTS, I'm afraid.

[jungshik at Google, 2014-10-20 21:05:31]

On 2014/10/20 18:42:31, agl wrote:
> Not a member of security team and I no longer handle OTS, I'm afraid.

Sorry that I misinterpreted what's shown at our internal page. 

I'll contact security@chromium.org (and follow the procedure at
http://dev.chromium.org/developers/adding-3rd-party-libraries ).

[h.joshi, 2014-10-21 05:20:40]

Added README.chromium file to this patch.
Updated patch description to include hash tag and list of some important
changes.

[jungshik at Google, 2014-11-26 19:54:31]

On 2014/10/21 05:20:40, h.joshi wrote:
> Added README.chromium file to this patch.
> Updated patch description to include hash tag and list of some important
> changes.

Thank you for the change and sorry for the delay. I think what need to be done
is to drop ots from DEPS file and add a copy of OTS (github version) to
third_party/ots directly (as we do with harfbuzz) as opposed to pulling it from
https:/​/​chromium.googlesource.com/​external/​ots.  

Note that http://dev.chromium.org/developers/adding-3rd-party-libraries has the
following. OTS belongs to the 1st category. 

If the code is applicable and will be compiled on all supported Chromium
platforms (Windows, Mac, Linux, ChromeOS, iOS, Android), check it in to
src/third_party. 

If the code is only applicable to certain platforms, check it in to
src/third_party and add an entry to the DEPS file that causes the code to be
checked out from src/third_party into src/third_party by gclient.

[h.joshi, 2014-12-03 04:18:31]

@jshin: Thank you for guiding me, sorry for the delay was busy with other 
issue patches.

In this CL I have done following steps:
1. Added copy of OTS (github version from
https://github.com/khaledhosny/ots.git)
2. Added Emoji related changes above that (Emoji related changes are not present
in github version)
3. Update "third_party/ots" with above changes.

As suggested, Will make new CL for the DEPS changes, as that is under Chromium
Repo.

I have one doubt, github version of OTS does not have Emoji related changes
while this patch
has those changes, can we use this CL or we have to take only github version ?

[bashi, 2014-12-03 04:29:17]

> I have one doubt, github version of OTS does not have Emoji related changes
> while this patch
> has those changes, can we use this CL or we have to take only github version ?

I think it's OK, given that blink cannot handle Emoji web fonts AFAIK.

[h.joshi, 2014-12-03 04:35:53]

@bashi1: So I have update current patch with github version?
Or this patch will work?

[bashi, 2014-12-03 07:39:19]

On 2014/12/03 04:35:53, h.joshi wrote:
> @bashi1: So I have update current patch with github version?
> Or this patch will work?

Sorry for the confusion. According to the comment #14, it seems that jshin@ may
want to update the CL. But I'm not sure it's the current plan.

jshin@, behdad@, could you provide information h.joshi@ is needed when it's
ready?

@h.joshi, it will take time, so please wait for a while. Thank you for working
on this again!

[jungshik at Google, 2014-12-03 19:58:17]

On 2014/12/03 04:29:17, bashi1 wrote:
> > I have one doubt, github version of OTS does not have Emoji related changes
> > while this patch
> > has those changes, can we use this CL or we have to take only github version
?
> 
> I think it's OK, given that blink cannot handle Emoji web fonts AFAIK.

Actually, Emoji web fonts work if OTS passes the table according to bungeman@.

[jungshik at Google, 2014-12-03 20:27:53]

On 2014/12/03 04:35:53, h.joshi wrote:
> @bashi1: So I have update current patch with github version?

Let's start with the gitbub version. 

@behdad, can you merge the Emoji-related changes to the github version? 

Once it's done, we can update our tree once more. 


> Or this patch will work?

What need to be done is :

0. Make a new work  branch in the topmost chromium tree (src/) 
1. Drop the reference to OTS  in DEPS (we'll not use DEPS to pull ots any more.
Instead, it'll be just a part of the topmost chromium repo.) 
2. Delete third_party/ots
3. gclient sync
4. Import the github version to third_party/ots afresh
5. Add README.chromium 
6. git commit && git cl upload

[chromium-reviews, 2014-12-03 20:28:55]

I'll figure that out today.

On Wed, Dec 3, 2014, 12:27 PM null <jshin@chromium.org> wrote:

> On 2014/12/03 04:35:53, h.joshi wrote:
> > @bashi1: So I have update current patch with github version?
>
> Let's start with the gitbub version.
>
> @behdad, can you merge the Emoji-related changes to the github version?
>
> Once it's done, we can update our tree once more.
>
>
> > Or this patch will work?
>
> What need to be done is :
>
> 0. Make a new work  branch in the topmost chromium tree (src/)
> 1. Drop the reference to OTS  in DEPS (we'll not use DEPS to pull ots any
> more.
> Instead, it'll be just a part of the topmost chromium repo.)
> 2. Delete third_party/ots
> 3. gclient sync
> 4. Import the github version to third_party/ots afresh
> 5. Add README.chromium
> 6. git commit && git cl upload
>
>
> https://codereview.chromium.org/658573004/
>

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

[h.joshi, 2014-12-04 06:13:51]

@Jungshik Shin: Thank you for detailed comments.

I followed the steps, currently facing problem when trying to upload changes.
Getting following error:
checkperms.py failed.
  /opensource/blinkLinux/src/third_party/ots/test/test_malicious_fonts.sh \
  /opensource/blinkLinux/src/third_party/ots/test/test_unmalicious_fonts.sh \
  /opensource/blinkLinux/src/third_party/ots/tools/ttf-checksum.py

No other information related to error is shown, trying to fix the same.

[h.joshi, 2014-12-04 07:05:01]

Issue was due to difference in file bit permission settings, corrected the same.

[h.joshi, 2014-12-04 07:55:31]

New Blink patch is also submitted: https://codereview.chromium.org/774253008

 This is required as "ots::EnableWOFF2()" is no longer part of new OTS.

[h.joshi, 2014-12-04 14:45:02]

@Behdad: Should I make patch to merger Emoji-related changes to the github
version?
I already have patch for that, pls suggest.

[jungshik at Google, 2014-12-04 21:08:24]

On 2014/12/04 07:55:31, h.joshi wrote:
> New Blink patch is also submitted: https://codereview.chromium.org/774253008
> 
>  This is required as "ots::EnableWOFF2()" is no longer part of new OTS.

I thought that you meant the blink CL had been landed. It's not yet landed. We
have to make sure that this CL is ready to be landed before landing the blink CL
( https://codereview.chromium.org/774253008 ). Once this CL is ready, we can do
the following:

1. Land the blink CL. We have to disable blink tests (layout) that uses WOFF2 in
the blink CL. 
2. Wait for Blink with the Blink CL (774253008) to be rolled in
3. Land this CL
4. Re-enable the Blink tests disabled in step 1.

[behdad_google, 2014-12-04 21:14:52]

On 2014/12/04 14:45:02, h.joshi wrote:
> @Behdad: Should I make patch to merger Emoji-related changes to the github
> version?
> I already have patch for that, pls suggest.

Please send a Pull Request on github to Khaled with the changes and mention me
(@behdad).

[h.joshi, 2014-12-05 06:57:11]

Dummy ots::EnableWOFF method is added.

[h.joshi, 2014-12-05 09:13:48]

In Windows bots, issue is due to "forcing value to bool 'true' or 'false'",
checking the same.

[h.joshi, 2014-12-05 12:41:21]

With dummy "ots::EnableWOFF" method, patch is compiling on Windows,Mac,Linux
bots.

[jungshik at Google, 2014-12-05 18:40:30]

jshin@chromium.org changed reviewers:
+ bashi@chromium.org
- agl@chromium.org

[jungshik at Google, 2014-12-05 18:40:32]

In addition to my comment below about README.chromium, please update the CL
description and mention that a dummy EnableWOFF is added temporarily.

https://codereview.chromium.org/658573004/diff/160001/third_party/ots/README....
File third_party/ots/README.chromium (right):

https://codereview.chromium.org/658573004/diff/160001/third_party/ots/README....
third_party/ots/README.chromium:3: Version:
7710e6ed23fb2fed146b2d051bc313b7538e1304
Both URL and Version (git hash) fields have to be updated. The upstream is now
github.

[jungshik at Google, 2014-12-05 18:43:14]

> Depends on https://codereview.chromium.org/774253008

This is not the case any more. How about this in the CL description? 

A dummy version of EnableWOFF() is added to get Blink compiled. After the
following CL is landed in Blink, it'll be removed. 

https://codereview.chromium.org/774253008

[jungshik at Google, 2014-12-05 18:46:08]

On 2014/12/05 18:43:14, Jungshik Shin wrote:
> > Depends on https://codereview.chromium.org/774253008
> 
> This is not the case any more. How about this in the CL description? 
> 
> A dummy version of EnableWOFF() is added to get Blink compiled. After the
> following CL is landed in Blink, it'll be removed. 
> 
> https://codereview.chromium.org/774253008

One more change in the CL description. You need to add TEST line as following:

TEST: Blink layout tests: fast/css/font-face*

[h.joshi, 2014-12-08 08:55:47]

Made changes to README.chromium with correct github link.

[jungshik at Google, 2014-12-08 19:36:29]

On 2014/12/08 08:55:47, h.joshi wrote:
> Made changes to README.chromium with correct github link.

Thank you for the changes. LGTM

[jungshik at Google, 2014-12-08 19:37:25]

On 2014/12/08 19:36:29, Jungshik Shin wrote:
> On 2014/12/08 08:55:47, h.joshi wrote:
> > Made changes to README.chromium with correct github link.
> 
> Thank you for the changes. LGTM

To make it clear, we got all the approvals (offline) so that you're ready to go.

[jungshik at Google, 2014-12-08 19:43:42]

https://codereview.chromium.org/658573004/diff/160001/third_party/ots/ots.gyp
File third_party/ots/ots.gyp (right):

https://codereview.chromium.org/658573004/diff/160001/third_party/ots/ots.gyp...
third_party/ots/ots.gyp:36: 4800,
You may file a bug upstream so that that can be fixed in the upstream. (i.e.
C4800 warning is not issued any more by MSVC). 

After filing an upstream bug, add a comment pointing to that bug here.

[jungshik at Google, 2014-12-09 00:38:13]

On 2014/12/08 19:43:42, Jungshik Shin wrote:
> https://codereview.chromium.org/658573004/diff/160001/third_party/ots/ots.gyp
> File third_party/ots/ots.gyp (right):
> 
>
https://codereview.chromium.org/658573004/diff/160001/third_party/ots/ots.gyp...
> third_party/ots/ots.gyp:36: 4800,
> You may file a bug upstream so that that can be fixed in the upstream. (i.e.
> C4800 warning is not issued any more by MSVC). 
> 
> After filing an upstream bug, add a comment pointing to that bug here.

One more change is necessary; Add BUILD.gn

See https://code.google.com/p/ots/source/browse/trunk/BUILD.gn

[h.joshi, 2014-12-09 04:35:06]

@Jungshik Shin: Thank you.

Github bug for C4800 is filed and comment added pointing to that bug here:
>> https://github.com/khaledhosny/ots/issues/44

One more change is necessary; Add BUILD.gn:
>> Done

Pls review, as new file is added to patch.

[jungshik at Google, 2014-12-09 19:31:03]

With what's below taken care of, LGTM.

https://codereview.chromium.org/658573004/diff/200001/third_party/ots/ots.gyp
File third_party/ots/ots.gyp (right):

https://codereview.chromium.org/658573004/diff/200001/third_party/ots/ots.gyp...
third_party/ots/ots.gyp:36: 4800,
It looks like I failed to get you to understand what I meant. 
 
4800 is NOT covered by the above cr bug. So, you can do the following:

1) Add a reference to the github bug you filed
2) Fix the OTS code so that MSVC does not issue C4800 any more and make a pull
request to the upstream.  How many 4800 warnings do you get? If it's not that
many, it should be pretty simple to fix, shouldn't it?

[jungshik at Google, 2014-12-10 07:22:19]

You have to get gn builds succeed.

https://codereview.chromium.org/658573004/diff/200001/third_party/ots/BUILD.gn
File third_party/ots/BUILD.gn (right):

https://codereview.chromium.org/658573004/diff/200001/third_party/ots/BUILD.g...
third_party/ots/BUILD.gn:13: "src/cbdt.cc",
gn build bots failed to compile. They have this error:

ninja -C /Volumes/data/b/build/slave/mac_gn/build/src/out/Release all -j50
ninja: Entering directory
`/Volumes/data/b/build/slave/mac_gn/build/src/out/Release'
ninja: error: '../../third_party/ots/src/cbdt.cc', needed by
'obj/third_party/ots/src/ots.cbdt.o', missing and no known rule to make it


cbdt.{h,cc} are not included in this CL (and they're not listed in
ots_common.gypi, either).  You're planning to add it in the next CL, aren't you?
 Please, drop them from BUILD.gn for now.

[h.joshi, 2014-12-11 05:16:59]

@Jungshik Shin: Yes these files will be added in the next review process.
Will correct Build.gm and test.

[h.joshi, 2014-12-11 09:54:52]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-12-11 09:56:23]

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

[commit-bot: I haz the power, 2014-12-11 09:56:25]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[h.joshi, 2014-12-11 11:11:02]

Patchset #13 (id:240001) has been deleted

[h.joshi, 2014-12-11 11:25:43]

Getting following error when trying to submit patch:
Error: "Commit queue rejected this change because it did not recognize the base
URL, Please commit your change manually."

Current Base URL: https://chromium.googlesource.com/external/ots@master
This is the very first URL which was used when changes were limited to OTS
folder only. 
new changes are in base Chromium folder i;e "/src"

"git cl baseurl" can change baseurl but that is not working for me.
Pls suggest

[h.joshi, 2014-12-11 13:14:33]

In some sites it written that for such base url issues we need to create new
patch URL.
I have uploaded current changes to new URL:
https://codereview.chromium.org/775893002/

Tried few more things, but not able to change base URL.