Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1830543003:
Using STL to implement functionality in input_method_util.cc (Closed)
|
Created:
4 years, 9 months ago by chakshu Modified:
4 years, 8 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nona+watch_chromium.org, rlp+watch_chromium.org, rouslan+spell_chromium.org, oshima+watch_chromium.org, groby+spellwatch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUsing STL to implement functionality in input_method_util.cc
BUG=None
Committed: https://crrev.com/a1334adbb8a24a39118f272ed00d5e0d7de72600
Cr-Commit-Position: refs/heads/master@{#386963}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Changes as per review comments #Patch Set 3 : Deleted the changes made in spellcheck_service #Patch Set 4 : Updated as per latest code #Messages
Total messages: 34 (14 generated)
Description was changed from ========== Using STL to implement functionality in input_method_util.cc and spellcheck_service.cc BUG=561800 ========== to ========== Using STL to implement functionality in input_method_util.cc and spellcheck_service.cc BUG=561800 ==========
chakshu.a@samsung.com changed reviewers: + r.kasibhatla@samsung.com
On 2016/03/24 09:55:43, chakshu wrote: > mailto:chakshu.a@samsung.com changed reviewers: > + mailto:r.kasibhatla@samsung.com samsung-peer-l-g-t-m
chakshu.a@samsung.com changed reviewers: + groby@chromium.org, shuchen@chromium.org
shuchen@chromium.org: Please review changes in input_method_util.cc groby@chromium.org: Please review changes in spellcheck_service.cc
https://codereview.chromium.org/1830543003/diff/1/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/input_method_util.cc (right): https://codereview.chromium.org/1830543003/diff/1/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/input_method_util.cc:693: std::sort(ids.begin(), ids.end()); The sort() here is risky since the order of the input method ids matters.
Query regarding use of std::unique https://codereview.chromium.org/1830543003/diff/1/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/input_method_util.cc (right): https://codereview.chromium.org/1830543003/diff/1/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/input_method_util.cc:693: std::sort(ids.begin(), ids.end()); On 2016/03/24 13:05:31, Shu Chen wrote: > The sort() here is risky since the order of the input method ids matters. Okay, I was not aware that the order of input method ids matters. I was suggested to use std::unique for this (https://codereview.chromium.org/1715683002/diff/20001/chrome/browser/chromeos...) for which it is required to sort. So should I use unique with some modifications so that the order is retained? Or just roll back this change?
https://codereview.chromium.org/1830543003/diff/1/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/input_method_util.cc (right): https://codereview.chromium.org/1830543003/diff/1/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/input_method_util.cc:693: std::sort(ids.begin(), ids.end()); On 2016/04/04 05:28:32, chakshu wrote: > On 2016/03/24 13:05:31, Shu Chen wrote: > > The sort() here is risky since the order of the input method ids matters. > > Okay, I was not aware that the order of input method ids matters. I was > suggested to use std::unique for this > (https://codereview.chromium.org/1715683002/diff/20001/chrome/browser/chromeos...) > for which it is required to sort. > > So should I use unique with some modifications so that the order is retained? Or > just roll back this change? I'm not sure whether std::unique can do the thing. But using std::unordered_set should be enough to improve the performance here. Thanks.
Also: This change is _not_ related to the bug quoted. The bug specifically refers to changing find to ContainsValue. https://codereview.chromium.org/1830543003/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1830543003/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_service.cc:357: std::sort(dictionaries.begin(), dictionaries.end()); Before modifying things, please check the history _why_ it's different from what you think is the better pattern. In this case, https://codereview.chromium.org/1673783003 - we cannot change the order of dictionaries because that affects a preference the user has expressed.
Description was changed from ========== Using STL to implement functionality in input_method_util.cc and spellcheck_service.cc BUG=561800 ========== to ========== Using STL to implement functionality in input_method_util.cc and spellcheck_service.cc BUG=None ==========
On 2016/04/04 22:30:40, groby wrote: > Also: This change is _not_ related to the bug quoted. The bug specifically > refers to changing find to ContainsValue. > > https://codereview.chromium.org/1830543003/diff/1/chrome/browser/spellchecker... > File chrome/browser/spellchecker/spellcheck_service.cc (right): > > https://codereview.chromium.org/1830543003/diff/1/chrome/browser/spellchecker... > chrome/browser/spellchecker/spellcheck_service.cc:357: > std::sort(dictionaries.begin(), dictionaries.end()); > Before modifying things, please check the history _why_ it's different from what > you think is the better pattern. > > In this case, https://codereview.chromium.org/1673783003 - we cannot change the > order of dictionaries because that affects a preference the user has expressed. @groby Sorry about that, I have removed the Bug quoted.
Changes as per review comments https://codereview.chromium.org/1830543003/diff/1/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/input_method_util.cc (right): https://codereview.chromium.org/1830543003/diff/1/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/input_method_util.cc:693: std::sort(ids.begin(), ids.end()); On 2016/04/04 12:48:15, Shu Chen wrote: > On 2016/04/04 05:28:32, chakshu wrote: > > On 2016/03/24 13:05:31, Shu Chen wrote: > > > The sort() here is risky since the order of the input method ids matters. > > > > Okay, I was not aware that the order of input method ids matters. I was > > suggested to use std::unique for this > > > (https://codereview.chromium.org/1715683002/diff/20001/chrome/browser/chromeos...) > > for which it is required to sort. > > > > So should I use unique with some modifications so that the order is retained? > Or > > just roll back this change? > > I'm not sure whether std::unique can do the thing. > But using std::unordered_set should be enough to improve the performance here. > Thanks. Done ! Also if you want it to be done in-place without creating the new_ids vector but instead erase directly in ids vector, let me know. I will make that change else this should be fine. Thank you. https://codereview.chromium.org/1830543003/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1830543003/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_service.cc:357: std::sort(dictionaries.begin(), dictionaries.end()); On 2016/04/04 22:30:40, groby wrote: > Before modifying things, please check the history _why_ it's different from what > you think is the better pattern. > > In this case, https://codereview.chromium.org/1673783003 - we cannot change the > order of dictionaries because that affects a preference the user has expressed. Thank you for the review. Using unordered_set will improve the performance so I think it is the better pattern from what has been done till now. Now I have also taken care that the order of dictionaries does not change.
lgtm for input_method_util.cc.
On 2016/04/11 20:46:02, Shu Chen wrote: > lgtm for input_method_util.cc. not lgtm There is no reason to make this change in spellcheck_service.
On 2016/04/12 00:59:19, groby wrote: > On 2016/04/11 20:46:02, Shu Chen wrote: > > lgtm for input_method_util.cc. > > not lgtm > > There is no reason to make this change in spellcheck_service. Okay, I will revert back the changes made in spellcheck_service.
The CQ bit was checked by chakshu.a@samsung.com
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/1830543003/#ps40001 (title: "Deleted the changes made in spellcheck_service")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1830543003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1830543003/40001
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: groby@chromium.org. Please make sure to get positive review before another CQ attempt.
On 2016/04/12 05:20:08, chakshu wrote: > On 2016/04/12 00:59:19, groby wrote: > > On 2016/04/11 20:46:02, Shu Chen wrote: > > > lgtm for input_method_util.cc. > > > > not lgtm > > > > There is no reason to make this change in spellcheck_service. > > Okay, I will revert back the changes made in spellcheck_service. Removed the changes in spellcheck_service as per review comments. Can you please take a look at it? Because of not lgtm, I am not able to commit changes in the other file as well.
Description was changed from ========== Using STL to implement functionality in input_method_util.cc and spellcheck_service.cc BUG=None ========== to ========== Using STL to implement functionality in input_method_util.cc BUG=None ==========
On 2016/04/12 05:29:09, chakshu wrote: > On 2016/04/12 05:20:08, chakshu wrote: > > On 2016/04/12 00:59:19, groby wrote: > > > On 2016/04/11 20:46:02, Shu Chen wrote: > > > > lgtm for input_method_util.cc. > > > > > > not lgtm > > > > > > There is no reason to make this change in spellcheck_service. > > > > Okay, I will revert back the changes made in spellcheck_service. > > Removed the changes in spellcheck_service as per review comments. Can you please > take a look at it? > Because of not lgtm, I am not able to commit changes in the other file as well. LGTM for (removed) spellcheck.
The CQ bit was checked by chakshu.a@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1830543003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1830543003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn 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_...)
The CQ bit was checked by chakshu.a@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org, groby@chromium.org Link to the patchset: https://codereview.chromium.org/1830543003/#ps60001 (title: "Updated as per latest code")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1830543003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1830543003/60001
Message was sent while issue was closed.
Description was changed from ========== Using STL to implement functionality in input_method_util.cc BUG=None ========== to ========== Using STL to implement functionality in input_method_util.cc BUG=None ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Using STL to implement functionality in input_method_util.cc BUG=None ========== to ========== Using STL to implement functionality in input_method_util.cc BUG=None Committed: https://crrev.com/a1334adbb8a24a39118f272ed00d5e0d7de72600 Cr-Commit-Position: refs/heads/master@{#386963} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a1334adbb8a24a39118f272ed00d5e0d7de72600 Cr-Commit-Position: refs/heads/master@{#386963} |
