Roll HarfBuzz to 0.9.32

Rolling HarfBuzz to 0.9.32

Windows build fix cherry-picked.
 
BUG=394484
R=eae,bashi,behdad

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

[bashi, 2014-07-17 12:18:48]

Thank you for working on this!  Could you submit try jobs?

https://codereview.chromium.org/396393005/diff/1/third_party/harfbuzz-ng/BUIL...
File third_party/harfbuzz-ng/BUILD.gn (right):

https://codereview.chromium.org/396393005/diff/1/third_party/harfbuzz-ng/BUIL...
third_party/harfbuzz-ng/BUILD.gn:59: "src/hb-ot-font.h",
Do we really need this (at this moment)? In other words, can we use this and
remove HarfBuzzFace{Skia,CoreText}.cpp?

[Dominik Röttsches, 2014-07-17 12:25:30]

On 2014/07/17 12:18:48, bashi1 wrote:
> Thank you for working on this!  Could you submit try jobs?

Yes.

>
https://codereview.chromium.org/396393005/diff/1/third_party/harfbuzz-ng/BUIL...
> File third_party/harfbuzz-ng/BUILD.gn (right):
> 
>
https://codereview.chromium.org/396393005/diff/1/third_party/harfbuzz-ng/BUIL...
> third_party/harfbuzz-ng/BUILD.gn:59: "src/hb-ot-font.h",
> Do we really need this (at this moment)? In other words, can we use this and
> remove HarfBuzzFace{Skia,CoreText}.cpp?

As far as I remember my discussion with Behdad - the latter is indeed the plan,
even though it's not complete yet: Move out some of the font parsing functions
from the HarfBuzzFace* files into HarfBuzz, this is also part of the work for
removing the CoreText shaper.

[bashi, 2014-07-17 12:34:02]

On 2014/07/17 12:25:30, Dominik Röttsches wrote:
> On 2014/07/17 12:18:48, bashi1 wrote:
> > Thank you for working on this!  Could you submit try jobs?
> 
> Yes.
> 
> >
>
https://codereview.chromium.org/396393005/diff/1/third_party/harfbuzz-ng/BUIL...
> > File third_party/harfbuzz-ng/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/396393005/diff/1/third_party/harfbuzz-ng/BUIL...
> > third_party/harfbuzz-ng/BUILD.gn:59: "src/hb-ot-font.h",
> > Do we really need this (at this moment)? In other words, can we use this and
> > remove HarfBuzzFace{Skia,CoreText}.cpp?
> 
> As far as I remember my discussion with Behdad - the latter is indeed the
plan,
> even though it's not complete yet: Move out some of the font parsing functions
> from the HarfBuzzFace* files into HarfBuzz, this is also part of the work for
> removing the CoreText shaper.

Thanks for explanation. Can we defer adding these files until they are ready? We
don't want to add unused files to keep binary size as small as we can.

[Dominik Röttsches, 2014-07-17 12:55:35]

On 2014/07/17 12:34:02, bashi1 wrote:
> On 2014/07/17 12:25:30, Dominik Röttsches wrote:
> > On 2014/07/17 12:18:48, bashi1 wrote:
> > > Thank you for working on this!  Could you submit try jobs?
> > 
> > Yes.
> > 
> > >
> >
>
https://codereview.chromium.org/396393005/diff/1/third_party/harfbuzz-ng/BUIL...
> > > File third_party/harfbuzz-ng/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/396393005/diff/1/third_party/harfbuzz-ng/BUIL...
> > > third_party/harfbuzz-ng/BUILD.gn:59: "src/hb-ot-font.h",
> > > Do we really need this (at this moment)? In other words, can we use this
and
> > > remove HarfBuzzFace{Skia,CoreText}.cpp?
> > 
> > As far as I remember my discussion with Behdad - the latter is indeed the
> plan,
> > even though it's not complete yet: Move out some of the font parsing
functions
> > from the HarfBuzzFace* files into HarfBuzz, this is also part of the work
for
> > removing the CoreText shaper.
> 
> Thanks for explanation. Can we defer adding these files until they are ready?
We
> don't want to add unused files to keep binary size as small as we can.

Ultimately, yours and Behdad's call. However, I also find it easier to merge and
roll a Harfbuzz release if we don't do custom changes on the Chromium side when
it comes to which files to include in the build etc (the files are not
optionally built in HarfBuzz's makefile) since it's already pretty more
complicated to merge than it should be. I plan to pick up the work on the
Coretext removal again quite soon - and it would also speed up this work if I
wouldn't have to have another review and merge on the Chromium side.

[bashi, 2014-07-17 13:22:34]

On 2014/07/17 12:55:35, Dominik Röttsches wrote:
> On 2014/07/17 12:34:02, bashi1 wrote:
> > On 2014/07/17 12:25:30, Dominik Röttsches wrote:
> > > On 2014/07/17 12:18:48, bashi1 wrote:
> > > > Thank you for working on this!  Could you submit try jobs?
> > > 
> > > Yes.
> > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/396393005/diff/1/third_party/harfbuzz-ng/BUIL...
> > > > File third_party/harfbuzz-ng/BUILD.gn (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/396393005/diff/1/third_party/harfbuzz-ng/BUIL...
> > > > third_party/harfbuzz-ng/BUILD.gn:59: "src/hb-ot-font.h",
> > > > Do we really need this (at this moment)? In other words, can we use this
> and
> > > > remove HarfBuzzFace{Skia,CoreText}.cpp?
> > > 
> > > As far as I remember my discussion with Behdad - the latter is indeed the
> > plan,
> > > even though it's not complete yet: Move out some of the font parsing
> functions
> > > from the HarfBuzzFace* files into HarfBuzz, this is also part of the work
> for
> > > removing the CoreText shaper.
> > 
> > Thanks for explanation. Can we defer adding these files until they are
ready?
> We
> > don't want to add unused files to keep binary size as small as we can.
> 
> Ultimately, yours and Behdad's call. However, I also find it easier to merge
and
> roll a Harfbuzz release if we don't do custom changes on the Chromium side
when
> it comes to which files to include in the build etc (the files are not
> optionally built in HarfBuzz's makefile) since it's already pretty more
> complicated to merge than it should be. I plan to pick up the work on the
> Coretext removal again quite soon - and it would also speed up this work if I
> wouldn't have to have another review and merge on the Chromium side.

I agree with rolling Harfbuzz is a pain now. We should improve. But don't we
need another roll to pull your work on coretext removal? If we don't, I'm fine
including the files.

Could you also address compile errors on win?

[behdad_google, 2014-07-17 16:28:17]

On 2014/07/17 12:34:02, bashi1 wrote:
> On 2014/07/17 12:25:30, Dominik Röttsches wrote:
> > On 2014/07/17 12:18:48, bashi1 wrote:
> > > Thank you for working on this!  Could you submit try jobs?
> > 
> > Yes.
> > 
> > >
> >
>
https://codereview.chromium.org/396393005/diff/1/third_party/harfbuzz-ng/BUIL...
> > > File third_party/harfbuzz-ng/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/396393005/diff/1/third_party/harfbuzz-ng/BUIL...
> > > third_party/harfbuzz-ng/BUILD.gn:59: "src/hb-ot-font.h",
> > > Do we really need this (at this moment)? In other words, can we use this
and
> > > remove HarfBuzzFace{Skia,CoreText}.cpp?
> > 
> > As far as I remember my discussion with Behdad - the latter is indeed the
> plan,
> > even though it's not complete yet: Move out some of the font parsing
functions
> > from the HarfBuzzFace* files into HarfBuzz, this is also part of the work
for
> > removing the CoreText shaper.
> 
> Thanks for explanation. Can we defer adding these files until they are ready?
We
> don't want to add unused files to keep binary size as small as we can.

Doesn't the final linker remove unused code?

[behdad_google, 2014-07-17 20:12:53]

So, apparently 0.9.31 wasn't enough for fixing the Mongolian issue.  I already
released 0.9.32 which should fix it IMO.  But lets wait till we confirm the fix
before updating again.

Sorry about the trouble :(.

Also, lets make rolling HarfBuzz easier.  How can we do that?  What are the
current pain points?

[Dominik Röttsches, 2014-07-17 21:56:07]

On 2014/07/17 20:12:53, behdad_google wrote:
> So, apparently 0.9.31 wasn't enough for fixing the Mongolian issue.  I already
> released 0.9.32 which should fix it IMO.  But lets wait till we confirm the
fix
> before updating again.
> 
> Sorry about the trouble :(.

No probs, for 0.9.33, can you do something about the windows compile warning as
well? (see win_chromium_rel compile log:)

e:\b\build\slave\win\build\src\third_party\harfbuzz-ng\src\hb-private.hh(798) :
warning C4101: 'error_hb_in_range_called_with_signed_type' : unreferenced local
variable
       
e:\b\build\slave\win\build\src\third_party\harfbuzz-ng\src\hb-unicode-private.hh(179)
: see reference to function template instantiation 'bool
hb_in_range<hb_codepoint_t>(T,T,T)' being compiled

> Also, lets make rolling HarfBuzz easier.  How can we do that?  What are the
> current pain points?

1) The generated .hh files which are not in the repository are hard to track.
2) Buildsystem Translation is time consuming.

What I do for updating Harfbuzz, I generate a patch from the old version to the
current one, apply that, then check the patch for changes in the Makefile and
transfer those to gyp and BUILD.gn. The patch however does not contain .hh
updates, so then I need to checkout hb, compile and copy the generated .hh files
over, too. Also, the patch doesn't apply cleanly since you need to skip a couple
of files which have been removed in Chromium's copy of harfbuzz.

How to improve:
Ideally, rolling harfbuzz would be something like copying upstream into
third_party/harfbuzz-ng - or similar to the above procedure: keeping harfbuzz in
a way that generating a patch from old to new version and applying it to
chromium's third_party/harfbuzz-ng would be sufficient.

Other suggestions are welcome.

[behdad, 2014-07-17 22:35:42]

On 2014/07/17 21:56:07, Dominik Röttsches wrote:
> On 2014/07/17 20:12:53, behdad_google wrote:
> > So, apparently 0.9.31 wasn't enough for fixing the Mongolian issue.  I
already
> > released 0.9.32 which should fix it IMO.  But lets wait till we confirm the
> fix
> > before updating again.
> > 
> > Sorry about the trouble :(.
> 
> No probs, for 0.9.33, can you do something about the windows compile warning
as
> well? (see win_chromium_rel compile log:)
> 
> e:\b\build\slave\win\build\src\third_party\harfbuzz-ng\src\hb-private.hh(798)
:
> warning C4101: 'error_hb_in_range_called_with_signed_type' : unreferenced
local
> variable
>        
>
e:\b\build\slave\win\build\src\third_party\harfbuzz-ng\src\hb-unicode-private.hh(179)
> : see reference to function template instantiation 'bool
> hb_in_range<hb_codepoint_t>(T,T,T)' being compiled

I pushed a commit out that I think should fix this.


> > Also, lets make rolling HarfBuzz easier.  How can we do that?  What are the
> > current pain points?
> 
> 1) The generated .hh files which are not in the repository are hard to track.
> 2) Buildsystem Translation is time consuming.
> 
> What I do for updating Harfbuzz, I generate a patch from the old version to
the
> current one, apply that,

Ok, I think we want to make sure the source files have a diff of zero, so we can
always copy src/hb*.cc src/hb*.hh from a harfbuzz git checkout or tarball to
Chromium without worrying about changes.  In fact, I see that even files like
test* exist in Chromium's copy.  So I think we should aim for an update
procedure like this:

  - Delete third_party/harfbuzz-ng/src
  - cp *.cc *.hh from harfbuzz/src from a tarball
  - Run git status to see what files to add / remove from build file


> The patch however does not contain .hh

The tarballs contain all the generated .hh files, so we should be good.

[Dominik Röttsches, 2014-07-18 11:21:58]

Thanks, Behdad, for fixes and explanations. Your suggested update strategy
sounds fine. I tried it for rolling to 0.9.32 now. Corner cases like new
conditionally compiled files (e.g. coretext) should get a special mention in the
release notes, then we should be good.

So, 0.9.32 includes the CoreText build fix, plus, I cherry picked the windows
build fix. Let's see how the bots are doing with this one.

Bashi: As far as I can foresee today, there's no extra roll required for the
CoreText shaper removal (unless we find more issues).

[bashi, 2014-07-18 11:36:55]

On 2014/07/18 11:21:58, Dominik Röttsches wrote:
> Thanks, Behdad, for fixes and explanations. Your suggested update strategy
> sounds fine. I tried it for rolling to 0.9.32 now. Corner cases like new
> conditionally compiled files (e.g. coretext) should get a special mention in
the
> release notes, then we should be good.
> 
> So, 0.9.32 includes the CoreText build fix, plus, I cherry picked the windows
> build fix. Let's see how the bots are doing with this one.
> 
> Bashi: As far as I can foresee today, there's no extra roll required for the
> CoreText shaper removal (unless we find more issues).

That's great! LGTM if bots are happy. Behdad will confirm as an OWNER.

Speaking easier roll, we might want to write a tiny script which does what
Behdad suggested. I think we can put the script in third_party/harfbuzz-ng. I
can write it if no one willing to do:)

Behdad: I think Dominik (and maybe me, just for rolling) should be an OWNER.
WDYT?

[behdad_google, 2014-07-18 15:30:35]

On 2014/07/18 11:36:55, bashi1 wrote:

> Speaking easier roll, we might want to write a tiny script which does what
> Behdad suggested. I think we can put the script in third_party/harfbuzz-ng. I
> can write it if no one willing to do:)

You and Dominik would know best.  I find it easy enough with copying.  I haven't
done it in Chromium in a long while though, so don't know.  At least a note on
how to do it would be great.

> Behdad: I think Dominik (and maybe me, just for rolling) should be an OWNER.
> WDYT?

I thought you *were* in there.  Yes, definitely.  If anyone doesn't belong it's
me :)).  Please submit a CL.

[Dominik Röttsches, 2014-07-18 18:10:58]

It contains the other patch from
https://github.com/behdad/harfbuzz/commit/e3b42f1af409c073b819bfc696024ccb1f1...
as well now.

Okay to land it then? Or do you need more time for fixing Mongolian?

https://codereview.chromium.org/396393005/diff/40001/third_party/harfbuzz-ng/...
File third_party/harfbuzz-ng/README.chromium (right):

https://codereview.chromium.org/396393005/diff/40001/third_party/harfbuzz-ng/...
third_party/harfbuzz-ng/README.chromium:14: This copy of harfbuzz is updated by
downloading the release tarball
Notes how to update are already here.

[Dominik Röttsches, 2014-07-18 18:28:29]

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

[Dominik Röttsches, 2014-07-18 18:29:28]

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

[Dominik Röttsches, 2014-07-18 18:34:18]

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

[commit-bot: I haz the power, 2014-07-18 18:34:52]

CQ is trying da patch. Follow status at

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

[behdad_google, 2014-07-18 18:40:29]

On 2014/07/18 18:10:58, Dominik Röttsches wrote:
> It contains the other patch from
>
https://github.com/behdad/harfbuzz/commit/e3b42f1af409c073b819bfc696024ccb1f1...
> as well now.
> 
> Okay to land it then? Or do you need more time for fixing Mongolian?

The Mongolian issue is fixed as far as I understand.  Thanks!

[behdad_google, 2014-07-18 19:05:40]

lgtm

[behdad_google, 2014-07-18 19:05:43]

lgtm

[eae, 2014-07-18 19:09:15]

LGTM.

[behdad%chromium.org, 2014-07-18 19:09:27]

lgtm

[behdad%chromium.org, 2014-07-18 19:09:28]

lgtm

[commit-bot: I haz the power, 2014-07-18 19:09:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-18 19:12:35]

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

[commit-bot: I haz the power, 2014-07-18 19:12:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Dominik Röttsches, 2014-07-18 19:27:07]

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

[commit-bot: I haz the power, 2014-07-18 19:30:03]

CQ is trying da patch. Follow status at

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

[eae, 2014-07-18 19:49:26]

presubmit is failing due to missing approval. Needs a harfbuzz OWNER, i.e. one
of:

jshin@chromium.org
behdad@chromium.org

Behdad has approved it from behdad@google.com and
behdad%chromium.org@gtempaccount.com. Neither of which counts.

We should probably add Dominik and/or myself to the list of harfbuzz OWNERS.

[behdad_google, 2014-07-18 20:02:06]

On 2014/07/18 19:49:26, eae wrote:
> presubmit is failing due to missing approval. Needs a harfbuzz OWNER, i.e. one
> of:
> 
> mailto:jshin@chromium.org
> mailto:behdad@chromium.org
> 
> Behdad has approved it from mailto:behdad@google.com and
> mailto:behdad%chromium.org@gtempaccount.com. Neither of which counts.

My behdad@chromium.org is b0rked and my multiple attempts to fix it didn't get
far.

> We should probably add Dominik and/or myself to the list of harfbuzz OWNERS.

Please add you, Dominik, and bashi.

[Dominik Röttsches, 2014-07-18 20:15:51]

On 2014/07/18 20:02:06, behdad_google wrote:
> On 2014/07/18 19:49:26, eae wrote:

> > We should probably add Dominik and/or myself to the list of harfbuzz OWNERS.
> 
> Please add you, Dominik, and bashi.

https://codereview.chromium.org/407583004

[commit-bot: I haz the power, 2014-07-18 22:11:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-18 22:18:46]

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

[commit-bot: I haz the power, 2014-07-18 22:18:48]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Dominik Röttsches, 2014-07-19 06:07:32]

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

[commit-bot: I haz the power, 2014-07-19 06:08:51]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-07-19 06:27:23]

Change committed as 284324