Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2935723002:
Fix ManualTextFramerTest.OriginRTLTest on iOS 11 (Closed)
|
Created:
3 years, 6 months ago by lpromero Modified:
3 years, 6 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, vmpstr+watch_chromium.org, noyau+watch_chromium.org, marq+watch_chromium.org, danakj+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix ManualTextFramerTest.OriginRTLTest on iOS 11
The test uses an arabic text. On iOS 11, Apple updated the Arabic system
font. The new font takes more horizontal space for the same text and
font size. This CL updates the target text frame on iOS 11 and above to
accomodate that.
https://screenshot.googleplex.com/vF2x9SLEyrx
BUG=731156
R=kkhorimoto@chromium.org
Review-Url: https://codereview.chromium.org/2935723002
Cr-Commit-Position: refs/heads/master@{#479641}
Committed: https://chromium.googlesource.com/chromium/src/+/ec010b6a08e247661e07d9eb883508a74bd60468
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add comment #
Total comments: 3
Patch Set 3 : Just bump the size #Messages
Total messages: 37 (23 generated)
The CQ bit was checked by lpromero@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lpromero@chromium.org changed reviewers: + sdefresne@chromium.org
sdefresne@chromium.org: Please review changes in
Description was changed from ========== Fix ManualTextFramerTest.OriginRTLTest on iOS 11 The test uses an arabic text. On iOS 11, Apple updated the Arabic system font. The new font takes more horizontal space for the same text and font size. This CL updates the target text frame on iOS 11 and above to accomodate that. BUG=731156 R=kkhorimoto@chromium.org ========== to ========== Fix ManualTextFramerTest.OriginRTLTest on iOS 11 The test uses an arabic text. On iOS 11, Apple updated the Arabic system font. The new font takes more horizontal space for the same text and font size. This CL updates the target text frame on iOS 11 and above to accomodate that. https://screenshot.googleplex.com/vF2x9SLEyrx BUG=731156 R=kkhorimoto@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, with some added commenting. The width calculations are a little opaque; sorry I should have added commenting to it to begin with. https://codereview.chromium.org/2935723002/diff/1/ios/chrome/browser/ui/util/... File ios/chrome/browser/ui/util/manual_text_framer_unittest.mm (right): https://codereview.chromium.org/2935723002/diff/1/ios/chrome/browser/ui/util/... ios/chrome/browser/ui/util/manual_text_framer_unittest.mm:202: CGRect bounds = base::ios::IsRunningOnIOS11OrLater() Can we separate the width calculation out and add some commenting? Something like: // The bounds width is chosen so that the RTL string above // is laid out into 3 lines. The font that iOS 11 uses for Arabic // strings takes more horizontal space, so the bounds width // is increased accordingly. CGFloat width = base::ios::IsRunningOnIOS11OrLater() ? 130.0 : 100.0;
The CQ bit was checked by lpromero@chromium.org to run a CQ dry run
On 2017/06/12 19:42:04, kkhorimoto_ wrote: > lgtm, with some added commenting. The width calculations are a little opaque; > sorry I should have added commenting to it to begin with. Makes total sense now that we have this runtime check on top of it. Thanks!
https://codereview.chromium.org/2935723002/diff/1/ios/chrome/browser/ui/util/... File ios/chrome/browser/ui/util/manual_text_framer_unittest.mm (right): https://codereview.chromium.org/2935723002/diff/1/ios/chrome/browser/ui/util/... ios/chrome/browser/ui/util/manual_text_framer_unittest.mm:202: CGRect bounds = base::ios::IsRunningOnIOS11OrLater() On 2017/06/12 19:42:03, kkhorimoto_ wrote: > Can we separate the width calculation out and add some commenting? Something > like: > > // The bounds width is chosen so that the RTL string above > // is laid out into 3 lines. The font that iOS 11 uses for Arabic > // strings takes more horizontal space, so the bounds width > // is increased accordingly. > CGFloat width = base::ios::IsRunningOnIOS11OrLater() ? 130.0 : 100.0; Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lpromero@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2935723002/#ps20001 (title: "Add comment")
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lpromero@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sdefresne, PTAL base/ios changes.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
lpromero@chromium.org changed reviewers: + rohitrao@chromium.org
+rohitrao in sdefresne's absence.
lgtm https://codereview.chromium.org/2935723002/diff/20001/ios/chrome/browser/ui/u... File ios/chrome/browser/ui/util/manual_text_framer_unittest.mm (right): https://codereview.chromium.org/2935723002/diff/20001/ios/chrome/browser/ui/u... ios/chrome/browser/ui/util/manual_text_framer_unittest.mm:205: CGFloat width = base::ios::IsRunningOnIOS11OrLater() ? 130.0 : 100.0; This test is very dependent on the exact sizes of iOS font, because all of the expected values are hardcoded. Is there any way to write a meaningful test that doesn't hardcode as much? For example, could we determine the width by taking the length of the full string and dividing by something close to 3? How did you come up with 130.0?
https://codereview.chromium.org/2935723002/diff/20001/base/ios/ios_util.h File base/ios/ios_util.h (right): https://codereview.chromium.org/2935723002/diff/20001/base/ios/ios_util.h#new... base/ios/ios_util.h:20: BASE_EXPORT bool IsRunningOnIOS11OrLater(); I'm going to pull this out into a separate CL.
The CQ bit was checked by lpromero@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2935723002/diff/20001/ios/chrome/browser/ui/u... File ios/chrome/browser/ui/util/manual_text_framer_unittest.mm (right): https://codereview.chromium.org/2935723002/diff/20001/ios/chrome/browser/ui/u... ios/chrome/browser/ui/util/manual_text_framer_unittest.mm:205: CGFloat width = base::ios::IsRunningOnIOS11OrLater() ? 130.0 : 100.0; On 2017/06/14 11:58:03, rohitrao (ping after 24h) wrote: > This test is very dependent on the exact sizes of iOS font, because all of the > expected values are hardcoded. Is there any way to write a meaningful test that > doesn't hardcode as much? > > For example, could we determine the width by taking the length of the full > string and dividing by something close to 3? How did you come up with 130.0? Dividing by 3 is not sufficient, I need to come up with some padding anyway. What I did in my latest patch is just unconditionally bump the width (i.e. for all configs). The box can be larger than necessary. The issue with iOS 11 was that with the new font the text got clipped. Now with the new value, it passes everywhere.
The CQ bit was unchecked by lpromero@chromium.org
The CQ bit was checked by lpromero@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2935723002/#ps40001 (title: "Just bump the size")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497515218194500,
"parent_rev": "09ab0857a3fd76bb950622fb270cb618007c3f47", "commit_rev":
"ec010b6a08e247661e07d9eb883508a74bd60468"}
Message was sent while issue was closed.
Description was changed from ========== Fix ManualTextFramerTest.OriginRTLTest on iOS 11 The test uses an arabic text. On iOS 11, Apple updated the Arabic system font. The new font takes more horizontal space for the same text and font size. This CL updates the target text frame on iOS 11 and above to accomodate that. https://screenshot.googleplex.com/vF2x9SLEyrx BUG=731156 R=kkhorimoto@chromium.org ========== to ========== Fix ManualTextFramerTest.OriginRTLTest on iOS 11 The test uses an arabic text. On iOS 11, Apple updated the Arabic system font. The new font takes more horizontal space for the same text and font size. This CL updates the target text frame on iOS 11 and above to accomodate that. https://screenshot.googleplex.com/vF2x9SLEyrx BUG=731156 R=kkhorimoto@chromium.org Review-Url: https://codereview.chromium.org/2935723002 Cr-Commit-Position: refs/heads/master@{#479641} Committed: https://chromium.googlesource.com/chromium/src/+/ec010b6a08e247661e07d9eb8835... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/ec010b6a08e247661e07d9eb8835... |
