Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1947393004:
add translate::kTranslateUI2016Q2 to chrome://flags page (Closed)
|
Created:
4 years, 7 months ago by ftang Modified:
4 years, 7 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionadd translate::kTranslateUI2016Q2 to chrome://flags page
BUG=607170
Committed: https://crrev.com/ee70896060915a77908913761506faed25f2763b
Cr-Commit-Position: refs/heads/master@{#392219}
Patch Set 1 #
Total comments: 6
Patch Set 2 : undo the format change and change the description #
Messages
Total messages: 19 (7 generated)
ftang@chromium.org changed reviewers: + groby@chromium.org
implemented as you suggested
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1947393004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1947393004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Sorry about the extra work, but not reformatting the entire file is _really_ important. (I'll raise this on chromium-dev, because at some point we should fix this, but for this CL, keep the existing formatting and make sure your new entry follows that formatting) https://codereview.chromium.org/1947393004/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1947393004/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:6381: + Translate 2016Q2 U Please use a slightly more descriptive name :) Maybe "New Translate Bubble UI"? https://codereview.chromium.org/1947393004/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:6384: + Enable the Translate 2016Q2 UI design which show icon inside the bubble and new triggering logic. You don't need to enumerate everything this does. Also, please don't start with 'enable', because we will show the same description for all flag states. How about "Improved triggering logic and look for Translate Bubble UI"? https://codereview.chromium.org/1947393004/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1947393004/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:131: {IDS_GENERIC_EXPERIMENT_CHOICE_ENABLED, switches::kTouchEvents, Please undo the auto-formatting in unrelated entries. (If you do 'git cl format', 'git add -p' is a great tool to pick up only the changes that are relevant) https://codereview.chromium.org/1947393004/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:1152: {"translate-2016q2-ui", IDS_FLAGS_TRANSLATE_2016Q2_UI_NAME, this part LG :)
undo the format change and change the description
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/1947393004/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1947393004/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:6381: + Translate 2016Q2 U On 2016/05/05 22:44:34, groby wrote: > Please use a slightly more descriptive name :) > > Maybe "New Translate Bubble UI"? I am pretty against the use of word "New" to describe something will very soon be old once the project is launched and we start to work on the next new thing in several months (or weeks). I know "Translate 2016Q2 UI" is not super descriptive but it is just a name and at least everyone will know how "new" it means in 2017 or later if the code is not 100% removed yet and they won't got confused w/ some other new thing we will do in 2017 or later. https://codereview.chromium.org/1947393004/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:6384: + Enable the Translate 2016Q2 UI design which show icon inside the bubble and new triggering logic. On 2016/05/05 22:44:34, groby wrote: > You don't need to enumerate everything this does. Also, please don't start with > 'enable', because we will show the same description for all flag states. > > How about "Improved triggering logic and look for Translate Bubble UI"? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1947393004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1947393004/20001
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 groby@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1947393004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1947393004/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== add translate::kTranslateUI2016Q2 to chrome://flags page BUG=607170 ========== to ========== add translate::kTranslateUI2016Q2 to chrome://flags page BUG=607170 Committed: https://crrev.com/ee70896060915a77908913761506faed25f2763b Cr-Commit-Position: refs/heads/master@{#392219} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ee70896060915a77908913761506faed25f2763b Cr-Commit-Position: refs/heads/master@{#392219} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
