Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2841013002:
Permissions/Mac: Fix RTL positions for permission decision strings. (Closed)
DescriptionPermissions/Mac: Fix RTL positions for permission decision strings.
Support RTL for permission decision strings, wrapping strings that may be too
long to fit.
See
https://drive.google.com/file/d/0BzEa5HU1aAqBUEl4THZuSkd5Tms/view?usp=sharing
for before / after screenshots.
BUG=714859
Review-Url: https://codereview.chromium.org/2841013002
Cr-Commit-Position: refs/heads/master@{#468568}
Committed: https://chromium.googlesource.com/chromium/src/+/09eed0675cc8f316344caf5ac695f140608493f6
Patch Set 1 #
Total comments: 2
Patch Set 2 : Wrap instead of ellide. #Patch Set 3 : Wrap instead of ellide. #Messages
Total messages: 27 (21 generated)
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Permissions/Mac: Fix RTL positions for permission decision strings. Support RTL for permission decision strings. BUG=714859 ========== to ========== Permissions/Mac: Fix RTL positions for permission decision strings. Support RTL for permission decision strings, elliding strings that may be too long to fit. See https://drive.google.com/file/d/0BzEa5HU1aAqBRUZIZTBpUElObDQ/view?usp=sharing for before / after screenshots. BUG=714859 ==========
patricialor@chromium.org changed reviewers: + lgarron@chromium.org
Hi lgarron, PTAL? Thanks!
lgarron@chromium.org changed reviewers: + rsesek@chromium.org
LGTM with a consistency question. rsesek@, could you review for platform LGTM? https://codereview.chromium.org/2841013002/diff/1/chrome/browser/ui/cocoa/pag... File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm (right): https://codereview.chromium.org/2841013002/diff/1/chrome/browser/ui/cocoa/pag... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:929: [[label cell] setLineBreakMode:NSLineBreakByTruncatingTail]; How does this work in LTR right now? Should this be shared by both?
The CQ bit was checked by patricialor@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...
Description was changed from ========== Permissions/Mac: Fix RTL positions for permission decision strings. Support RTL for permission decision strings, elliding strings that may be too long to fit. See https://drive.google.com/file/d/0BzEa5HU1aAqBRUZIZTBpUElObDQ/view?usp=sharing for before / after screenshots. BUG=714859 ========== to ========== Permissions/Mac: Fix RTL positions for permission decision strings. Support RTL for permission decision strings, wrapping strings that may be too long to fit. See https://drive.google.com/open?id=0BzEa5HU1aAqBaFlUWkxGUWd1eWs for before / after screenshots. BUG=714859 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Permissions/Mac: Fix RTL positions for permission decision strings. Support RTL for permission decision strings, wrapping strings that may be too long to fit. See https://drive.google.com/open?id=0BzEa5HU1aAqBaFlUWkxGUWd1eWs for before / after screenshots. BUG=714859 ========== to ========== Permissions/Mac: Fix RTL positions for permission decision strings. Support RTL for permission decision strings, wrapping strings that may be too long to fit. See https://drive.google.com/file/d/0BzEa5HU1aAqBUEl4THZuSkd5Tms/view?usp=sharing for before / after screenshots. BUG=714859 ==========
The CQ bit was checked by patricialor@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...
Hi lgarron, PTAL, I've changed the code a bit. Thanks in advance both for the reviews! https://codereview.chromium.org/2841013002/diff/1/chrome/browser/ui/cocoa/pag... File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm (right): https://codereview.chromium.org/2841013002/diff/1/chrome/browser/ui/cocoa/pag... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:929: [[label cell] setLineBreakMode:NSLineBreakByTruncatingTail]; On 2017/04/28 01:26:07, lgarron wrote: > How does this work in LTR right now? Should this be shared by both? Yup, so LTR has custom logic in -addText which will wrap the text and set the correct height instead of elliding it. Originally I went with the latter option because I saw that was what was being done for other RTL text above (the permission label), but on second thoughts it's probably better to wrap the string, since there aren't any other other views that should be on the same y-level. I've updated it to use the -addText's text wrapping and updated the screenshots as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org Link to the patchset: https://codereview.chromium.org/2841013002/#ps40001 (title: "Wrap instead of ellide.")
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": 1493701613738720,
"parent_rev": "50e675c553115af32441ed00149935ff604fa71e", "commit_rev":
"09eed0675cc8f316344caf5ac695f140608493f6"}
Message was sent while issue was closed.
Description was changed from ========== Permissions/Mac: Fix RTL positions for permission decision strings. Support RTL for permission decision strings, wrapping strings that may be too long to fit. See https://drive.google.com/file/d/0BzEa5HU1aAqBUEl4THZuSkd5Tms/view?usp=sharing for before / after screenshots. BUG=714859 ========== to ========== Permissions/Mac: Fix RTL positions for permission decision strings. Support RTL for permission decision strings, wrapping strings that may be too long to fit. See https://drive.google.com/file/d/0BzEa5HU1aAqBUEl4THZuSkd5Tms/view?usp=sharing for before / after screenshots. BUG=714859 Review-Url: https://codereview.chromium.org/2841013002 Cr-Commit-Position: refs/heads/master@{#468568} Committed: https://chromium.googlesource.com/chromium/src/+/09eed0675cc8f316344caf5ac695... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/09eed0675cc8f316344caf5ac695... |
