Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1611463002:
Remove the close button in context menu for IME menu. (Closed)
|
Created:
4 years, 11 months ago by Azure Wei Modified:
4 years, 10 months ago CC:
Shu Chen, chromium-reviews, kalyank, sadrul, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove the close button in context menu for IME menu.
BUG=570761
TEST=Verified the close button is removed on local build.
Committed: https://crrev.com/47108caae01b627d069a9a8f4f2fe7dec35fa5dd
Cr-Commit-Position: refs/heads/master@{#372880}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed Steven and Devlin's comments. #Patch Set 3 : #
Total comments: 11
Patch Set 4 : Remove the constants from the header. #Patch Set 5 : Move the helper funciton below the constants. #
Total comments: 8
Patch Set 6 : Addressed Steven's comments. #
Total comments: 2
Patch Set 7 : Sync up to date. #
Messages
Total messages: 29 (8 generated)
Description was changed from ========== Remove the close button in context menu for IME menu. BUG=570761 TEST=Verified the close button is removed on local build. ========== to ========== Remove the close button in context menu for IME menu. BUG=570761 TEST=Verified the close button is removed on local build. ==========
azurewei@chromium.org changed reviewers: + rdevlin.cronin@chromium.org, stevenjb@chromium.org
Hi Steven/Devlin, can you please help to review this cl? Thank you!
https://codereview.chromium.org/1611463002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1611463002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:155: #if defined(OS_CHROMEOS) Is there any reason why this needs to be OS_CHROMEOS only? https://codereview.chromium.org/1611463002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:161: } This logic really shouldn't be here. We should create a helper method, e.g. extension_misc::isIMEExtension(id) https://codereview.chromium.org/1611463002/diff/1/chrome/common/extensions/ex... File chrome/common/extensions/extension_constants.cc (right): https://codereview.chromium.org/1611463002/diff/1/chrome/common/extensions/ex... chrome/common/extensions/extension_constants.cc:84: const char* const kIMEExtensionIds[2] = {"gjaehgfemfahhmlgpdfknkhdnemmolop", Elim '2'
(in addition to Steven's) https://codereview.chromium.org/1611463002/diff/1/chrome/common/extensions/ex... File chrome/common/extensions/extension_constants.h (right): https://codereview.chromium.org/1611463002/diff/1/chrome/common/extensions/ex... chrome/common/extensions/extension_constants.h:224: // The extension id of the IME extensions. can we be slightly more specific? IME extensions aren't only limited to these in chrome.
https://codereview.chromium.org/1611463002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1611463002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:155: #if defined(OS_CHROMEOS) On 2016/01/20 18:03:29, stevenjb wrote: > Is there any reason why this needs to be OS_CHROMEOS only? The IME menu extensions are for Chrome OS only, so I put the definition of kImeMenuExtensionIds[] under #if defined(OS_CHROMEOS) section. https://codereview.chromium.org/1611463002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:161: } On 2016/01/20 18:03:29, stevenjb wrote: > This logic really shouldn't be here. We should create a helper method, e.g. > extension_misc::isIMEExtension(id) Done. https://codereview.chromium.org/1611463002/diff/1/chrome/common/extensions/ex... File chrome/common/extensions/extension_constants.cc (right): https://codereview.chromium.org/1611463002/diff/1/chrome/common/extensions/ex... chrome/common/extensions/extension_constants.cc:84: const char* const kIMEExtensionIds[2] = {"gjaehgfemfahhmlgpdfknkhdnemmolop", On 2016/01/20 18:03:29, stevenjb wrote: > Elim '2' Done. https://codereview.chromium.org/1611463002/diff/1/chrome/common/extensions/ex... File chrome/common/extensions/extension_constants.h (right): https://codereview.chromium.org/1611463002/diff/1/chrome/common/extensions/ex... chrome/common/extensions/extension_constants.h:224: // The extension id of the IME extensions. On 2016/01/20 23:52:18, Devlin wrote: > can we be slightly more specific? IME extensions aren't only limited to these > in chrome. I've updated the document, hope it describes better.
Kindly pinging... stevenjb@, Devlin, can you please review this cl? Thank you!
Sorry, I missed the last update to the CL. https://codereview.chromium.org/1611463002/diff/40001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1611463002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:162: l10n_util::GetStringUTF16(IDS_LAUNCHER_CONTEXT_MENU_CLOSE)); nit: Instead of duplicating this, I would suggest: bool show_close_button = true; #if CHROMEOS if (...IsIme...) show_close_button = false; #endif if (show_close_button) AddItem(...) https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... File chrome/common/extensions/extension_constants.cc (right): https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... chrome/common/extensions/extension_constants.cc:86: bool IsImeMenuExtensionId(const std::string extension_id) { string& https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... File chrome/common/extensions/extension_constants.h (right): https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... chrome/common/extensions/extension_constants.h:225: extern const char* const kImeMenuExtensionIds[2]; No need to expose the constants in the header if the helper function is defined here. https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... chrome/common/extensions/extension_constants.h:228: bool IsImeMenuExtensionId(const std::string extension_id); Move this after constant definitions (and maybe to a different file - devlin, WDYT?)
https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... File chrome/common/extensions/extension_constants.h (right): https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... chrome/common/extensions/extension_constants.h:228: bool IsImeMenuExtensionId(const std::string extension_id); On 2016/01/28 18:45:04, stevenjb wrote: > Move this after constant definitions (and maybe to a different file - devlin, > WDYT?) I'd just get rid of this function since it's only used in once place. If it's used in more places in the future, yeah, it should go in some IME-related file most likely.
https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... File chrome/common/extensions/extension_constants.h (right): https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... chrome/common/extensions/extension_constants.h:228: bool IsImeMenuExtensionId(const std::string extension_id); On 2016/01/28 18:56:59, Devlin wrote: > On 2016/01/28 18:45:04, stevenjb wrote: > > Move this after constant definitions (and maybe to a different file - devlin, > > WDYT?) > > I'd just get rid of this function since it's only used in once place. If it's > used in more places in the future, yeah, it should go in some IME-related file > most likely. Heh, so, I actually recommended that we not embed this logic in the place where it is getting used (launcher_context_menu.cc, which, imho, should know as little as possible about the details of what defines an ImeMenuExtension). I could be convinced to go ahead and embed it there if you don't want it here.
https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... File chrome/common/extensions/extension_constants.h (right): https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... chrome/common/extensions/extension_constants.h:228: bool IsImeMenuExtensionId(const std::string extension_id); On 2016/01/28 19:28:42, stevenjb wrote: > On 2016/01/28 18:56:59, Devlin wrote: > > On 2016/01/28 18:45:04, stevenjb wrote: > > > Move this after constant definitions (and maybe to a different file - > devlin, > > > WDYT?) > > > > I'd just get rid of this function since it's only used in once place. If it's > > used in more places in the future, yeah, it should go in some IME-related file > > most likely. > > Heh, so, I actually recommended that we not embed this logic in the place where > it is getting used (launcher_context_menu.cc, which, imho, should know as little > as possible about the details of what defines an ImeMenuExtension). > > I could be convinced to go ahead and embed it there if you don't want it here. haha oh. I agree that the ids shouldn't necessarily go there, but the function here is just messing with my sense of symmetry. If you don't even want the "if (id == ImeIds[0] || id == ImeIds[1])" in there, then I don't feel really strongly, and it can stay here (underneath the other constant declarations, as you said).
https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... File chrome/common/extensions/extension_constants.h (right): https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... chrome/common/extensions/extension_constants.h:228: bool IsImeMenuExtensionId(const std::string extension_id); On 2016/01/28 19:39:00, Devlin wrote: > On 2016/01/28 19:28:42, stevenjb wrote: > > On 2016/01/28 18:56:59, Devlin wrote: > > > On 2016/01/28 18:45:04, stevenjb wrote: > > > > Move this after constant definitions (and maybe to a different file - > > devlin, > > > > WDYT?) > > > > > > I'd just get rid of this function since it's only used in once place. If > it's > > > used in more places in the future, yeah, it should go in some IME-related > file > > > most likely. > > > > Heh, so, I actually recommended that we not embed this logic in the place > where > > it is getting used (launcher_context_menu.cc, which, imho, should know as > little > > as possible about the details of what defines an ImeMenuExtension). > > > > I could be convinced to go ahead and embed it there if you don't want it here. > > haha oh. I agree that the ids shouldn't necessarily go there, but the function > here is just messing with my sense of symmetry. > > If you don't even want the "if (id == ImeIds[0] || id == ImeIds[1])" in there, > then I don't feel really strongly, and it can stay here (underneath the other > constant declarations, as you said). OK, let's go with that then (leaving the helper function in this C++ file, removing the new constant from the header, and moving the function declaration and implementation below the constants).
https://codereview.chromium.org/1611463002/diff/40001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1611463002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:162: l10n_util::GetStringUTF16(IDS_LAUNCHER_CONTEXT_MENU_CLOSE)); On 2016/01/28 18:45:04, stevenjb wrote: > nit: Instead of duplicating this, I would suggest: > bool show_close_button = true; > #if CHROMEOS > if (...IsIme...) show_close_button = false; > #endif > if (show_close_button) > AddItem(...) Done. https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... File chrome/common/extensions/extension_constants.cc (right): https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... chrome/common/extensions/extension_constants.cc:86: bool IsImeMenuExtensionId(const std::string extension_id) { On 2016/01/28 18:45:04, stevenjb wrote: > string& Done. https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... File chrome/common/extensions/extension_constants.h (right): https://codereview.chromium.org/1611463002/diff/40001/chrome/common/extension... chrome/common/extensions/extension_constants.h:228: bool IsImeMenuExtensionId(const std::string extension_id); On 2016/01/28 19:46:17, stevenjb wrote: > On 2016/01/28 19:39:00, Devlin wrote: > > On 2016/01/28 19:28:42, stevenjb wrote: > > > On 2016/01/28 18:56:59, Devlin wrote: > > > > On 2016/01/28 18:45:04, stevenjb wrote: > > > > > Move this after constant definitions (and maybe to a different file - > > > devlin, > > > > > WDYT?) > > > > > > > > I'd just get rid of this function since it's only used in once place. If > > it's > > > > used in more places in the future, yeah, it should go in some IME-related > > file > > > > most likely. > > > > > > Heh, so, I actually recommended that we not embed this logic in the place > > where > > > it is getting used (launcher_context_menu.cc, which, imho, should know as > > little > > > as possible about the details of what defines an ImeMenuExtension). > > > > > > I could be convinced to go ahead and embed it there if you don't want it > here. > > > > haha oh. I agree that the ids shouldn't necessarily go there, but the > function > > here is just messing with my sense of symmetry. > > > > If you don't even want the "if (id == ImeIds[0] || id == ImeIds[1])" in there, > > then I don't feel really strongly, and it can stay here (underneath the other > > constant declarations, as you said). > > OK, let's go with that then (leaving the helper function in this C++ file, > removing the new constant from the header, and moving the function declaration > and implementation below the constants). Done. Thanks for all your comments.
https://codereview.chromium.org/1611463002/diff/80001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1611463002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:154: bool show_close_button = true; nit (sorry, I didn't notice this before) this could be: bool show_close_button = controller_->IsOpen(item_.id); That way we could eliminate one level of indention here. https://codereview.chromium.org/1611463002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:158: show_close_button = false; nit: {} https://codereview.chromium.org/1611463002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:162: l10n_util::GetStringUTF16(IDS_LAUNCHER_CONTEXT_MENU_CLOSE)); {} https://codereview.chromium.org/1611463002/diff/80001/chrome/common/extension... File chrome/common/extensions/extension_constants.cc (right): https://codereview.chromium.org/1611463002/diff/80001/chrome/common/extension... chrome/common/extensions/extension_constants.cc:93: } Sorry if I wasn't clear, but this should also be placed below all constants, i.e. at the bottom of the file.
https://codereview.chromium.org/1611463002/diff/80001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1611463002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:154: bool show_close_button = true; On 2016/01/29 17:53:27, stevenjb wrote: > nit (sorry, I didn't notice this before) this could be: > bool show_close_button = controller_->IsOpen(item_.id); > > That way we could eliminate one level of indention here. Done. Thank you for pointing this out. https://codereview.chromium.org/1611463002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:158: show_close_button = false; On 2016/01/29 17:53:27, stevenjb wrote: > nit: {} Done. https://codereview.chromium.org/1611463002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:162: l10n_util::GetStringUTF16(IDS_LAUNCHER_CONTEXT_MENU_CLOSE)); On 2016/01/29 17:53:27, stevenjb wrote: > {} Done. https://codereview.chromium.org/1611463002/diff/80001/chrome/common/extension... File chrome/common/extensions/extension_constants.cc (right): https://codereview.chromium.org/1611463002/diff/80001/chrome/common/extension... chrome/common/extensions/extension_constants.cc:93: } On 2016/01/29 17:53:27, stevenjb wrote: > Sorry if I wasn't clear, but this should also be placed below all constants, > i.e. at the bottom of the file. Done.
lgtm
lgtm with nit https://codereview.chromium.org/1611463002/diff/100001/chrome/common/extensio... File chrome/common/extensions/extension_constants.cc (right): https://codereview.chromium.org/1611463002/diff/100001/chrome/common/extensio... chrome/common/extensions/extension_constants.cc:132: "gjaehgfemfahhmlgpdfknkhdnemmolop", "jkghodnilhceideoidjikpgommlajknk"}; is this git cl formatted? (The bracket style looks a little weird to me.) If so, fine, if not, please do so. :)
https://codereview.chromium.org/1611463002/diff/100001/chrome/common/extensio... File chrome/common/extensions/extension_constants.cc (right): https://codereview.chromium.org/1611463002/diff/100001/chrome/common/extensio... chrome/common/extensions/extension_constants.cc:132: "gjaehgfemfahhmlgpdfknkhdnemmolop", "jkghodnilhceideoidjikpgommlajknk"}; On 2016/02/01 18:24:40, Devlin wrote: > is this git cl formatted? (The bracket style looks a little weird to me.) If > so, fine, if not, please do so. :) Yes. This line is generated by 'git cl format'.
The CQ bit was checked by azurewei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1611463002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1611463002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/1611463002/#ps120001 (title: "Sync up to date.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1611463002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1611463002/120001
Message was sent while issue was closed.
Description was changed from ========== Remove the close button in context menu for IME menu. BUG=570761 TEST=Verified the close button is removed on local build. ========== to ========== Remove the close button in context menu for IME menu. BUG=570761 TEST=Verified the close button is removed on local build. ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Remove the close button in context menu for IME menu. BUG=570761 TEST=Verified the close button is removed on local build. ========== to ========== Remove the close button in context menu for IME menu. BUG=570761 TEST=Verified the close button is removed on local build. Committed: https://crrev.com/47108caae01b627d069a9a8f4f2fe7dec35fa5dd Cr-Commit-Position: refs/heads/master@{#372880} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/47108caae01b627d069a9a8f4f2fe7dec35fa5dd Cr-Commit-Position: refs/heads/master@{#372880} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
