|
|
Created:
4 years, 2 months ago by Azure Wei Modified:
4 years, 1 month ago Reviewers:
Shu Chen CC:
chromium-reviews, nona+watch_chromium.org, oshima+watch_chromium.org, shuchen+watch_chromium.org, yusukes+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOverride keyset with the IME layout info.
For system IME extension, the default input view url is:
chrome-extension://${extension_id}/inputview.html#id=us.compact.qwerty&language=en-US&passwordLayout=us.compact.qwerty&name=keyboard_us
To show the keyboard with emoji/handwriting keyset directly, we used to override the default input view url as "id=emoji/hwt" directly.
While, the keyboard needs to know the default keyset to get the right 'back to main layout' behavior.
Thus, we override the url as id=${keyset}.emoji/hwt to pass the default keyset and specific keyset info together.
Reload the keyset to notify the hash changed when hide keyboard. Thus the VK will get the onhashchange event when
show with from clicking voice button for many times.
BUG=654247, 654248
TEST=InputMethodManagerImplTest.Override*
Committed: https://crrev.com/9abb3ee29957feee6fa1eacb407238fbc9b48a6e
Cr-Commit-Position: refs/heads/master@{#427637}
Patch Set 1 #Patch Set 2 : No need to reload. #Patch Set 3 : Add call of Reload() back. #
Total comments: 2
Patch Set 4 : Make safer replacement. #
Total comments: 1
Patch Set 5 : Addressed comments. #Patch Set 6 #
Messages
Total messages: 30 (16 generated)
Description was changed from ========== Override keyset with the IME layout info. BUG=65424 TEST=InputMethodManagerImplTest.Override* ========== to ========== Override keyset with the IME layout info. For system IME extension, the default input view url is: chrome-extension://${extension_id}/inputview.html#id=us.compact.qwerty&language=en-US&passwordLayout=us.compact.qwerty&name=keyboard_us To show the keyboard with emoji/handwriting keyset directly, we used to override the default input view url as "id=emoji/hwt" directly. While, the keyboard needs to know the default keyset to get the right 'back to main layout' behavior. Thus, we override the url as id=${keyset}.emoji/hwt to pass the default keyset and specific keyset info together. BUG=654247 TEST=InputMethodManagerImplTest.Override* ==========
azurewei@chromium.org changed reviewers: + shuchen@chromium.org
Please review this CL. Thanks!
On 2016/10/19 12:05:35, Azure Wei wrote: > Please review this CL. Thanks! Removed the calling of 'keyboard_controller->Reload()' in patch set 1. It's unnecessary to reload the keyboard when hiding it. And it will be automatically reloaded when showing it. Please take another look. Thanks!
Description was changed from ========== Override keyset with the IME layout info. For system IME extension, the default input view url is: chrome-extension://${extension_id}/inputview.html#id=us.compact.qwerty&language=en-US&passwordLayout=us.compact.qwerty&name=keyboard_us To show the keyboard with emoji/handwriting keyset directly, we used to override the default input view url as "id=emoji/hwt" directly. While, the keyboard needs to know the default keyset to get the right 'back to main layout' behavior. Thus, we override the url as id=${keyset}.emoji/hwt to pass the default keyset and specific keyset info together. BUG=654247 TEST=InputMethodManagerImplTest.Override* ========== to ========== Override keyset with the IME layout info. For system IME extension, the default input view url is: chrome-extension://${extension_id}/inputview.html#id=us.compact.qwerty&language=en-US&passwordLayout=us.compact.qwerty&name=keyboard_us To show the keyboard with emoji/handwriting keyset directly, we used to override the default input view url as "id=emoji/hwt" directly. While, the keyboard needs to know the default keyset to get the right 'back to main layout' behavior. Thus, we override the url as id=${keyset}.emoji/hwt to pass the default keyset and specific keyset info together. Reload the keyset to notify the hash changed when hide keyboard. Thus the VK will get the onhashchange event when show with from clicking voice button for many times. BUG=654247, 654248 TEST=InputMethodManagerImplTest.Override* ==========
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/10/21 07:06:58, Azure Wei wrote: > On 2016/10/19 12:05:35, Azure Wei wrote: > > Please review this CL. Thanks! > > Removed the calling of 'keyboard_controller->Reload()' in patch set 1. > It's unnecessary to reload the keyboard when hiding it. And it will be > automatically reloaded when showing it. > Please take another look. Thanks! Sorry, I've added the calling of 'keyboard_controller->Reload()' back. It's still needed by case to open voice input for more than one times. See crbug.com/654248/.
https://codereview.chromium.org/2436723002/diff/30001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2436723002/diff/30001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1248: overridden_ref.replace(j, 0, "." + keyset); To reduce the risk of potential breakage, can you please make safer replacement? e.g. find the "id=" and then find the "&" or EOS after it.
https://codereview.chromium.org/2436723002/diff/30001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2436723002/diff/30001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1248: overridden_ref.replace(j, 0, "." + keyset); On 2016/10/24 01:30:47, Shu Chen wrote: > To reduce the risk of potential breakage, can you please make safer replacement? > e.g. find the "id=" and then find the "&" or EOS after it. Done.
lgtm https://codereview.chromium.org/2436723002/diff/50001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2436723002/diff/50001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1248: if (j == std::string::npos) if (j == std::string::npos) overridden_ref += "." + keyset;
On 2016/10/25 13:24:59, Shu Chen wrote: > lgtm > > https://codereview.chromium.org/2436723002/diff/50001/chrome/browser/chromeos... > File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): > > https://codereview.chromium.org/2436723002/diff/50001/chrome/browser/chromeos... > chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1248: if (j == > std::string::npos) > if (j == std::string::npos) > overridden_ref += "." + keyset; Done. Updated and verified.
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org Link to the patchset: https://codereview.chromium.org/2436723002/#ps70001 (title: "Addressed comments.")
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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) 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
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: linux_chromium_chromeos_ozone_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
The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org Link to the patchset: https://codereview.chromium.org/2436723002/#ps90001 (title: "")
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.
Description was changed from ========== Override keyset with the IME layout info. For system IME extension, the default input view url is: chrome-extension://${extension_id}/inputview.html#id=us.compact.qwerty&language=en-US&passwordLayout=us.compact.qwerty&name=keyboard_us To show the keyboard with emoji/handwriting keyset directly, we used to override the default input view url as "id=emoji/hwt" directly. While, the keyboard needs to know the default keyset to get the right 'back to main layout' behavior. Thus, we override the url as id=${keyset}.emoji/hwt to pass the default keyset and specific keyset info together. Reload the keyset to notify the hash changed when hide keyboard. Thus the VK will get the onhashchange event when show with from clicking voice button for many times. BUG=654247, 654248 TEST=InputMethodManagerImplTest.Override* ========== to ========== Override keyset with the IME layout info. For system IME extension, the default input view url is: chrome-extension://${extension_id}/inputview.html#id=us.compact.qwerty&language=en-US&passwordLayout=us.compact.qwerty&name=keyboard_us To show the keyboard with emoji/handwriting keyset directly, we used to override the default input view url as "id=emoji/hwt" directly. While, the keyboard needs to know the default keyset to get the right 'back to main layout' behavior. Thus, we override the url as id=${keyset}.emoji/hwt to pass the default keyset and specific keyset info together. Reload the keyset to notify the hash changed when hide keyboard. Thus the VK will get the onhashchange event when show with from clicking voice button for many times. BUG=654247, 654248 TEST=InputMethodManagerImplTest.Override* ==========
Message was sent while issue was closed.
Committed patchset #6 (id:90001)
Message was sent while issue was closed.
Description was changed from ========== Override keyset with the IME layout info. For system IME extension, the default input view url is: chrome-extension://${extension_id}/inputview.html#id=us.compact.qwerty&language=en-US&passwordLayout=us.compact.qwerty&name=keyboard_us To show the keyboard with emoji/handwriting keyset directly, we used to override the default input view url as "id=emoji/hwt" directly. While, the keyboard needs to know the default keyset to get the right 'back to main layout' behavior. Thus, we override the url as id=${keyset}.emoji/hwt to pass the default keyset and specific keyset info together. Reload the keyset to notify the hash changed when hide keyboard. Thus the VK will get the onhashchange event when show with from clicking voice button for many times. BUG=654247, 654248 TEST=InputMethodManagerImplTest.Override* ========== to ========== Override keyset with the IME layout info. For system IME extension, the default input view url is: chrome-extension://${extension_id}/inputview.html#id=us.compact.qwerty&language=en-US&passwordLayout=us.compact.qwerty&name=keyboard_us To show the keyboard with emoji/handwriting keyset directly, we used to override the default input view url as "id=emoji/hwt" directly. While, the keyboard needs to know the default keyset to get the right 'back to main layout' behavior. Thus, we override the url as id=${keyset}.emoji/hwt to pass the default keyset and specific keyset info together. Reload the keyset to notify the hash changed when hide keyboard. Thus the VK will get the onhashchange event when show with from clicking voice button for many times. BUG=654247, 654248 TEST=InputMethodManagerImplTest.Override* Committed: https://crrev.com/9abb3ee29957feee6fa1eacb407238fbc9b48a6e Cr-Commit-Position: refs/heads/master@{#427637} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9abb3ee29957feee6fa1eacb407238fbc9b48a6e Cr-Commit-Position: refs/heads/master@{#427637} |