Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2489893003:
[omnibox] Give Apple products different capitalization (Closed)
|
Created:
4 years, 1 month ago by Kevin Bailey Modified:
4 years, 1 month ago Reviewers:
Peter Kasting CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[omnibox] Give Apple products different capitalization
Apple products capitalize every word. All the rest only capitalize
the first word. Handle this difference in Omnibox string.
BUG=664141
Committed: https://crrev.com/a2688dd1ce88366c273f1132b806f91edd6fca27
Cr-Commit-Position: refs/heads/master@{#431364}
Patch Set 1 #
Total comments: 1
Patch Set 2 : use_titlecase #Messages
Total messages: 17 (10 generated)
Description was changed from ========== [omnibox] Give Apple products different capitalization Apple products capitalize every word. All the rest only capitalize the first word. Handle this difference in Omnibox string. BUG=664141 ========== to ========== [omnibox] Give Apple products different capitalization Apple products capitalize every word. All the rest only capitalize the first word. Handle this difference in Omnibox string. BUG=664141 ==========
krb@chromium.org changed reviewers: + pkasting@chromium.org
Hi Peter, Is there some way to avoid copying the long 'desc' twice? Perhaps we could move it up to a dummy variable above the 'if'?
LGTM
On 2016/11/10 16:13:00, Kevin Bailey wrote:
> Is there some way to avoid copying the long 'desc' twice?
> Perhaps we could move it up to a dummy variable above the
> 'if'?
What if we nested the <if> inside the <message>?
<message ...>
<if expr="use_titlecase">
<then>
Not Secure
</then>
<else>
Not secure
</else>
</if>
</message>
I would assume this would do the right thing.
https://codereview.chromium.org/2489893003/diff/1/components/omnibox_strings....
File components/omnibox_strings.grdp (right):
https://codereview.chromium.org/2489893003/diff/1/components/omnibox_strings....
components/omnibox_strings.grdp:20: <if expr="is_ios or is_macosx">
I believe this should key off use_titlecase, instead of directly off the
specific platform.
On 2016/11/10 20:54:23, Peter Kasting wrote: > LGTM > > On 2016/11/10 16:13:00, Kevin Bailey wrote: > > Is there some way to avoid copying the long 'desc' twice? > > Perhaps we could move it up to a dummy variable above the > > 'if'? > > What if we nested the <if> inside the <message>? > > <message ...> > <if expr="use_titlecase"> > <then> > Not Secure > </then> > <else> > Not secure > </else> > </if> > </message> > > I would assume this would do the right thing. > > https://codereview.chromium.org/2489893003/diff/1/components/omnibox_strings.... > File components/omnibox_strings.grdp (right): > > https://codereview.chromium.org/2489893003/diff/1/components/omnibox_strings.... > components/omnibox_strings.grdp:20: <if expr="is_ios or is_macosx"> > I believe this should key off use_titlecase, instead of directly off the > specific platform. I had tried that, but it said: grit.exception.UnexpectedChild: An unexpected child element was encountered (on a leaf node).: invalid child <grit.node.misc.IfNode object at 0x7f4b066ca090> for parent message so I assume messages just can't do that. I wonder if there's some way to refer to an earlier object, like: <message name="IDS_NOT_SECURE_VERBOSE_STATE" desc=message["SOME_MESSAGE"].desc>
The CQ bit was checked by krb@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...
On 2016/11/10 21:09:18, Kevin Bailey wrote: > On 2016/11/10 20:54:23, Peter Kasting wrote: > > LGTM > > > > On 2016/11/10 16:13:00, Kevin Bailey wrote: > > > Is there some way to avoid copying the long 'desc' twice? > > > Perhaps we could move it up to a dummy variable above the > > > 'if'? > > > > What if we nested the <if> inside the <message>? > > > > <message ...> > > <if expr="use_titlecase"> > > <then> > > Not Secure > > </then> > > <else> > > Not secure > > </else> > > </if> > > </message> > > > > I would assume this would do the right thing. > > > > > https://codereview.chromium.org/2489893003/diff/1/components/omnibox_strings.... > > File components/omnibox_strings.grdp (right): > > > > > https://codereview.chromium.org/2489893003/diff/1/components/omnibox_strings.... > > components/omnibox_strings.grdp:20: <if expr="is_ios or is_macosx"> > > I believe this should key off use_titlecase, instead of directly off the > > specific platform. > > I had tried that, but it said: > > grit.exception.UnexpectedChild: An unexpected child element was encountered (on > a leaf node).: invalid child <grit.node.misc.IfNode object at 0x7f4b066ca090> > for parent message > > so I assume messages just can't do that. I wonder if there's some way to refer > to > an earlier object, like: > > <message name="IDS_NOT_SECURE_VERBOSE_STATE" desc=message["SOME_MESSAGE"].desc> Hmm. I don't know the right people are, but I would try to find and check with the grit folks to see if there's a way to do this right, and if the behavior you encountered above should be considered to be a bug. I'm OK with duplicating the description, though; more OK than trying to reference an earlier object or something. The downsides of copy-and-paste here seem low, and the complexity and maintenance risk of the other route actually feels higher.
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 krb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2489893003/#ps20001 (title: "use_titlecase")
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 ========== [omnibox] Give Apple products different capitalization Apple products capitalize every word. All the rest only capitalize the first word. Handle this difference in Omnibox string. BUG=664141 ========== to ========== [omnibox] Give Apple products different capitalization Apple products capitalize every word. All the rest only capitalize the first word. Handle this difference in Omnibox string. BUG=664141 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [omnibox] Give Apple products different capitalization Apple products capitalize every word. All the rest only capitalize the first word. Handle this difference in Omnibox string. BUG=664141 ========== to ========== [omnibox] Give Apple products different capitalization Apple products capitalize every word. All the rest only capitalize the first word. Handle this difference in Omnibox string. BUG=664141 Committed: https://crrev.com/a2688dd1ce88366c273f1132b806f91edd6fca27 Cr-Commit-Position: refs/heads/master@{#431364} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a2688dd1ce88366c273f1132b806f91edd6fca27 Cr-Commit-Position: refs/heads/master@{#431364} |
