Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2112653003:
grit: Automatically replace ... with … (Closed)
|
Created:
4 years, 5 months ago by agrieve Modified:
4 years, 5 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, extensions-reviews_chromium.org, jam, kalyank, oshima+watch_chromium.org, sadrul, sdefresne+watch_chromium.org, Alexei Svitkine (slow), Daniel Erat, jochen (gone - plz use gerrit), jungshik at Google, msw, oshima, sdefresne, sky Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiongrit: Automatically replace ... with … (U+2026)
Android lint tool pointed out that ... should really be … (U+2026).
Tested that this glyph renders well on Mac + Win as well, so
applying to all OS's by default.
TBR=flackr
BUG=621772
Committed: https://crrev.com/e82c4e1b9de3b189c85bccff63d4ef3046e44d43
Cr-Commit-Position: refs/heads/master@{#407182}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Convert remaining ellipsis #
Total comments: 2
Patch Set 3 : remove accidental .cc change #Patch Set 4 : switch to hex escapes. #Patch Set 5 : Missed some files when converting to hex #Patch Set 6 : Fix unittest and presubmit warnings about PRODUCT_NAME #Patch Set 7 : grit-based-ellipsis #
Total comments: 4
Patch Set 8 : comments1 #Patch Set 9 : fix test #Patch Set 10 : Move replace to Translate() to avoid changing message IDs #
Messages
Total messages: 74 (35 generated)
agrieve@chromium.org changed reviewers: + sky@chromium.org
sky - please advise whether you think this is a worthwhile change.
sky@chromium.org changed reviewers: + derat@chromium.org
I think this makes sense. My only concern is how well the fonts we use honor it. Have you verified how this impacts chrome on the various platforms? +derat as he knows about fonts on chromeos (and likely linux too).
One more thought. Might using said glyph trigger additional loading we aren't doing now? On Thu, Jun 30, 2016 at 9:27 AM, <sky@chromium.org> wrote: > I think this makes sense. My only concern is how well the fonts we use honor > it. > Have you verified how this impacts chrome on the various platforms? > +derat as he knows about fonts on chromeos (and likely linux too). > > https://codereview.chromium.org/2112653003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/30 17:02:32, sky wrote: > One more thought. Might using said glyph trigger additional loading we > aren't doing now? > > On Thu, Jun 30, 2016 at 9:27 AM, <mailto:sky@chromium.org> wrote: > > I think this makes sense. My only concern is how well the fonts we use honor > > it. > > Have you verified how this impacts chrome on the various platforms? > > +derat as he knows about fonts on chromeos (and likely linux too). > > > > https://codereview.chromium.org/2112653003/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > Definitely good points, and certainly possible that it could trigger a fallback font. I've done 0 testing on this, and at least wanted to hear if there were any alarm bells at the thought of it before I invested more into it. I'll give this a go on Android and Linux (where I can easily test it) and see if things look right. If there's a "good" time to land this time-wise, I'd also appreciate the advice there. E.g. not sure what our translation schedule is like.
sky@chromium.org changed reviewers: + asvitkine@chromium.org, msw@chromium.org
+msw and +asvitkine for my eyes As to time to launch, I believe a branch is happening soon, so probably after that would be fine.
I wonder why we have so many ellipses in our UI strings; they don't seem to add any value for the most part. (why bother saying "Find..." or "Print...", when we just say "Copy", "Downloads", etc.). It's odd that some places add ellipses to 'Settings', but some don't. So long as this actually renders correctly, and you coordinate with the localization folks, I just have one minor comment; otherwise lgtm. https://codereview.chromium.org/2112653003/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2112653003/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:529: + <message name="IDS_CONTENT_CONTEXT_PRINT" desc="The name of the Print… command in the content area context menu. Brings a dialog to select the print options"> nit: I'm not sure it makes sense to replace the ellipses in the description comment strings. If anything it makes this less clear to the average reader.
I think ellipses are meant to say "a dialog will open with more options", so "Copy" shouldn't have it but "Print..." opens a dialog. For this change, I think it would be worth checking what the rendering looks like on different OSes for ... vs. the single-glyph … - I could imagine it could affect spacing and sizing of the .'s on some platforms and it may not necessarily look great. On Thu, Jun 30, 2016 at 2:25 PM, <msw@chromium.org> wrote: > I wonder why we have so many ellipses in our UI strings; they don't seem > to add > any value for the most part. (why bother saying "Find..." or "Print...", > when we > just say "Copy", "Downloads", etc.). It's odd that some places add > ellipses to > 'Settings', but some don't. So long as this actually renders correctly, > and you > coordinate with the localization folks, I just have one minor comment; > otherwise > lgtm. > > > > https://codereview.chromium.org/2112653003/diff/1/chrome/app/generated_resour... > File chrome/app/generated_resources.grd (right): > > > https://codereview.chromium.org/2112653003/diff/1/chrome/app/generated_resour... > chrome/app/generated_resources.grd:529: + <message > name="IDS_CONTENT_CONTEXT_PRINT" desc="The name of the Print… > command in the content area context menu. Brings a dialog to select the > print options"> > nit: I'm not sure it makes sense to replace the ellipses in the > description comment strings. If anything it makes this less clear to the > average reader. > > https://codereview.chromium.org/2112653003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
derat@chromium.org changed reviewers: + jshin@chromium.org
jungshik (added, but apparently ooo) will probably be able to comment on how well-supported 'HORIZONTAL ELLIPSIS' (U+2026) is, but i'd be surprised if any commonly-used UI fonts lack it. http://www.fileformat.info/info/unicode/char/2026/fontsupport.htm says that it's present in most of the usual suspects
https://codereview.chromium.org/2112653003/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2112653003/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:529: + <message name="IDS_CONTENT_CONTEXT_PRINT" desc="The name of the Print… command in the content area context menu. Brings a dialog to select the print options"> On 2016/06/30 18:25:37, msw wrote: > nit: I'm not sure it makes sense to replace the ellipses in the description > comment strings. If anything it makes this less clear to the average reader. Done.
agrieve@chromium.org changed reviewers: + jochen@chromium.org, oshima@chromium.org, sdefresne@chromium.org
jochen@chromium.org: Please review changes in content/, components/ sdefresne@chromium.org: Please review changes in ios/ oshima@chromium.org: Please review changes in ui/chromeos/
On 2016/07/04 15:58:35, agrieve wrote: > mailto:jochen@chromium.org: Please review changes in content/, components/ > > mailto:sdefresne@chromium.org: Please review changes in ios/ > > mailto:oshima@chromium.org: Please review changes in ui/chromeos/ about:history and menus look good on Android & Linux. Attached a screenshot to the bug of what the ...s look like on linux before/after.
ios/ lgtm
I asked ainsle@ on the referenced bug to chime in
ui/chromeos lgtm
On 2016/06/30 19:34:36, Daniel Erat wrote: > jungshik (added, but apparently ooo) will probably be able to comment on how > well-supported 'HORIZONTAL ELLIPSIS' (U+2026) is, but i'd be surprised if any > commonly-used UI fonts lack it. > http://www.fileformat.info/info/unicode/char/2026/fontsupport.htm says that it's > present in most of the usual suspects Thank you, Daniel for checking it out. U+2026 should be fine. BTW, it'd be nice for the CL description to have "U+2026" mentioned (in addition to the actual character). Replace ... with … (U+2026; Horizontal Ellipsis) in grd files LGTM
I suspect ... is going to creep back into these files without a check. https://codereview.chromium.org/2112653003/diff/20001/components/policy/core/... File components/policy/core/common/schema_registry.cc (right): https://codereview.chromium.org/2112653003/diff/20001/components/policy/core/... components/policy/core/common/schema_registry.cc:58: //NOTREACHED(); Why are you commenting this out?
A check is a good idea. I'll work on one in a follow-up. https://codereview.chromium.org/2112653003/diff/20001/components/policy/core/... File components/policy/core/common/schema_registry.cc (right): https://codereview.chromium.org/2112653003/diff/20001/components/policy/core/... components/policy/core/common/schema_registry.cc:58: //NOTREACHED(); On 2016/07/08 21:05:26, sky (OOO until 7-18) wrote: > Why are you commenting this out? Yikes! Reverted. Chrome wouldn't start on my machine with this here :(
Description was changed from ========== Replace ... with … in grd files. This was found by android lint tool, and I think it makes sense. The fix won't make it into builds until .xtb files are updated though. BUG=621772 ========== to ========== Replace ... with … (U+2026; Horizontal Ellipsis) in grd files This was found by android lint tool, and I think it makes sense. The fix won't make it into builds until .xtb files are updated though. BUG=621772 ==========
Description was changed from ========== Replace ... with … (U+2026; Horizontal Ellipsis) in grd files This was found by android lint tool, and I think it makes sense. The fix won't make it into builds until .xtb files are updated though. BUG=621772 ========== to ========== Replace ... with … (U+2026; Horizontal Ellipsis) in grd files This was found by android lint tool, and I think it makes sense. The fix won't make it into builds until .xtb files are updated though. TBR=sky BUG=621772 ==========
On 2016/07/11 14:43:28, agrieve wrote: > A check is a good idea. I'll work on one in a follow-up. > > https://codereview.chromium.org/2112653003/diff/20001/components/policy/core/... > File components/policy/core/common/schema_registry.cc (right): > > https://codereview.chromium.org/2112653003/diff/20001/components/policy/core/... > components/policy/core/common/schema_registry.cc:58: //NOTREACHED(); > On 2016/07/08 21:05:26, sky (OOO until 7-18) wrote: > > Why are you commenting this out? > > Yikes! Reverted. Chrome wouldn't start on my machine with this here :( tbr for sky as it looks like he's out for a week now and I think I've good pretty much an lgtm here.
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, oshima@chromium.org, jshin@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2112653003/#ps80001 (title: "Missed some files when converting to hex")
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 jochen@chromium.org
sorry for cancelling the commit, but can we please first wait for the UI team to come to a conclusion? Alex' feedback on the bug didn't sound like this was necessarily the way to go
On 2016/07/11 14:57:51, jochen wrote: > sorry for cancelling the commit, but can we please first wait for the UI team to > come to a conclusion? > > Alex' feedback on the bug didn't sound like this was necessarily the way to go Hey - no problem at all! Definitely happy to wait if there's anyone still not sure.
The CQ bit was checked by agrieve@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...
Okay, Mac/Windows screenshots look good. Thanks! Why is there a different ... for Cast... on Linux? Are you missing a change in this CL? It would be unfortunate to make things look inconsistent on Linux. Finally, have a higher level comment. What are the actual practical benefits from this? As far as I can tell: Benefits: 1. Silences some Android build warning. (Could we also silence it by disabling it?) Downsides: 1. Code is less readable - people have to look up what … to understand the strings if they don't have that magic number memorized. 2. Risk of inconsistency - people might still add new entries with 3-character ...'s and now things are inconsistent. On Linux, this will result in actual inconsistency in the UI since the two are rendered differently. (This could be mitigated with a presubmit or something that guards against new entries.) Is that right? Is there another benefit I'm missing? If the only benefit is silencing some Android warnings, I'd rather that we simply disabled those warnings. Unless those warnings are highlighting some more serious issue, but if so I'm missing context on what it is.
On 2016/07/12 15:07:42, Alexei Svitkine wrote: > Okay, Mac/Windows screenshots look good. Thanks! > > Why is there a different ... for Cast... on Linux? Are you missing a change in > this CL? It would be unfortunate to make things look inconsistent on Linux. I included that to show the different rendering. I've update the Cast... one in this CL as well. > > Finally, have a higher level comment. What are the actual practical benefits > from this? > > As far as I can tell: > > Benefits: > 1. Silences some Android build warning. (Could we also silence it by disabling > it?) Many believe it also looks nicer. I assume that's why there's an Android lint check for it in the first place. > > Downsides: > 1. Code is less readable - people have to look up what … to understand > the strings if they don't have that magic number memorized. We could just put the … right in the .grd files. It's already done here: https://cs.chromium.org/chromium/src/chrome/android/java/strings/android_chro... wdyt? > 2. Risk of inconsistency - people might still add new entries with 3-character > ...'s and now things are inconsistent. On Linux, this will result in actual > inconsistency in the UI since the two are rendered differently. (This could be > mitigated with a presubmit or something that guards against new entries.) I do intend on adding a presubmit. The risk of inconsistency right now is actually quite high as most Android-only strings already use ... > > Is that right? Is there another benefit I'm missing? > > If the only benefit is silencing some Android warnings, I'd rather that we > simply disabled those warnings. Unless those warnings are highlighting some more > serious issue, but if so I'm missing context on what it is.
On 2016/07/12 15:24:56, agrieve wrote: > On 2016/07/12 15:07:42, Alexei Svitkine wrote: > > Okay, Mac/Windows screenshots look good. Thanks! > > > > Why is there a different ... for Cast... on Linux? Are you missing a change in > > this CL? It would be unfortunate to make things look inconsistent on Linux. > > I included that to show the different rendering. I've update the Cast... one in > this CL as well. > > > > > Finally, have a higher level comment. What are the actual practical benefits > > from this? > > > > As far as I can tell: > > > > Benefits: > > 1. Silences some Android build warning. (Could we also silence it by > disabling > > it?) > > Many believe it also looks nicer. I assume that's why there's an Android lint > check for it in the first place. I see - so there's also a rendering difference on Android I guess? Might be worth putting up screenshots of that as well. > > > > > Downsides: > > 1. Code is less readable - people have to look up what … to > understand > > the strings if they don't have that magic number memorized. > > We could just put the … right in the .grd files. It's already done here: > https://cs.chromium.org/chromium/src/chrome/android/java/strings/android_chro... > > wdyt? I actually would prefer that, it certainly addresses my readability concern. > > > 2. Risk of inconsistency - people might still add new entries with > 3-character > > ...'s and now things are inconsistent. On Linux, this will result in actual > > inconsistency in the UI since the two are rendered differently. (This could be > > mitigated with a presubmit or something that guards against new entries.) > > I do intend on adding a presubmit. The risk of inconsistency right now is > actually quite high as most Android-only strings already use ... That sounds good and addresses my concern. By the way, another approach to consider would be to do the normalization in the GRT itself, when it processes the grd files. It might actually be less work than the presubmit and would just work and doesn't rely on authors remembering which ... to use. (Although, one can argue it's nice to have consistency in the source files themselves…) > > > > > > Is that right? Is there another benefit I'm missing? > > > > If the only benefit is silencing some Android warnings, I'd rather that we > > simply disabled those warnings. Unless those warnings are highlighting some > more > > serious issue, but if so I'm missing context on what it is.
On 2016/07/12 15:31:14, Alexei Svitkine wrote: > On 2016/07/12 15:24:56, agrieve wrote: > > On 2016/07/12 15:07:42, Alexei Svitkine wrote: > > > Okay, Mac/Windows screenshots look good. Thanks! > > > > > > Why is there a different ... for Cast... on Linux? Are you missing a change > in > > > this CL? It would be unfortunate to make things look inconsistent on Linux. > > > > I included that to show the different rendering. I've update the Cast... one > in > > this CL as well. > > > > > > > > Finally, have a higher level comment. What are the actual practical benefits > > > from this? > > > > > > As far as I can tell: > > > > > > Benefits: > > > 1. Silences some Android build warning. (Could we also silence it by > > disabling > > > it?) > > > > Many believe it also looks nicer. I assume that's why there's an Android lint > > check for it in the first place. > > I see - so there's also a rendering difference on Android I guess? Might be > worth putting up screenshots of that as well. > > > > > > > > > Downsides: > > > 1. Code is less readable - people have to look up what … to > > understand > > > the strings if they don't have that magic number memorized. > > > > We could just put the … right in the .grd files. It's already done here: > > > https://cs.chromium.org/chromium/src/chrome/android/java/strings/android_chro... > > > > wdyt? > > I actually would prefer that, it certainly addresses my readability concern. > > > > > > 2. Risk of inconsistency - people might still add new entries with > > 3-character > > > ...'s and now things are inconsistent. On Linux, this will result in actual > > > inconsistency in the UI since the two are rendered differently. (This could > be > > > mitigated with a presubmit or something that guards against new entries.) > > > > I do intend on adding a presubmit. The risk of inconsistency right now is > > actually quite high as most Android-only strings already use ... > > That sounds good and addresses my concern. > > By the way, another approach to consider would be to do the normalization in the > GRT itself, when it processes the grd files. It might actually be less work than > the presubmit and would just work and doesn't rely on authors remembering which > ... to use. (Although, one can argue it's nice to have consistency in the source > files themselves…) This sounds quite appealing to me and I'm a little bit sad I didn't think of it :P. I'll look into changing it there. > > > > > > > > > > > Is that right? Is there another benefit I'm missing? > > > > > > If the only benefit is silencing some Android warnings, I'd rather that we > > > simply disabled those warnings. Unless those warnings are highlighting some > > more > > > serious issue, but if so I'm missing context on what it is.
Description was changed from ========== Replace ... with … (U+2026; Horizontal Ellipsis) in grd files This was found by android lint tool, and I think it makes sense. The fix won't make it into builds until .xtb files are updated though. TBR=sky BUG=621772 ========== to ========== Replace ... with … (U+2026; Horizontal Ellipsis) in grd files This was found by android lint tool, and I think it makes sense. The fix won't make it into builds until .xtb files are updated though. BUG=621772 ==========
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
+thakis for grit changes
Description was changed from ========== Replace ... with … (U+2026; Horizontal Ellipsis) in grd files This was found by android lint tool, and I think it makes sense. The fix won't make it into builds until .xtb files are updated though. BUG=621772 ========== to ========== grit: Automatically replace ... with … (U+2026) Android lint tool pointed out that ... should really be … (U+2026). Tested that this glyph renders well on Mac + Win as well, so applying to all OS's by default. BUG=621772 ==========
agrieve@chromium.org changed reviewers: + thakis@chromium.org
+thakis for real this time.
agrieve@chromium.org changed reviewers: + flackr@chromium.org - thakis@chromium.org
On 2016/07/14 20:03:34, agrieve wrote: > +thakis for real this time. Looks like thakis has gone OOO for a while. Re-assigning to flackr.
https://codereview.chromium.org/2112653003/diff/120001/tools/grit/grit/node/m... File tools/grit/grit/node/message.py (right): https://codereview.chromium.org/2112653003/diff/120001/tools/grit/grit/node/m... tools/grit/grit/node/message.py:21: _ELLIPSIS_PATTERN = lazy_re.compile(r'(?<!\.)\.\.\.(?=$|\s)') Can you add a simple description comment of what '...' patterns this is matching? (i.e. exactly three dots ending a line or followed by a space) https://codereview.chromium.org/2112653003/diff/120001/tools/grit/grit/node/m... File tools/grit/grit/node/message_unittest.py (right): https://codereview.chromium.org/2112653003/diff/120001/tools/grit/grit/node/m... tools/grit/grit/node/message_unittest.py:15: # Prevent our io module conflicting with the system io module. I don't understand what you mean by this, why is it necessary to put the first path at the end?
https://codereview.chromium.org/2112653003/diff/120001/tools/grit/grit/node/m... File tools/grit/grit/node/message.py (right): https://codereview.chromium.org/2112653003/diff/120001/tools/grit/grit/node/m... tools/grit/grit/node/message.py:21: _ELLIPSIS_PATTERN = lazy_re.compile(r'(?<!\.)\.\.\.(?=$|\s)') On 2016/07/20 18:23:37, flackr wrote: > Can you add a simple description comment of what '...' patterns this is > matching? (i.e. exactly three dots ending a line or followed by a space) Done. https://codereview.chromium.org/2112653003/diff/120001/tools/grit/grit/node/m... File tools/grit/grit/node/message_unittest.py (right): https://codereview.chromium.org/2112653003/diff/120001/tools/grit/grit/node/m... tools/grit/grit/node/message_unittest.py:15: # Prevent our io module conflicting with the system io module. On 2016/07/20 18:23:37, flackr wrote: > I don't understand what you mean by this, why is it necessary to put the first > path at the end? Elaborated the comment and made the code a bit more clever.
lgtm
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, jshin@chromium.org, msw@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2112653003/#ps140001 (title: "comments1")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by agrieve@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by agrieve@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.
On 2016/07/21 15:01:02, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) +asanka for downloads & moved all others to CC. flackr: Had to change when the replace happens to be closer to when it outputs, as changing the clique parts caused message IDs to change
Description was changed from ========== grit: Automatically replace ... with … (U+2026) Android lint tool pointed out that ... should really be … (U+2026). Tested that this glyph renders well on Mac + Win as well, so applying to all OS's by default. BUG=621772 ========== to ========== grit: Automatically replace ... with … (U+2026) Android lint tool pointed out that ... should really be … (U+2026). Tested that this glyph renders well on Mac + Win as well, so applying to all OS's by default. TBR=flackr BUG=621772 ==========
On 2016/07/21 18:31:39, agrieve wrote: > On 2016/07/21 15:01:02, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > +asanka for downloads & moved all others to CC. > flackr: Had to change when the replace happens to be closer to when it outputs, > as changing the clique parts caused message IDs to change Looks like both downloads reviewers are OOO. TBR'ing since it's a trivial change.
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, jshin@chromium.org, flackr@chromium.org, msw@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2112653003/#ps180001 (title: "Move replace to Translate() to avoid changing message IDs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== grit: Automatically replace ... with … (U+2026) Android lint tool pointed out that ... should really be … (U+2026). Tested that this glyph renders well on Mac + Win as well, so applying to all OS's by default. TBR=flackr BUG=621772 ========== to ========== grit: Automatically replace ... with … (U+2026) Android lint tool pointed out that ... should really be … (U+2026). Tested that this glyph renders well on Mac + Win as well, so applying to all OS's by default. TBR=flackr BUG=621772 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== grit: Automatically replace ... with … (U+2026) Android lint tool pointed out that ... should really be … (U+2026). Tested that this glyph renders well on Mac + Win as well, so applying to all OS's by default. TBR=flackr BUG=621772 ========== to ========== grit: Automatically replace ... with … (U+2026) Android lint tool pointed out that ... should really be … (U+2026). Tested that this glyph renders well on Mac + Win as well, so applying to all OS's by default. TBR=flackr BUG=621772 Committed: https://crrev.com/e82c4e1b9de3b189c85bccff63d4ef3046e44d43 Cr-Commit-Position: refs/heads/master@{#407182} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/e82c4e1b9de3b189c85bccff63d4ef3046e44d43 Cr-Commit-Position: refs/heads/master@{#407182}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/2176663003/ by mpearson@chromium.org. The reason for reverting is: Likely cause of failures: --- https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/6... unexpected_failures: fast/forms/color/color-suggestion-picker-appearance-zoom125.html fast/forms/color/color-suggestion-picker-with-scrollbar-appearance.html fast/forms/color/color-suggestion-picker-appearance-zoom200.html all with the message image diff --- since it appears to replace "..." with a proper ellipsis, which all the three tests show differences from: https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux/678... also failures for also some other bots for tests fast/forms/color/color-suggestion-picker-appearance.html fast/forms/color/color-suggestion-picker-one-row-appearance.html fast/forms/color/color-suggestion-picker-two-row-appearance.html appear on other bots. And, for what it's worth, the bot https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.11%20%28... started showing a renderer crash in http/tests/images/restyle-decode-error.html in a blamelist that includes this change as well. Hopefully this revert will fix that too. :-) . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
