Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2499463003:
Apps related string update. (Closed)
|
Created:
4 years, 1 month ago by lgcheng Modified:
4 years, 1 month ago Reviewers:
Marc Treib CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionApps related string update.
BUG=664316
Committed: https://crrev.com/c45037d7cdd8a65080e96e5ab144bb9ad40c28f4
Cr-Commit-Position: refs/heads/master@{#432308}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Apps related string update. #
Total comments: 5
Patch Set 3 : String description. #
Total comments: 2
Patch Set 4 : Rebase. #Messages
Total messages: 37 (24 generated)
https://codereview.chromium.org/2499463003/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2499463003/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:3677: + "<ph name="APP_NAME">$1<ex>Gmail Checker</ex></ph>" will be uninstalled. There is a line break here. This works but I am not sure if it's the right way.
treib@chromium.org changed reviewers: + treib@chromium.org
Generally, we prefer to add new strings together with the code that uses them - otherwise you run the risk of somebody cleaning up the unused strings. https://codereview.chromium.org/2499463003/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2499463003/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:3676: + <message name="IDS_ARC_APP_UNINSTALL_PROMPT_HEADING" desc="Bold line in the content area of the arc app uninstallation prompt. Tells the user that local data associated with the app will be removed from this divice."> nit: should "arc" be capitalized? Also, translators probably won't know what ARC is. nit2: "divice" should be "device" https://codereview.chromium.org/2499463003/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:3677: + "<ph name="APP_NAME">$1<ex>Gmail Checker</ex></ph>" will be uninstalled. On 2016/11/11 18:21:46, lgcheng wrote: > There is a line break here. This works but I am not sure if it's the right way. Yup, this should work correctly. It's a different question if this is the right way to go about things in terms of layout etc - often it's better to have separate labels instead of one with a line break. But I'll leave that to you. https://codereview.chromium.org/2499463003/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:4810: + Uninstall There's an IDS_EXTENSION_PROMPT_UNINSTALL_BUTTON which is "Remove" (and a bunch of other similar strings with "remove") - we probably want to stay consistent in our terminology. Any reason for introducing a different term here? If we do keep the new string: The IDs around here all start with "IDS_EXTENSION_PROMPT_" and have an "_APP" suffix if necessary. Please stay consistent with that.
Hi Marc, Thanks for comments! The implementation CL is waiting for a blocking CL https://codereview.chromium.org/2474783002/ to pass so that I can send it for review. It's to meet the M56 branch point(string freeze) to create this separate string CL. I'd be very thankful that you are willing to take a look at the implementation CL and I'll add you as reviewer when I send that for review. Thanks! https://codereview.chromium.org/2499463003/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2499463003/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:3676: + <message name="IDS_ARC_APP_UNINSTALL_PROMPT_HEADING" desc="Bold line in the content area of the arc app uninstallation prompt. Tells the user that local data associated with the app will be removed from this divice."> On 2016/11/14 09:31:20, Marc Treib wrote: > nit: should "arc" be capitalized? Also, translators probably won't know what ARC > is. > nit2: "divice" should be "device" Done. https://codereview.chromium.org/2499463003/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:3677: + "<ph name="APP_NAME">$1<ex>Gmail Checker</ex></ph>" will be uninstalled. On 2016/11/14 09:31:20, Marc Treib wrote: > On 2016/11/11 18:21:46, lgcheng wrote: > > There is a line break here. This works but I am not sure if it's the right > way. > > Yup, this should work correctly. It's a different question if this is the right > way to go about things in terms of layout etc - often it's better to have > separate labels instead of one with a line break. But I'll leave that to you. From design it's more two sentence with line break within same label. As designer is happy with the demo in https://bugs.chromium.org/p/chromium/issues/detail?id=661076&q=owner%3Ame&col... I'll keep this for now. https://codereview.chromium.org/2499463003/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:4810: + Uninstall On 2016/11/14 09:31:20, Marc Treib wrote: > There's an IDS_EXTENSION_PROMPT_UNINSTALL_BUTTON which is "Remove" (and a bunch > of other similar strings with "remove") - we probably want to stay consistent in > our terminology. Any reason for introducing a different term here? > > If we do keep the new string: The IDs around here all start with > "IDS_EXTENSION_PROMPT_" and have an "_APP" suffix if necessary. Please stay > consistent with that. Change ID to IDS_EXTENSION_PROMPT_UNINSTALL_APP_BUTTON. It's by design to change string on the button to "Uninstall" for Non-platform app and ARC app to stay consistent with "Uninstall..." on context menu and Android key words(Remove and Uninstall means different in Android). Platform app(Chrome, youtube etc...) will keep that string as "Remove".
Sorry, I'm confused... these strings are for something other than the existing Chrome Apps? If so, then IMO we need to find a different name, because otherwise it's guaranteed to create confusion later on. https://codereview.chromium.org/2499463003/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2499463003/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:3628: + <!--App force stop--> nitty nit: spaces inside the tags, like <!-- App force stop --> https://codereview.chromium.org/2499463003/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:3670: + <message name="IDS_APP_UNINSTALL_PROMPT_TITLE" desc="Titlebar of the app uninstallation prompt window"> There's an IDS_EXTENSION_UNINSTALL_PROMPT_TITLE which looks like it's the corresponding thing, but it's actually different. Can we find a better name? Should all the new strings have "_ARC_" in their IDs?
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Hi Marc, Thanks for pointing out the potential misleading contents and I add more descriptions for strings. PTAL Thanks! https://codereview.chromium.org/2499463003/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2499463003/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:3628: + <!--App force stop--> On 2016/11/14 18:18:07, Marc Treib wrote: > nitty nit: spaces inside the tags, like > <!-- App force stop --> Done. Add _ARC_ as force stop is for ARC app only. https://codereview.chromium.org/2499463003/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:3670: + <message name="IDS_APP_UNINSTALL_PROMPT_TITLE" desc="Titlebar of the app uninstallation prompt window"> On 2016/11/14 18:18:07, Marc Treib wrote: > There's an IDS_EXTENSION_UNINSTALL_PROMPT_TITLE which looks like it's the > corresponding thing, but it's actually different. Can we find a better name? > Should all the new strings have "_ARC_" in their IDs? This string is not limited to ARC app. Currently, Chrome Apps share strings with extensions. But non-platform app (mainly third party non-extension app e.g "Postman" from Chrome app store) has different strings in context menu ("Uninstall..." for non-platform app VS "Remove from Chrome..." for platform app https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/extension_app... ). The prompt title will still be "Confirm Removal" for platform app, and this string("Uninstall app?") will be shared with ARC apps non-platform Chrome apps. https://codereview.chromium.org/2499463003/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:3673: + <message name="IDS_APP_UNINSTALL_PROMPT_HEADING" desc="First bold line in the content area of the app uninstallation prompt. Tells the user the app will be uninstalled if confirmed."> This string is for non-platform Chrome app. Platform Chrome app will still keep string with extensions.
I'm still somewhat confused about all the different app types and wonder if we could be clearer with the string IDs, but that's outside of the scope of this CL... So, the plan is to land the strings now, and then merge the actual implementation into to M56 branch later? FWIW, that's not how the process is supposed to work, and you'll probably have trouble getting all the launch approvals that late. But in the meantime, this CL LGTM :) https://codereview.chromium.org/2499463003/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2499463003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:3678: +Data associated with this app will be removed from this device. Hm, now that I look at this again: Would it be better to reuse the IDS_NON_PLATFORM_APP_UNINSTALL_PROMPT_HEADING string (renamed to IDS_APP_UNINSTALL_PROMPT_HEADING), and make a IDS_PLATFORM_APP_UNINSTALL_PROMPT_HEADING_EXTRA (or something like that) that's just the second line? But I won't block the CL on this.
On 2016/11/15 09:00:27, Marc Treib wrote: > I'm still somewhat confused about all the different app types and wonder if we > could be clearer with the string IDs, but that's outside of the scope of this > CL... > > So, the plan is to land the strings now, and then merge the actual > implementation into to M56 branch later? FWIW, that's not how the process is > supposed to work, and you'll probably have trouble getting all the launch > approvals that late. > > But in the meantime, this CL LGTM :) > > https://codereview.chromium.org/2499463003/diff/80001/chrome/app/generated_re... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/2499463003/diff/80001/chrome/app/generated_re... > chrome/app/generated_resources.grd:3678: +Data associated with this app will be > removed from this device. > Hm, now that I look at this again: Would it be better to reuse the > IDS_NON_PLATFORM_APP_UNINSTALL_PROMPT_HEADING string (renamed to > IDS_APP_UNINSTALL_PROMPT_HEADING), and make a > IDS_PLATFORM_APP_UNINSTALL_PROMPT_HEADING_EXTRA (or something like that) that's > just the second line? > But I won't block the CL on this. Thanks Marc for review! And also thanks for mentioning later merge approvals. I will focus on finalizing my implementation CL and I shall revisit the string clean up later when I find a better way. The app type is more on ChromeOS launcher, but it does lead to confusing in some situations. I will also check with PMs to finalize the UX.
https://codereview.chromium.org/2499463003/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2499463003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:3678: +Data associated with this app will be removed from this device. On 2016/11/15 09:00:27, Marc Treib wrote: > Hm, now that I look at this again: Would it be better to reuse the > IDS_NON_PLATFORM_APP_UNINSTALL_PROMPT_HEADING string (renamed to > IDS_APP_UNINSTALL_PROMPT_HEADING), and make a > IDS_PLATFORM_APP_UNINSTALL_PROMPT_HEADING_EXTRA (or something like that) that's > just the second line? > But I won't block the CL on this. I have just seen a previous usage for line break like this https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?l=9383. But I think your suggestion is very neat and I will revisit this when I find a better way for line break in string. Thanks for the suggestion!
The CQ bit was checked by lgcheng@google.com
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by lgcheng@google.com 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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by lgcheng@google.com 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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by lgcheng@google.com 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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by lgcheng@google.com 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.
The CQ bit was checked by lgcheng@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2499463003/#ps100001 (title: "Rebase.")
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.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Apps related string update. BUG=664316 ========== to ========== Apps related string update. BUG=664316 Committed: https://crrev.com/c45037d7cdd8a65080e96e5ab144bb9ad40c28f4 Cr-Commit-Position: refs/heads/master@{#432308} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c45037d7cdd8a65080e96e5ab144bb9ad40c28f4 Cr-Commit-Position: refs/heads/master@{#432308} |
