Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2521443002:
Reset settings button when update the list view. (Closed)
|
Created:
4 years, 1 month ago by Azure Wei Modified:
4 years ago Reviewers:
tdanderson CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nona+watch_chromium.org, oshima+watch_chromium.org, kalyank Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReset settings button when update the list view.
We need to reset the settings button object when updating the IME detailed view, otherwise there may has memory leak, and checking if(settings_button_) could be true but the IME view is updated without drawing the settings button.
BUG=667105
TEST=Verified on Clapper.
Committed: https://crrev.com/4ac84d7c1ef8eb9c8d1ab59608975de1db3c7821
Cr-Commit-Position: refs/heads/master@{#434083}
Patch Set 1 #
Total comments: 1
Messages
Total messages: 19 (13 generated)
Description was changed from ========== Reset settings button when update the list view. BUG=667105 TEST=Verfied on local build. ========== to ========== Reset settings button when update the list view. BUG=667105 TEST=Verfied on local build. ==========
azurewei@chromium.org changed reviewers: + tdanderson@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: This issue passed the CQ dry run.
Please review this CL. Thanks!
Description was changed from ========== Reset settings button when update the list view. BUG=667105 TEST=Verfied on local build. ========== to ========== Reset settings button when update the list view. BUG=667105, 666787 TEST=Verified on Clapper. ==========
LGTM for issue 667105. But you also list issue 666787 in the CL description, and I'm not sure how this CL is related to that issue. Can you please clarify? https://codereview.chromium.org/2521443002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/ime_menu/ime_list_view.h (right): https://codereview.chromium.org/2521443002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_list_view.h:3: // found in the LICENSE file. nit: Can you include a bit more of an explanation regarding what this change does in the CL description? Someone just reading the CL title as-is wouldn't necessarily know what it means.
Description was changed from ========== Reset settings button when update the list view. BUG=667105, 666787 TEST=Verified on Clapper. ========== to ========== Reset settings button when update the list view. We need to reset the settings button object when updating the IME detailed view, otherwise there may has memory leak, and checking if(settings_button_) could be true but the IME view is updated without drawing the settings button. BUG=667105, 666787 TEST=Verified on Clapper. ==========
Description was changed from ========== Reset settings button when update the list view. We need to reset the settings button object when updating the IME detailed view, otherwise there may has memory leak, and checking if(settings_button_) could be true but the IME view is updated without drawing the settings button. BUG=667105, 666787 TEST=Verified on Clapper. ========== to ========== Reset settings button when update the list view. We need to reset the settings button object when updating the IME detailed view, otherwise there may has memory leak, and checking if(settings_button_) could be true but the IME view is updated without drawing the settings button. BUG=667105 TEST=Verified on Clapper. ==========
On 2016/11/22 21:16:39, tdanderson wrote: > LGTM for issue 667105. But you also list issue 666787 in the CL description, and > I'm not sure how this CL is related to that issue. Can you please clarify? > > https://codereview.chromium.org/2521443002/diff/1/ash/common/system/chromeos/... > File ash/common/system/chromeos/ime_menu/ime_list_view.h (right): > > https://codereview.chromium.org/2521443002/diff/1/ash/common/system/chromeos/... > ash/common/system/chromeos/ime_menu/ime_list_view.h:3: // found in the LICENSE > file. > nit: Can you include a bit more of an explanation regarding what this change > does in the CL description? Someone just reading the CL title as-is wouldn't > necessarily know what it means. I was misunderstanding bug:666787. This CL only effects settings button but not VK toggle. I've update the CL description.
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...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1479863857133840, "parent_rev":
"d5eef4f78285791fb8f72bae7f2c9ce8911751d5", "commit_rev":
"e2e7aee0ff2acbb7a9be592e51f5bc174f814178"}
Message was sent while issue was closed.
Description was changed from ========== Reset settings button when update the list view. We need to reset the settings button object when updating the IME detailed view, otherwise there may has memory leak, and checking if(settings_button_) could be true but the IME view is updated without drawing the settings button. BUG=667105 TEST=Verified on Clapper. ========== to ========== Reset settings button when update the list view. We need to reset the settings button object when updating the IME detailed view, otherwise there may has memory leak, and checking if(settings_button_) could be true but the IME view is updated without drawing the settings button. BUG=667105 TEST=Verified on Clapper. ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Reset settings button when update the list view. We need to reset the settings button object when updating the IME detailed view, otherwise there may has memory leak, and checking if(settings_button_) could be true but the IME view is updated without drawing the settings button. BUG=667105 TEST=Verified on Clapper. ========== to ========== Reset settings button when update the list view. We need to reset the settings button object when updating the IME detailed view, otherwise there may has memory leak, and checking if(settings_button_) could be true but the IME view is updated without drawing the settings button. BUG=667105 TEST=Verified on Clapper. Committed: https://crrev.com/4ac84d7c1ef8eb9c8d1ab59608975de1db3c7821 Cr-Commit-Position: refs/heads/master@{#434083} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/4ac84d7c1ef8eb9c8d1ab59608975de1db3c7821 Cr-Commit-Position: refs/heads/master@{#434083} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
