Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2323823004:
Add action for each button on opt-in IME menu. (Closed)
|
Created:
4 years, 3 months ago by Azure Wei Modified:
4 years, 3 months ago CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nona+watch_chromium.org, oshima+watch_chromium.org, kalyank, James Su, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFor emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions:
1) Overrides the input view url with append parameter: emoji, hwt or voice, which will tell the virtual keyboard to open what kind of keyboard.
2) For Emoji and handwriting input, override the 'id' part like: id=emoji or id=hwt. For voice input, append 'voice' after the 'id' part like: id=us.compact.qwerty.voice
3) Show the virtual keyboard directly if 'On-screen keyboard' is enabled.
4) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once.
5) Add KeyboardControllerObserver::OnKeyboardHidden() to notify ImeMenuTray disable the onscreen keyboard it the tray forces to bring out the keyboard.
6) Add related tests:
InputMethodManagerImplTest.Override*
ImeMenuTrayTest.ShowEmojiKeyset
ImeMenuTrayTest.ForceToShowEmojiKeyset
BUG=642385
TEST=Verified on local build.
Committed: https://crrev.com/649030a2f49f6d868378de840ff08eb180bdffb6
Cr-Commit-Position: refs/heads/master@{#419735}
Patch Set 1 #Patch Set 2 #Patch Set 3 #
Total comments: 16
Patch Set 4 #Patch Set 5 #Patch Set 6 : Show buttons if feature enabled. #Patch Set 7 : Fix test error. #Patch Set 8 #
Total comments: 23
Patch Set 9 : Add KeyboardController::OnKeyboardHidden event. #Patch Set 10 #
Total comments: 22
Patch Set 11 : Addressed nits. #Patch Set 12 : Fix patch error. #Patch Set 13 : Update overriden ref. #Patch Set 14 : Overrides back with the keyboard id when closing input view. #Patch Set 15 #
Total comments: 4
Patch Set 16 : Addressed comments. #
Total comments: 4
Patch Set 17 #Messages
Total messages: 79 (50 generated)
Description was changed from ========== Ime menu button. BUG=642385 TEST=Verified on local build. ========== to ========== For emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions: 1) Overrides the input view url with append parameter: '&keyboard=xxx', which will tell the virtual keyboard to open what kind of keyboard. 2) Show the virtual keyboard directly if 'On-screen keyboard' is enabled. 3) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once. BUG=642385 TEST=Verified on local build. ==========
azurewei@chromium.org changed reviewers: + jamescook@chromium.org, shuchen@chromium.org
The CQ bit was checked by azurewei@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by azurewei@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please review this cl. Thanks!
https://codereview.chromium.org/2323823004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2323823004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1217: void InputMethodManagerImpl::ShowInputViewWithUrl( Is there any reason why this interface/implement is in IMF instead of ui/keyboard? If the logic below is only about ui/keyboard and nothing to do with IMF, I think it should be within ui/keyboard.
I only looked at //ash, since it sounds like the other code might need to live elsewhere. https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:93: SetBorder( nit: DCHECK(ime_menu_tray_) helps the reader understand that it cannot be null (without adding a comment!) https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:122: if (settings_button_ && sender == settings_button_) { nit: |sender| should never be null, so you don't need the |settings_button_ &&| part. (Also below.) https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:128: std::string appended_url; nit: This isn't a URL. It should be called something like |keyboard_type|. https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:129: if (emoji_button_ && sender == emoji_button_) { nit: {} not needed for single-line if() https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:136: if (appended_url.empty()) nit: This should never happen. I would do: else NOTREACHED(); https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:139: chromeos::input_method::InputMethodManager::Get()->ShowInputViewWithUrl( I think it would be better for this function to take a keyboard type string (or better yet, keyboard type enum). This level of the code should not need to know about URL formats. (For example, this assumes the URL has other query parameters on it and it's OK for the string to start with "&keyboard".) https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:180: // TODO(azurewei): Creates the proper button with icons. How did you test this without creating buttons? Could you add placeholder buttons (with intentionally incorrect art)?
The CQ bit was checked by azurewei@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== For emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions: 1) Overrides the input view url with append parameter: '&keyboard=xxx', which will tell the virtual keyboard to open what kind of keyboard. 2) Show the virtual keyboard directly if 'On-screen keyboard' is enabled. 3) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once. BUG=642385 TEST=Verified on local build. ========== to ========== For emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions: 1) Overrides the input view url with append parameter: '#id=emoji/handwriting/voice', which will tell the virtual keyboard to open what kind of keyboard. 2) Show the virtual keyboard directly if 'On-screen keyboard' is enabled. 3) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once. BUG=642385 TEST=Verified on local build. ==========
https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:93: SetBorder( On 2016/09/12 03:21:54, James Cook wrote: > nit: DCHECK(ime_menu_tray_) helps the reader understand that it cannot be null > (without adding a comment!) Done. https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:122: if (settings_button_ && sender == settings_button_) { On 2016/09/12 03:21:54, James Cook wrote: > nit: |sender| should never be null, so you don't need the |settings_button_ &&| > part. (Also below.) Done. Thanks. https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:128: std::string appended_url; On 2016/09/12 03:21:54, James Cook wrote: > nit: This isn't a URL. It should be called something like |keyboard_type|. Done. Renamed as keyset. https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:129: if (emoji_button_ && sender == emoji_button_) { On 2016/09/12 03:21:54, James Cook wrote: > nit: {} not needed for single-line if() Done. https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:136: if (appended_url.empty()) On 2016/09/12 03:21:54, James Cook wrote: > nit: This should never happen. I would do: > > else > NOTREACHED(); Done. https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:139: chromeos::input_method::InputMethodManager::Get()->ShowInputViewWithUrl( On 2016/09/12 03:21:54, James Cook wrote: > I think it would be better for this function to take a keyboard type string (or > better yet, keyboard type enum). This level of the code should not need to know > about URL formats. (For example, this assumes the URL has other query parameters > on it and it's OK for the string to start with "&keyboard".) Done. https://codereview.chromium.org/2323823004/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:180: // TODO(azurewei): Creates the proper button with icons. On 2016/09/12 03:21:54, James Cook wrote: > How did you test this without creating buttons? Could you add placeholder > buttons (with intentionally incorrect art)? Added codes to show the buttons if features::kEHVInputOnImeMenu is enabled. https://codereview.chromium.org/2323823004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2323823004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1217: void InputMethodManagerImpl::ShowInputViewWithUrl( On 2016/09/11 08:33:25, Shu Chen wrote: > Is there any reason why this interface/implement is in IMF instead of > ui/keyboard? > > If the logic below is only about ui/keyboard and nothing to do with IMF, I think > it should be within ui/keyboard. If 'on-screen keyboard' hasn't been enabled, we need to call: ash::Shell::GetInstance()->CreateKeyboard(); to forcibly show the keyboard. This can't be called uner ui/keyboard.
https://codereview.chromium.org/2323823004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2323823004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:36: const int button_right_border = 1; nit: kButtonRightBorder and comment https://codereview.chromium.org/2323823004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:126: if (settings_button_ && sender == settings_button_) { you don't need "settings_button_ &&" https://codereview.chromium.org/2323823004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:275: if (chromeos::input_method::InputMethodManager::Get() && optional: "using chromeos::input_method::InputMethodManager;" at the top of the file will make things wrap better. https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1221: std::string overriden_url = keyboard::GetOverrideContentUrl().spec(); nit: overriden -> overridden https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1223: auto i = overriden_url.find("#id"); Parsing URLs as strings is discouraged. Is there a way to do this with GURL or other URL utilities? https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1226: if (i == std::string::npos) { nit: no need for {} for single-line if https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1258: keyboard::SetAccessibilityKeyboardEnabled(false); Does it matter if the accessibilitykeyboard was enabled before this function was called? https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1259: } I don't know the virtual keyboard code very well, but this function seems odd. There's a bunch of repeated enable/disable going on. I think this needs better comments explaining what you are doing and why. This also needs a test, especially given that you are manipulating URLs and keyboard state. https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1262: return base::FeatureList::IsEnabled(features::kEHVInputOnImeMenu); FYI, I think the comment on kENVInputOnImeMenu in chrome_features.cc is wrong https://cs.chromium.org/chromium/src/chrome/common/chrome_features.cc?q=kEHVI... https://codereview.chromium.org/2323823004/diff/140001/ui/base/ime/chromeos/i... File ui/base/ime/chromeos/input_method_manager.h (right): https://codereview.chromium.org/2323823004/diff/140001/ui/base/ime/chromeos/i... ui/base/ime/chromeos/input_method_manager.h:307: // Shows the virtual keyboard with the specific keyset. nit: Give an example of what "keyset" can be, like (e.g. "emoji"). https://codereview.chromium.org/2323823004/diff/140001/ui/base/ime/chromeos/i... ui/base/ime/chromeos/input_method_manager.h:311: virtual bool IsEHVInputOnImeMenuEnabled() = 0; This should be called something else. I don't know what EHV means, and neither will anyone reading the calling code. IsEmojiHandwritingVoiceOnImeMenuEnabled? AreExtraInputsOnImeMenuEnabled and document what all the extra inputs are? separate functions for each part?
The CQ bit was checked by azurewei@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by azurewei@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== For emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions: 1) Overrides the input view url with append parameter: '#id=emoji/handwriting/voice', which will tell the virtual keyboard to open what kind of keyboard. 2) Show the virtual keyboard directly if 'On-screen keyboard' is enabled. 3) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once. BUG=642385 TEST=Verified on local build. ========== to ========== For emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions: 1) Overrides the input view url with append parameter: '#id=emoji/handwriting/voice', which will tell the virtual keyboard to open what kind of keyboard. 2) Show the virtual keyboard directly if 'On-screen keyboard' is enabled. 3) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once. 4) Add related tests: InputMethodManagerImplTest.OverrideKeyboardRefWithEmoji ImeMenuTrayTest.ShowEmojiKeyset ImeMenuTrayTest.ForceToShowEmojiKeyse BUG=642385 TEST=Verified on local build. ==========
Description was changed from ========== For emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions: 1) Overrides the input view url with append parameter: '#id=emoji/handwriting/voice', which will tell the virtual keyboard to open what kind of keyboard. 2) Show the virtual keyboard directly if 'On-screen keyboard' is enabled. 3) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once. 4) Add related tests: InputMethodManagerImplTest.OverrideKeyboardRefWithEmoji ImeMenuTrayTest.ShowEmojiKeyset ImeMenuTrayTest.ForceToShowEmojiKeyse BUG=642385 TEST=Verified on local build. ========== to ========== For emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions: 1) Overrides the input view url with append parameter: '#id=emoji/handwriting/voice', which will tell the virtual keyboard to open what kind of keyboard. 2) Show the virtual keyboard directly if 'On-screen keyboard' is enabled. 3) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once. 4) Add KeyboardControllerObserver::OnKeyboardHidden() to notify ImeMenuTray disable the onscreen keyboard it the tray forces to bring out the keyboard. 5) Add related tests: InputMethodManagerImplTest.OverrideKeyboardRefWithEmoji ImeMenuTrayTest.ShowEmojiKeyset ImeMenuTrayTest.ForceToShowEmojiKeyse BUG=642385 TEST=Verified on local build. ==========
https://codereview.chromium.org/2323823004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2323823004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:36: const int button_right_border = 1; On 2016/09/12 17:05:56, James Cook wrote: > nit: kButtonRightBorder and comment Done. https://codereview.chromium.org/2323823004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:126: if (settings_button_ && sender == settings_button_) { On 2016/09/12 17:05:56, James Cook wrote: > you don't need "settings_button_ &&" Done. https://codereview.chromium.org/2323823004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:275: if (chromeos::input_method::InputMethodManager::Get() && On 2016/09/12 17:05:56, James Cook wrote: > optional: "using chromeos::input_method::InputMethodManager;" at the top of the > file will make things wrap better. Done. https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1221: std::string overriden_url = keyboard::GetOverrideContentUrl().spec(); On 2016/09/12 17:05:56, James Cook wrote: > nit: overriden -> overridden Done. https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1223: auto i = overriden_url.find("#id"); On 2016/09/12 17:05:57, James Cook wrote: > Parsing URLs as strings is discouraged. Is there a way to do this with GURL or > other URL utilities? Updated with using ReplaceComponents(), it this better? https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1226: if (i == std::string::npos) { On 2016/09/12 17:05:56, James Cook wrote: > nit: no need for {} for single-line if Done. https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1258: keyboard::SetAccessibilityKeyboardEnabled(false); On 2016/09/12 17:05:57, James Cook wrote: > Does it matter if the accessibilitykeyboard was enabled before this function was > called? Updated this logic. https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1259: } On 2016/09/12 17:05:56, James Cook wrote: > I don't know the virtual keyboard code very well, but this function seems odd. > There's a bunch of repeated enable/disable going on. I think this needs better > comments explaining what you are doing and why. > > This also needs a test, especially given that you are manipulating URLs and > keyboard state. Updated. Also added tests: InputMethodManagerImplTest.OverrideKeyboardRefWithEmoji ImeMenuTrayTest.ShowEmojiKeyset ImeMenuTrayTest.ForceToShowEmojiKeyset https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1262: return base::FeatureList::IsEnabled(features::kEHVInputOnImeMenu); On 2016/09/12 17:05:56, James Cook wrote: > FYI, I think the comment on kENVInputOnImeMenu in chrome_features.cc is wrong > > https://cs.chromium.org/chromium/src/chrome/common/chrome_features.cc?q=kEHVI... Updated. https://codereview.chromium.org/2323823004/diff/140001/ui/base/ime/chromeos/i... File ui/base/ime/chromeos/input_method_manager.h (right): https://codereview.chromium.org/2323823004/diff/140001/ui/base/ime/chromeos/i... ui/base/ime/chromeos/input_method_manager.h:307: // Shows the virtual keyboard with the specific keyset. On 2016/09/12 17:05:57, James Cook wrote: > nit: Give an example of what "keyset" can be, like (e.g. "emoji"). Done. https://codereview.chromium.org/2323823004/diff/140001/ui/base/ime/chromeos/i... ui/base/ime/chromeos/input_method_manager.h:311: virtual bool IsEHVInputOnImeMenuEnabled() = 0; On 2016/09/12 17:05:57, James Cook wrote: > This should be called something else. I don't know what EHV means, and neither > will anyone reading the calling code. > > IsEmojiHandwritingVoiceOnImeMenuEnabled? > > AreExtraInputsOnImeMenuEnabled and document what all the extra inputs are? > > separate functions for each part? Renamed as IsEmojiHandwritingVoiceOnImeMenuEnabled.
azurewei@chromium.org changed reviewers: + hidehiko@chromium.org, sky@chromium.org
R: +sky@ for all related files listens to KeyboardControllObserver. +hidehiko@ for files: components/arc/ime/arc_ime_service.h/.cc
https://codereview.chromium.org/2323823004/diff/180001/ui/keyboard/keyboard_c... File ui/keyboard/keyboard_controller_observer.h (right): https://codereview.chromium.org/2323823004/diff/180001/ui/keyboard/keyboard_c... ui/keyboard/keyboard_controller_observer.h:27: virtual void OnKeyboardHidden() = 0; Given most call sites don't care about this is there a reason to make it pure virtual instead of having an empty implementation?
//ash/* LGTM with nits. https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2323823004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1223: auto i = overriden_url.find("#id"); On 2016/09/14 06:53:28, Azure Wei wrote: > On 2016/09/12 17:05:57, James Cook wrote: > > Parsing URLs as strings is discouraged. Is there a way to do this with GURL or > > other URL utilities? > > Updated with using ReplaceComponents(), it this better? Yes, thanks. https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:306: void ImeMenuTray::ShowKeyboardWithKeyset(const std::string& keyset) { I like this being here instead of in //chrome. https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:437: // disables the keyboard when it's hidden. nit: disables -> disable https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:438: if (force_to_show_keyboard_) { optional nit: |force_show_keyboard_|? optional nit: if (!force_shelf_keyboard) return; https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc (right): https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:251: WmShell::Get()->accessibility_delegate()->SetVirtualKeyboardEnabled(true); optional: The lines might wrap a little better if you put the AccessibilityDelegate* in a local variable. https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:259: ASSERT_TRUE( nit: This and below could be EXPECT_TRUE. In general, ASSERT_TRUE is for things that will crash the test if they are false (like a null ptr) or things that are a precondition for the test to run correctly (like you did at the top of this test). https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:276: ASSERT_TRUE( ditto https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:284: } Thanks for adding nice, simple tests! https://codereview.chromium.org/2323823004/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2323823004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1219: void InputMethodManagerImpl::OverrideKeyboardRef(const std::string& keyset) { nit: maybe OverrideKeyboardUrlRef? Or UpdateKeyboardUrl? Something with url would be good. https://codereview.chromium.org/2323823004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1223: // not system IME, and we don't support show e/v/h input for such IME nit: spell out emoji, voice, ... https://codereview.chromium.org/2323823004/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc (right): https://codereview.chromium.org/2323823004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1431: GURL default_url_("chrome://inputview.html"); nit: no trailing _ for local variable https://codereview.chromium.org/2323823004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1434: EXPECT_EQ(default_url_, keyboard::GetOverrideContentUrl()); optional: it's OK to copy/paste strings in tests if you don't want to use a local Or you could make the GURL const Just for fun, some notes about URLs in tests: go/tott-338 https://codereview.chromium.org/2323823004/diff/180001/ui/base/ime/chromeos/i... File ui/base/ime/chromeos/input_method_manager.h (right): https://codereview.chromium.org/2323823004/diff/180001/ui/base/ime/chromeos/i... ui/base/ime/chromeos/input_method_manager.h:308: // string) with the given keyset (emoji, handwriting or voice). Nice comment.
lgtm for components/arc/*, but maybe it'll unnecessary if you follow sky@'s comment.
The CQ bit was checked by azurewei@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by azurewei@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:437: // disables the keyboard when it's hidden. On 2016/09/14 21:14:27, James Cook wrote: > nit: disables -> disable Done. https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:438: if (force_to_show_keyboard_) { On 2016/09/14 21:14:27, James Cook wrote: > optional nit: |force_show_keyboard_|? > > optional nit: > if (!force_shelf_keyboard) > return; Done. https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc (right): https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:251: WmShell::Get()->accessibility_delegate()->SetVirtualKeyboardEnabled(true); On 2016/09/14 21:14:28, James Cook wrote: > optional: The lines might wrap a little better if you put the > AccessibilityDelegate* in a local variable. Done. https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:259: ASSERT_TRUE( On 2016/09/14 21:14:28, James Cook wrote: > nit: This and below could be EXPECT_TRUE. In general, ASSERT_TRUE is for things > that will crash the test if they are false (like a null ptr) or things that are > a precondition for the test to run correctly (like you did at the top of this > test). Done. Thanks. https://codereview.chromium.org/2323823004/diff/180001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:276: ASSERT_TRUE( On 2016/09/14 21:14:27, James Cook wrote: > ditto Done. https://codereview.chromium.org/2323823004/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2323823004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1219: void InputMethodManagerImpl::OverrideKeyboardRef(const std::string& keyset) { On 2016/09/14 21:14:28, James Cook wrote: > nit: maybe OverrideKeyboardUrlRef? Or UpdateKeyboardUrl? Something with url > would be good. Done. https://codereview.chromium.org/2323823004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1223: // not system IME, and we don't support show e/v/h input for such IME On 2016/09/14 21:14:28, James Cook wrote: > nit: spell out emoji, voice, ... Done. https://codereview.chromium.org/2323823004/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc (right): https://codereview.chromium.org/2323823004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1431: GURL default_url_("chrome://inputview.html"); On 2016/09/14 21:14:28, James Cook wrote: > nit: no trailing _ for local variable Done. https://codereview.chromium.org/2323823004/diff/180001/ui/keyboard/keyboard_c... File ui/keyboard/keyboard_controller_observer.h (right): https://codereview.chromium.org/2323823004/diff/180001/ui/keyboard/keyboard_c... ui/keyboard/keyboard_controller_observer.h:27: virtual void OnKeyboardHidden() = 0; On 2016/09/14 18:03:43, sky wrote: > Given most call sites don't care about this is there a reason to make it pure > virtual instead of having an empty implementation? Updated with virtual method with empty implementation. Please take another look. Thanks.
sky@chromium.org changed reviewers: - sky@chromium.org
Thanks for making the function have an empty implementation. I don't believe you need me as a reviewer anymore. If I'm wrong, add me back and let me know what you need me to review.
ash still lgtm
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/2323823004/#ps220001 (title: "Fix patch error.")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== For emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions: 1) Overrides the input view url with append parameter: '#id=emoji/handwriting/voice', which will tell the virtual keyboard to open what kind of keyboard. 2) Show the virtual keyboard directly if 'On-screen keyboard' is enabled. 3) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once. 4) Add KeyboardControllerObserver::OnKeyboardHidden() to notify ImeMenuTray disable the onscreen keyboard it the tray forces to bring out the keyboard. 5) Add related tests: InputMethodManagerImplTest.OverrideKeyboardRefWithEmoji ImeMenuTrayTest.ShowEmojiKeyset ImeMenuTrayTest.ForceToShowEmojiKeyse BUG=642385 TEST=Verified on local build. ========== to ========== For emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions: 1) Overrides the input view url with append parameter: emoji, hwt or voice, which will tell the virtual keyboard to open what kind of keyboard. 2) For Emoji and handwriting input, override the 'id' part like: id=emoji or id=hwt. For voice input, append 'voice' after the 'id' part like: id=us.compact.qwerty.voice 3) Show the virtual keyboard directly if 'On-screen keyboard' is enabled. 4) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once. 5) Add KeyboardControllerObserver::OnKeyboardHidden() to notify ImeMenuTray disable the onscreen keyboard it the tray forces to bring out the keyboard. 6) Add related tests: InputMethodManagerImplTest.Override* ImeMenuTrayTest.ShowEmojiKeyset ImeMenuTrayTest.ForceToShowEmojiKeyse BUG=642385 TEST=Verified on local build. ==========
azurewei@chromium.org changed reviewers: + sky@chromium.org
On 2016/09/15 03:53:59, sky wrote: > Thanks for making the function have an empty implementation. I don't believe you > need me as a reviewer anymore. If I'm wrong, add me back and let me know what > you need me to review. Hi sky@, this cl still needs your review for files under ui/keyboard/. PTAL. Thanks!
sky@chromium.org changed reviewers: + bshe@chromium.org
+bshe for ui/keyboard
https://codereview.chromium.org/2323823004/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2323823004/diff/280001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:140: keyset = "emoji"; please add comments for whether these magic strings are consumed. https://codereview.chromium.org/2323823004/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2323823004/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1234: if (keyset == "keyboard") { you can use empty string to determine whether to reset the url.
https://codereview.chromium.org/2323823004/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2323823004/diff/280001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:140: keyset = "emoji"; On 2016/09/19 01:39:28, Shu Chen wrote: > please add comments for whether these magic strings are consumed. Done. https://codereview.chromium.org/2323823004/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2323823004/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1234: if (keyset == "keyboard") { On 2016/09/19 01:39:28, Shu Chen wrote: > you can use empty string to determine whether to reset the url. Done.
https://codereview.chromium.org/2323823004/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2323823004/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1232: // If |keyset| is empty, it indicates that we should override the url back Suggest to move this comment to the interface definition in input_method_manager.h. https://codereview.chromium.org/2323823004/diff/300001/ui/base/ime/chromeos/i... File ui/base/ime/chromeos/input_method_manager.h (right): https://codereview.chromium.org/2323823004/diff/300001/ui/base/ime/chromeos/i... ui/base/ime/chromeos/input_method_manager.h:308: // string) with the given keyset (emoji, handwriting or voice). This comment needs to be updated.
https://codereview.chromium.org/2323823004/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2323823004/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1232: // If |keyset| is empty, it indicates that we should override the url back On 2016/09/19 06:02:10, Shu Chen wrote: > Suggest to move this comment to the interface definition in > input_method_manager.h. Done. https://codereview.chromium.org/2323823004/diff/300001/ui/base/ime/chromeos/i... File ui/base/ime/chromeos/input_method_manager.h (right): https://codereview.chromium.org/2323823004/diff/300001/ui/base/ime/chromeos/i... ui/base/ime/chromeos/input_method_manager.h:308: // string) with the given keyset (emoji, handwriting or voice). On 2016/09/19 06:02:10, Shu Chen wrote: > This comment needs to be updated. Done.
lgtm
The CQ bit was checked by azurewei@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/19 12:08:30, Shu Chen wrote: > lgtm ui/keyboard lgtm
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2323823004/#ps320001 (title: "")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== For emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions: 1) Overrides the input view url with append parameter: emoji, hwt or voice, which will tell the virtual keyboard to open what kind of keyboard. 2) For Emoji and handwriting input, override the 'id' part like: id=emoji or id=hwt. For voice input, append 'voice' after the 'id' part like: id=us.compact.qwerty.voice 3) Show the virtual keyboard directly if 'On-screen keyboard' is enabled. 4) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once. 5) Add KeyboardControllerObserver::OnKeyboardHidden() to notify ImeMenuTray disable the onscreen keyboard it the tray forces to bring out the keyboard. 6) Add related tests: InputMethodManagerImplTest.Override* ImeMenuTrayTest.ShowEmojiKeyset ImeMenuTrayTest.ForceToShowEmojiKeyse BUG=642385 TEST=Verified on local build. ========== to ========== For emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions: 1) Overrides the input view url with append parameter: emoji, hwt or voice, which will tell the virtual keyboard to open what kind of keyboard. 2) For Emoji and handwriting input, override the 'id' part like: id=emoji or id=hwt. For voice input, append 'voice' after the 'id' part like: id=us.compact.qwerty.voice 3) Show the virtual keyboard directly if 'On-screen keyboard' is enabled. 4) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once. 5) Add KeyboardControllerObserver::OnKeyboardHidden() to notify ImeMenuTray disable the onscreen keyboard it the tray forces to bring out the keyboard. 6) Add related tests: InputMethodManagerImplTest.Override* ImeMenuTrayTest.ShowEmojiKeyset ImeMenuTrayTest.ForceToShowEmojiKeyset BUG=642385 TEST=Verified on local build. ==========
Message was sent while issue was closed.
Description was changed from ========== For emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions: 1) Overrides the input view url with append parameter: emoji, hwt or voice, which will tell the virtual keyboard to open what kind of keyboard. 2) For Emoji and handwriting input, override the 'id' part like: id=emoji or id=hwt. For voice input, append 'voice' after the 'id' part like: id=us.compact.qwerty.voice 3) Show the virtual keyboard directly if 'On-screen keyboard' is enabled. 4) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once. 5) Add KeyboardControllerObserver::OnKeyboardHidden() to notify ImeMenuTray disable the onscreen keyboard it the tray forces to bring out the keyboard. 6) Add related tests: InputMethodManagerImplTest.Override* ImeMenuTrayTest.ShowEmojiKeyset ImeMenuTrayTest.ForceToShowEmojiKeyset BUG=642385 TEST=Verified on local build. ========== to ========== For emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions: 1) Overrides the input view url with append parameter: emoji, hwt or voice, which will tell the virtual keyboard to open what kind of keyboard. 2) For Emoji and handwriting input, override the 'id' part like: id=emoji or id=hwt. For voice input, append 'voice' after the 'id' part like: id=us.compact.qwerty.voice 3) Show the virtual keyboard directly if 'On-screen keyboard' is enabled. 4) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once. 5) Add KeyboardControllerObserver::OnKeyboardHidden() to notify ImeMenuTray disable the onscreen keyboard it the tray forces to bring out the keyboard. 6) Add related tests: InputMethodManagerImplTest.Override* ImeMenuTrayTest.ShowEmojiKeyset ImeMenuTrayTest.ForceToShowEmojiKeyset BUG=642385 TEST=Verified on local build. ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== For emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions: 1) Overrides the input view url with append parameter: emoji, hwt or voice, which will tell the virtual keyboard to open what kind of keyboard. 2) For Emoji and handwriting input, override the 'id' part like: id=emoji or id=hwt. For voice input, append 'voice' after the 'id' part like: id=us.compact.qwerty.voice 3) Show the virtual keyboard directly if 'On-screen keyboard' is enabled. 4) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once. 5) Add KeyboardControllerObserver::OnKeyboardHidden() to notify ImeMenuTray disable the onscreen keyboard it the tray forces to bring out the keyboard. 6) Add related tests: InputMethodManagerImplTest.Override* ImeMenuTrayTest.ShowEmojiKeyset ImeMenuTrayTest.ForceToShowEmojiKeyset BUG=642385 TEST=Verified on local build. ========== to ========== For emoji, voice and handwriting buttons on the opt-in IME menu, adds the corresponding actions: 1) Overrides the input view url with append parameter: emoji, hwt or voice, which will tell the virtual keyboard to open what kind of keyboard. 2) For Emoji and handwriting input, override the 'id' part like: id=emoji or id=hwt. For voice input, append 'voice' after the 'id' part like: id=us.compact.qwerty.voice 3) Show the virtual keyboard directly if 'On-screen keyboard' is enabled. 4) Forcibly enables the a11y onscreen keyboard if there is on keyboard enabled. And re-disables it after showing once. 5) Add KeyboardControllerObserver::OnKeyboardHidden() to notify ImeMenuTray disable the onscreen keyboard it the tray forces to bring out the keyboard. 6) Add related tests: InputMethodManagerImplTest.Override* ImeMenuTrayTest.ShowEmojiKeyset ImeMenuTrayTest.ForceToShowEmojiKeyset BUG=642385 TEST=Verified on local build. Committed: https://crrev.com/649030a2f49f6d868378de840ff08eb180bdffb6 Cr-Commit-Position: refs/heads/master@{#419735} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/649030a2f49f6d868378de840ff08eb180bdffb6 Cr-Commit-Position: refs/heads/master@{#419735}
Message was sent while issue was closed.
On 2016/09/20 13:13:39, commit-bot: I haz the power wrote: > Patchset 17 (id:??) landed as > https://crrev.com/649030a2f49f6d868378de840ff08eb180bdffb6 > Cr-Commit-Position: refs/heads/master@{#419735} Is there a missing dep somewhere? I'm seeing build errors like this on a Windows component build: FAILED: ash_library.dll ash_library.dll.lib ash_library.dll.pdb C:/python_27_amd64/files/python.exe ../../build/toolchain/win/tool_wrapper.py link-wrapper environment.x64 False link.exe /nologo /IMPLIB:./ash_library.dll.lib /DLL /OUT:./ash_library.dll /PDB:./ash_library.dll.pdb @./ash_library.dll.rsp root_window_controller.obj : error LNK2001: unresolved external symbol "public: virtual void __cdecl keyboard::KeyboardControllerObserver::OnKeyboardHidden(void)" (?OnKeyboardHidden@KeyboardControllerObserver@keyboard@@UEAAXXZ) ./ash_library.dll : fatal error LNK1120: 1 unresolved externals
Message was sent while issue was closed.
On 2016/09/22 20:03:01, dcheng wrote: > On 2016/09/20 13:13:39, commit-bot: I haz the power wrote: > > Patchset 17 (id:??) landed as > > https://crrev.com/649030a2f49f6d868378de840ff08eb180bdffb6 > > Cr-Commit-Position: refs/heads/master@{#419735} > > Is there a missing dep somewhere? I'm seeing build errors like this on a Windows > component build: > > FAILED: ash_library.dll ash_library.dll.lib ash_library.dll.pdb > C:/python_27_amd64/files/python.exe ../../build/toolchain/win/tool_wrapper.py > link-wrapper environment.x64 False link.exe /nologo > /IMPLIB:./ash_library.dll.lib /DLL /OUT:./ash_library.dll > /PDB:./ash_library.dll.pdb @./ash_library.dll.rsp > root_window_controller.obj : error LNK2001: unresolved external symbol "public: > virtual void __cdecl > keyboard::KeyboardControllerObserver::OnKeyboardHidden(void)" > (?OnKeyboardHidden@KeyboardControllerObserver@keyboard@@UEAAXXZ) > ./ash_library.dll : fatal error LNK1120: 1 unresolved externals Hmm.... something is weird with my repo. I'll keep poking at it...
Message was sent while issue was closed.
On 2016/09/22 20:12:19, dcheng wrote: > On 2016/09/22 20:03:01, dcheng wrote: > > On 2016/09/20 13:13:39, commit-bot: I haz the power wrote: > > > Patchset 17 (id:??) landed as > > > https://crrev.com/649030a2f49f6d868378de840ff08eb180bdffb6 > > > Cr-Commit-Position: refs/heads/master@{#419735} > > > > Is there a missing dep somewhere? I'm seeing build errors like this on a > Windows > > component build: > > > > FAILED: ash_library.dll ash_library.dll.lib ash_library.dll.pdb > > C:/python_27_amd64/files/python.exe ../../build/toolchain/win/tool_wrapper.py > > link-wrapper environment.x64 False link.exe /nologo > > /IMPLIB:./ash_library.dll.lib /DLL /OUT:./ash_library.dll > > /PDB:./ash_library.dll.pdb @./ash_library.dll.rsp > > root_window_controller.obj : error LNK2001: unresolved external symbol > "public: > > virtual void __cdecl > > keyboard::KeyboardControllerObserver::OnKeyboardHidden(void)" > > (?OnKeyboardHidden@KeyboardControllerObserver@keyboard@@UEAAXXZ) > > ./ash_library.dll : fatal error LNK1120: 1 unresolved externals > > Hmm.... something is weird with my repo. I'll keep poking at it... OK, it's a missing dep, I'll send a fix for review. (We should really have better build coverage...) |
