Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2540023002:
Fix browser version string in RTL mode. (Closed)
|
Created:
4 years ago by dpapad Modified:
4 years ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, Dan Beam, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix browser version string in RTL mode.
BUG=669256
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/a2a00b738abb8301a46a6bb92c150d1fe1155e2c
Cr-Commit-Position: refs/heads/master@{#436156}
Patch Set 1 #Patch Set 2 : Fix alignment #Patch Set 3 : Nit. #Patch Set 4 : Revert about_page.html changes, not needed anymore. #
Total comments: 4
Messages
Total messages: 22 (9 generated)
Description was changed from ========== MD Settings: Fix browser version string in RTL mode. BUG=669256 ========== to ========== MD Settings: Fix browser version string in RTL mode. BUG=669256 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Fix browser version string in RTL mode. BUG=669256 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix browser version string in RTL mode. BUG=669256 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dpapad@chromium.org changed reviewers: + dschuyler@chromium.org
Before/after screenshots at http://imgur.com/a/tgZnA.
Ah, <span> works nicely there! LGTM
On 2016/11/30 at 00:33:07, dschuyler wrote: > Ah, <span> works nicely there! > LGTM @dbeam pointed out that chrome://version does something more sophisticated (breaks the version string into components), which results in a perhaps saner (but more involved) way to display the string in RTL. We can either 1) Hold off until I'll hear back from some people familiar on RTL 2) Land this CL as an improvement (not obviously broken, matches old Options), and do 1 separately.
On 2016/11/30 00:53:31, dpapad wrote: > On 2016/11/30 at 00:33:07, dschuyler wrote: > > Ah, <span> works nicely there! > > LGTM > > @dbeam pointed out that chrome://version does something more sophisticated > (breaks the version string into components), which results in a perhaps saner > (but more involved) way to display the string in RTL. > > We can either > 1) Hold off until I'll hear back from some people familiar on RTL > 2) Land this CL as an improvement (not obviously broken, matches old Options), > and do 1 separately. Maybe write a crbug for option 1 so it is tracked?
On 2016/11/30 at 01:04:53, dschuyler wrote: > On 2016/11/30 00:53:31, dpapad wrote: > > On 2016/11/30 at 00:33:07, dschuyler wrote: > > > Ah, <span> works nicely there! > > > LGTM > > > > @dbeam pointed out that chrome://version does something more sophisticated > > (breaks the version string into components), which results in a perhaps saner > > (but more involved) way to display the string in RTL. > > > > We can either > > 1) Hold off until I'll hear back from some people familiar on RTL > > 2) Land this CL as an improvement (not obviously broken, matches old Options), > > and do 1 separately. > > Maybe write a crbug for option 1 so it is tracked? @dbeam: Filed feature request bug at https://bugs.chromium.org/p/chromium/issues/detail?id=669760, to keep track. Let me know if following approach 2 in previous comment seems reasonable.
On 2016/11/30 at 02:45:02, dpapad wrote: > On 2016/11/30 at 01:04:53, dschuyler wrote: > > On 2016/11/30 00:53:31, dpapad wrote: > > > On 2016/11/30 at 00:33:07, dschuyler wrote: > > > > Ah, <span> works nicely there! > > > > LGTM > > > > > > @dbeam pointed out that chrome://version does something more sophisticated > > > (breaks the version string into components), which results in a perhaps saner > > > (but more involved) way to display the string in RTL. > > > > > > We can either > > > 1) Hold off until I'll hear back from some people familiar on RTL > > > 2) Land this CL as an improvement (not obviously broken, matches old Options), > > > and do 1 separately. > > > > Maybe write a crbug for option 1 so it is tracked? > > @dbeam: Filed feature request bug at https://bugs.chromium.org/p/chromium/issues/detail?id=669760, to keep track. Let me know if following approach 2 in previous comment seems reasonable. Friendly ping @dbeam^
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
some folks inside google told us that about:version was better in their eyes.
they gave us alternate translations, but about:version is pretty close and it
looks like a matter of opinion
can we slap 4 translations into 4 <span>s or whatever like about:version?
<span>$i18n{versionNumber}</span>
(<span>$i18n{versionOfficial}</span>)
<span>$i18n{versionModifier}</span
<span>$i18n{versionBits}</span>
https://cs.chromium.org/chromium/src/components/version_ui/resources/about_ve...
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/version_ui.cc?sq...
Patchset #3 (id:40001) has been deleted
On 2016/12/02 at 01:48:41, dbeam wrote:
> some folks inside google told us that about:version was better in their eyes.
they gave us alternate translations, but about:version is pretty close and it
looks like a matter of opinion
>
> can we slap 4 translations into 4 <span>s or whatever like about:version?
>
> <span>$i18n{versionNumber}</span>
> (<span>$i18n{versionOfficial}</span>)
> <span>$i18n{versionModifier}</span
> <span>$i18n{versionBits}</span>
>
>
https://cs.chromium.org/chromium/src/components/version_ui/resources/about_ve...
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/version_ui.cc?sq...
Done, except that I am building the string in C++ and passing it as a single
loadTimeData entry. Screenshot at http://imgur.com/a/3ZG4n. Note that in RTL the
word version appears 2nd, which I believe will be fixed automatically once the
new IDS_ string gets localized.
lgtm https://codereview.chromium.org/2540023002/diff/80001/chrome/browser/resource... File chrome/browser/resources/help/help_content.html (right): https://codereview.chromium.org/2540023002/diff/80001/chrome/browser/resource... chrome/browser/resources/help/help_content.html:19: <span i18n-content="browserVersion" dir="ltr"></span> ^ still needed? https://codereview.chromium.org/2540023002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (left): https://codereview.chromium.org/2540023002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:408: version += " (64-bit)"; ahhhh, this wasn't using IDS_VERSION_UI_{32,64}BIT
https://codereview.chromium.org/2540023002/diff/80001/chrome/browser/resource... File chrome/browser/resources/help/help_content.html (right): https://codereview.chromium.org/2540023002/diff/80001/chrome/browser/resource... chrome/browser/resources/help/help_content.html:19: <span i18n-content="browserVersion" dir="ltr"></span> On 2016/12/03 at 01:26:07, Dan Beam wrote: > ^ still needed? Yes, that is the old page (chrome://help), in which I am just fixing the text alignment, but not changing the string itself. https://codereview.chromium.org/2540023002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (left): https://codereview.chromium.org/2540023002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:408: version += " (64-bit)"; On 2016/12/03 at 01:26:07, Dan Beam wrote: > ahhhh, this wasn't using IDS_VERSION_UI_{32,64}BIT Acknowledged.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2540023002/#ps80001 (title: "Revert about_page.html changes, not needed anymore.")
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": 80001, "attempt_start_ts": 1480732080893050,
"parent_rev": "4f7fb6bdeb4d004048fe66d1ca4e6f4fc0a74129", "commit_rev":
"efca2869707ac86543f83070d6459db2fe947dba"}
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix browser version string in RTL mode. BUG=669256 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix browser version string in RTL mode. BUG=669256 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/a2a00b738abb8301a46a6bb92c150d1fe1155e2c Cr-Commit-Position: refs/heads/master@{#436156} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a2a00b738abb8301a46a6bb92c150d1fe1155e2c Cr-Commit-Position: refs/heads/master@{#436156} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
