New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 601739 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

HarfBuzz 1.2.1 regression in FixedVersion::to_int()

Project Member Reported by kojii@chromium.org, Apr 8 2016

Issue description

This is split from  issue 601528 , only for the regression part
 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 8 2016

Comment 2 by kojii@chromium.org, Apr 8 2016

Cc: drott@chromium.org kojii@chromium.org js...@chromium.org e...@chromium.org behdad@chromium.org
Labels: -Pri-3 OS-All Pri-1
Owner: drott@chromium.org
Status: Assigned (was: Untriaged)
From the comment #8 of  issue 601528 , I'm splitting regression part from the roll to 1.2.5.

drott@ or behdad@, before adding Merge-Request-50 label, can you explain what kind of regression this fixes, so that TPM can triage? According to tkent@, the bar to merge to M50 is quite high at this point.

Comment 3 by kojii@chromium.org, Apr 8 2016

Summary: HarfBuzz 1.2.1 regression in FixedVersion::to_int() (was: HarfBuzz FixedVersion::to_int() has a regression)

Comment 4 by drott@chromium.org, Apr 11 2016

Also, can we exclude that this one-line fix would re-introduce the "Times New Roman" quotation mark issue?

Comment 5 by kojii@chromium.org, Apr 11 2016

Are you asking whether this fix breaks "Times New Roman" quotation mark or not? I don't think it does, bots pass this change while 1.2.5 didn't.

Shall we just try Merge-Request-50 label without adding further information, or will you be able to describe what issues this fixes?

Comment 6 by e...@chromium.org, Apr 11 2016

Labels: Merge-Request-50

Comment 7 by tin...@google.com, Apr 11 2016

Labels: -Merge-Request-50 Merge-Review-50 Hotlist-Merge-Review
[Automated comment] Less than 2 weeks to go before stable on M50, manual review required.
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/f764f879976eb07bc6efa83fdab269d1e20866ac

commit f764f879976eb07bc6efa83fdab269d1e20866ac
Author: Jungshik Shin <jshin@chromium.org>
Date: Sat Apr 09 00:08:32 2016

Cherry pick an upstream patch (commit: 6dd80faf0)

This cherry-pick is recommended by the upstream maintainer
behdad@behdad.org. This is to make a merging to R50 branch
easier and is the same cherry-pick was done for Chromium.

For the trunk, this will be followed by upgrading to 1.2.[56].

BUG= chromium:601528 ,  chromium:601739 
TEST=emerge-${BOARD} harfbuzz

Change-Id: If900942f0b461ddb15f84deb004438b81548a3f4
Reviewed-on: https://chromium-review.googlesource.com/338004
Commit-Ready: Jungshik Shin <jshin@chromium.org>
Tested-by: Jungshik Shin <jshin@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/f764f879976eb07bc6efa83fdab269d1e20866ac/media-libs/harfbuzz/harfbuzz-1.2.3.ebuild
[add] https://crrev.com/f764f879976eb07bc6efa83fdab269d1e20866ac/media-libs/harfbuzz/files/harfbuzz-1.2.3-fixedversion2int.patch
[rename] https://crrev.com/f764f879976eb07bc6efa83fdab269d1e20866ac/media-libs/harfbuzz/harfbuzz-1.2.3-r2.ebuild

Comment 9 by js...@chromium.org, Apr 12 2016

Both (non-CrOS and CrOS) changes are extremely safe (it's a trivial one-line fix). 


Comment 10 by kojii@chromium.org, Apr 12 2016

In case this helps the merge reviewer, this issue breaks some Arabic fonts rendering.

Firefox merged this patch to all affected releases:
https://bugzilla.mozilla.org/show_bug.cgi?id=1262774

Comment 11 by tin...@google.com, Apr 12 2016

Labels: -Merge-Review-50 Merge-Approved-50
Merge approved for M50 (branch 2661). Pls go ahead merge.

Comment 12 by js...@chromium.org, Apr 12 2016

Thanks, Tina. I'll merge CrOS CL.  I'll go with Chromium CL, too if drott@ is not around (it's late). 

Project Member

Comment 13 by bugdroid1@chromium.org, Apr 12 2016

Labels: -merge-approved-50 merge-merged-2661
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/12bddafcdd166cdc0471c92b8f079932dfde8a38

commit 12bddafcdd166cdc0471c92b8f079932dfde8a38
Author: Dominik Röttsches <drott@chromium.org>
Date: Tue Apr 12 20:36:35 2016

Cheery pick Fix FixedVersion::to_int()

Cherry pick to fix a regression in mark-filtering-sets in GDEF.

https://github.com/behdad/harfbuzz/commit/6dd80fa

BUG= 601528 ,  601739 

Review URL: https://codereview.chromium.org/1869993002

Cr-Commit-Position: refs/heads/master@{#386039}
(cherry picked from commit 62cf1d88109abbf7020fc01ca6c32a76d133230d)

Review URL: https://codereview.chromium.org/1878053003 .

Cr-Commit-Position: refs/branch-heads/2661@{#570}
Cr-Branched-From: ef6f6ae5e4c96622286b563658d5cd62a6cf1197-refs/heads/master@{#378081}

[modify] https://crrev.com/12bddafcdd166cdc0471c92b8f079932dfde8a38/third_party/harfbuzz-ng/README.chromium
[modify] https://crrev.com/12bddafcdd166cdc0471c92b8f079932dfde8a38/third_party/harfbuzz-ng/src/hb-open-type-private.hh

Comment 14 by js...@chromium.org, Apr 12 2016

Labels: Merge-Request-51
The CrOS cherry-pick of upstream fix was landed too late for the initial R51 branch (cut on Friday). 

Requesting for merge to R51 for CrOS only. (NOT necessary for other platforms). 


Again, this change is very safe and Chrome already has it in M51 as well as in M50.
 
The CL to merge is https://chromium-review.googlesource.com/338004

Cc: bhthompson@chromium.org
Ccing bhthompson@ for CrOS M51 merge request [Please see comment #14 for more details]
Labels: -Merge-Request-51 Merge-Approved-51

Comment 17 Deleted

Please merge your change to M51 branch 2704 ASAP (before 5:00 PM PST, today) so we can take it in for M51 last Dev release tomorrow.
Project Member

Comment 19 by bugdroid1@chromium.org, Apr 18 2016

Labels: merge-merged-release-R51-8172.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/27a7794d68413db81587268baca6485993b20595

commit 27a7794d68413db81587268baca6485993b20595
Author: Jungshik Shin <jshin@chromium.org>
Date: Sat Apr 09 00:08:32 2016

Cherry pick an upstream patch (commit: 6dd80faf0)

This cherry-pick is recommended by the upstream maintainer
behdad@behdad.org. This is to make a merging to R50 branch
easier and is the same cherry-pick was done for Chromium.

For the trunk, this will be followed by upgrading to harfbuzz 1.2.[56].

BUG= chromium:601528 ,  chromium:601739 
TEST=emerge-${BOARD} harfbuzz

Change-Id: If900942f0b461ddb15f84deb004438b81548a3f4
Previous-Reviewed-on: https://chromium-review.googlesource.com/338004
(cherry picked from commit 1afcc6053eedb88bedf9a61b451cc7d4de273f47)
Reviewed-on: https://chromium-review.googlesource.com/339433
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Jungshik Shin <jshin@chromium.org>
Commit-Queue: Jungshik Shin <jshin@chromium.org>
Tested-by: Jungshik Shin <jshin@chromium.org>

[modify] https://crrev.com/27a7794d68413db81587268baca6485993b20595/media-libs/harfbuzz/harfbuzz-1.2.3.ebuild
[add] https://crrev.com/27a7794d68413db81587268baca6485993b20595/media-libs/harfbuzz/files/harfbuzz-1.2.3-fixedversion2int.patch
[rename] https://crrev.com/27a7794d68413db81587268baca6485993b20595/media-libs/harfbuzz/harfbuzz-1.2.3-r2.ebuild

Comment 20 by drott@chromium.org, Apr 18 2016

Status: Fixed (was: Assigned)
Re #18:
> Please merge your change to M51 branch 2704 ASAP (before 5:00 PM PST, today) so we can take it in for M51 last Dev release tomorrow.

This is already in 2704:
https://chromium.googlesource.com/chromium/src.git/+/51.0.2704.18/third_party/harfbuzz-ng/README.chromium
Labels: -Merge-Approved-51
Ok, thank you for confirmation. Removing "Merge-Approved-51" label. bhthompson@, please feel free to add this label back if needed for CrOS. Thank you.

Comment 22 by js...@chromium.org, Apr 20 2016

govind@: You must have missed "(NOT necessary for other platforms)." in my comment 14. :-)

CrOS R51 was also taken care of (comment 19). CrOS R50 has older HarfBuzz 1.0.x and does not have this issue (HarfBuzz 1.2.x was landed after 50 branch). So, it's all set for CrOS, too. 




Project Member

Comment 23 by bugdroid1@chromium.org, Apr 27 2016

Labels: merge-merged-release-R50-7978.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/5fa80ba5786717931374ac536dc2ff37fa0ece11

commit 5fa80ba5786717931374ac536dc2ff37fa0ece11
Author: Jungshik Shin <jshin@chromium.org>
Date: Sat Apr 09 00:08:32 2016

Cherry pick an upstream patch (commit: 6dd80faf0)

This cherry-pick is recommended by the upstream maintainer
behdad@behdad.org. This is to make a merging to R50 branch
easier and is the same cherry-pick was done for Chromium.

For the trunk, this will be followed by upgrading to 1.2.[56].

BUG= chromium:601739 ,  chromium:569997 
TEST=emerge-${BOARD} harfbuzz
CQ-DEPEND=CL:340547

Change-Id: If900942f0b461ddb15f84deb004438b81548a3f4
Previous-Reviewed-on: https://chromium-review.googlesource.com/338004
(cherry picked from commit 1a49a8be21fa861b857c1be7f8f2e7783d758ace)
Reviewed-on: https://chromium-review.googlesource.com/340664
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Tested-by: Jungshik Shin <jshin@chromium.org>
Commit-Queue: Jungshik Shin <jshin@chromium.org>

[modify] https://crrev.com/5fa80ba5786717931374ac536dc2ff37fa0ece11/media-libs/harfbuzz/harfbuzz-1.2.3.ebuild
[add] https://crrev.com/5fa80ba5786717931374ac536dc2ff37fa0ece11/media-libs/harfbuzz/files/harfbuzz-1.2.3-fixedversion2int.patch
[rename] https://crrev.com/5fa80ba5786717931374ac536dc2ff37fa0ece11/media-libs/harfbuzz/harfbuzz-1.2.3-r2.ebuild

Sign in to add a comment