Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2318333003:
[ARC] Add "(beta)" string to Play Store. (Closed)
|
Created:
4 years, 3 months ago by xdai1 Modified:
4 years, 3 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, elijahtaylor+arcwatch_chromium.org, michaelpg+watch-options_chromium.org, yusukes+watch_chromium.org, tfarina, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, arv+watch_chromium.org, Matt Giuca Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ARC] Add "(beta)" string to Play Store.
- Add (beta) at the end of header of chrome://settings section that says "Google Play Store"
- Add (beta) to Play Store icon in launcher
Note: This change is only for M53 and will be reverted on Tot after being merged to M53.
BUG=644576
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/e10991699b852408f64d9e5195b07b45ebebb202
Cr-Commit-Position: refs/heads/master@{#417637}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address xiyuan@'s comments. #
Total comments: 2
Patch Set 3 : Add a TODO. #
Total comments: 4
Patch Set 4 : Address dbeam@'s comments. #
Messages
Total messages: 17 (7 generated)
Description was changed from ========== [ARC] Add "(beta)" string to Play Store. - Add (beta) at the end of header of chrome://settings section that says "Google Play Store" - Add (beta) to Play Store icon in launcher Note: This change is only for M53 and will be reverted on Tot after being merged to M53. BUG=644576 ========== to ========== [ARC] Add "(beta)" string to Play Store. - Add (beta) at the end of header of chrome://settings section that says "Google Play Store" - Add (beta) to Play Store icon in launcher Note: This change is only for M53 and will be reverted on Tot after being merged to M53. BUG=644576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
xdai@chromium.org changed reviewers: + dbeam@chromium.org, xiyuan@chromium.org
xiyuan@, dbeam@, could you help review this CL please? Thanks! For screenshots about what it looks like, see https://drive.google.com/corp/drive/u/0/folders/0B4R9nvLdzmC9Zl9zRUR2LTRxYlk.
The CQ bit was checked by xdai@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...
https://codereview.chromium.org/2318333003/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2318333003/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:704: name_beta = name + " (" + Don't like the concatenation but don't have a better way. :( Also, use |name_beta| in other places looks strange, maybe rename it to something like |updated_name| or |actual_name|. https://codereview.chromium.org/2318333003/diff/1/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2318333003/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/browser_options_handler.cc:444: { "androidAppsTitleBeta", IDS_ABOUT_PAGE_CURRENT_CHANNEL_BETA}, I think we should append "(beta)" to IDS_OPTIONS_ARC_TITLE instead of creating two spans and concat them. Cancating does not work for all languages.
xiyuan@, I've addressed your comments. Please take another look, thanks! https://codereview.chromium.org/2318333003/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2318333003/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:704: name_beta = name + " (" + On 2016/09/07 22:56:36, xiyuan wrote: > Don't like the concatenation but don't have a better way. :( > > Also, use |name_beta| in other places looks strange, maybe rename it to > something like |updated_name| or |actual_name|. Renamed it. https://codereview.chromium.org/2318333003/diff/1/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2318333003/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/browser_options_handler.cc:444: { "androidAppsTitleBeta", IDS_ABOUT_PAGE_CURRENT_CHANNEL_BETA}, On 2016/09/07 22:56:36, xiyuan wrote: > I think we should append "(beta)" to IDS_OPTIONS_ARC_TITLE instead of creating > two spans and concat them. Cancating does not work for all languages. Done.
lgtm https://codereview.chromium.org/2318333003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2318333003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:574: std::string androidAppsTitle = nit: Put a TODO to remove this after merging it to M53.
dbeam@, kindly ping? xiyuan@, I've addressed your comment. Thanks for the review! https://codereview.chromium.org/2318333003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2318333003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:574: std::string androidAppsTitle = On 2016/09/08 18:01:29, xiyuan wrote: > nit: Put a TODO to remove this after merging it to M53. Done. Thanks for reviewing!
lgtm if this is a one-time thing, I suppose https://codereview.chromium.org/2318333003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2318333003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:575: std::string androidAppsTitle = android_apps_title https://codereview.chromium.org/2318333003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:577: l10n_util::GetStringUTF8(IDS_ABOUT_PAGE_CURRENT_CHANNEL_BETA) + ")"; why isn't this using <ph> placeholders in a .grd file?
dbeam@, I've addressed your comments. Thanks for your review! https://codereview.chromium.org/2318333003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2318333003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:575: std::string androidAppsTitle = On 2016/09/09 00:59:09, Dan Beam wrote: > android_apps_title Done. https://codereview.chromium.org/2318333003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:577: l10n_util::GetStringUTF8(IDS_ABOUT_PAGE_CURRENT_CHANNEL_BETA) + ")"; On 2016/09/09 00:59:09, Dan Beam wrote: > why isn't this using <ph> placeholders in a .grd file? I think I'm not supposed to modify the .grd file any more since it's a urgent one-time merge for M53?
The CQ bit was checked by xdai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2318333003/#ps60001 (title: "Address dbeam@'s comments.")
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:60001)
Message was sent while issue was closed.
Description was changed from ========== [ARC] Add "(beta)" string to Play Store. - Add (beta) at the end of header of chrome://settings section that says "Google Play Store" - Add (beta) to Play Store icon in launcher Note: This change is only for M53 and will be reverted on Tot after being merged to M53. BUG=644576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [ARC] Add "(beta)" string to Play Store. - Add (beta) at the end of header of chrome://settings section that says "Google Play Store" - Add (beta) to Play Store icon in launcher Note: This change is only for M53 and will be reverted on Tot after being merged to M53. BUG=644576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/e10991699b852408f64d9e5195b07b45ebebb202 Cr-Commit-Position: refs/heads/master@{#417637} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e10991699b852408f64d9e5195b07b45ebebb202 Cr-Commit-Position: refs/heads/master@{#417637} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
