Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2271483003:
Updates the IME list when the IME has refreshed. (Closed)
|
Created:
4 years, 4 months ago by Azure Wei Modified:
4 years, 3 months ago Reviewers:
James Cook CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nona+watch_chromium.org, oshima+watch_chromium.org, kalyank, Shu Chen Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdates the IME list when the IME has refreshed.
When the IME refreshes, we need to update the view of the list in IME menu.
Saves the created ImeListView and updates it when get IME refreshing event.
Add ImeMenuTrayTest.RefreshImeWithListViewCreated test.
BUG=640432
TEST=Verified on local build.
Committed: https://crrev.com/c6f28a1effd401fd7e2e03707e0dd84f5346185b
Cr-Commit-Position: refs/heads/master@{#414329}
Patch Set 1 #Patch Set 2 #
Total comments: 20
Patch Set 3 : Remove ImeBubbleWrapper and add test. #Patch Set 4 #
Total comments: 15
Patch Set 5 : Addressed comments. #Patch Set 6 : Addressed comments. #Patch Set 7 : fix patch failure. #
Messages
Total messages: 42 (22 generated)
Description was changed from ========== Updates the IME list when the IME has refreshed. BUG=570761 TEST=Verified on local build. ========== to ========== Updates the IME list when the IME has refreshed. When the IME refreshes, we need to update the view of the list in IME menu. Adds the class ImeBubbleWrapper to wrap and manage the ImeListView, ImeButtonsView and TrayBubbleWrapper instances in the bubble, thus we can updates the ImeListView from ImeBubbleWrapper. BUG=570761 TEST=Verified on local build. ==========
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_compile_dbg_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.
azurewei@chromium.org changed reviewers: + jamescook@chromium.org, shuchen@chromium.org
Please review this cl. Thanks!
https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:143: class ImeBubbleWrapper { Why do you need this class? Could the ImeListView* be stored in ImeMenuTray? https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:145: ImeBubbleWrapper(ImeMenuTray* tray) : tray_(tray) {} nit: explicit https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:165: bubble_view->AddChildView(list_view_.get()); AddChildView() takes ownership of list_view_, so list_view_ should not be a unique_ptr<>. This will probably cause a double-delete when the ImeBubbleWrapper is destroyed. https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:173: bubble_view->AddChildView(button_view_.get()); ditto https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:174: bubble_wrapper_.reset(new ash::TrayBubbleWrapper(tray_, bubble_view)); nit: no ash:: https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:179: if (bubble_wrapper_.get()) { nit: no {} https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:193: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:326: return (bubble_ && bubble_->bubble_view()); nit: no extra () https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc (right): https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:45: bool IsBubbleShown() { return (GetTray()->IsBubbleShownForTesting()); } nit: no () https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:53: TEST_F(ImeMenuTrayTest, ImeMenuTrayVisibility) { Please add a test for the IME menu refreshing.
Description was changed from ========== Updates the IME list when the IME has refreshed. When the IME refreshes, we need to update the view of the list in IME menu. Adds the class ImeBubbleWrapper to wrap and manage the ImeListView, ImeButtonsView and TrayBubbleWrapper instances in the bubble, thus we can updates the ImeListView from ImeBubbleWrapper. BUG=570761 TEST=Verified on local build. ========== to ========== Updates the IME list when the IME has refreshed. When the IME refreshes, we need to update the view of the list in IME menu. Adds the class ImeBubbleWrapper to wrap and manage the ImeListView, ImeButtonsView and TrayBubbleWrapper instances in the bubble, thus we can updates the ImeListView from ImeBubbleWrapper. BUG=640432 TEST=Verified on local build. ==========
Description was changed from ========== Updates the IME list when the IME has refreshed. When the IME refreshes, we need to update the view of the list in IME menu. Adds the class ImeBubbleWrapper to wrap and manage the ImeListView, ImeButtonsView and TrayBubbleWrapper instances in the bubble, thus we can updates the ImeListView from ImeBubbleWrapper. BUG=640432 TEST=Verified on local build. ========== to ========== Updates the IME list when the IME has refreshed. When the IME refreshes, we need to update the view of the list in IME menu. Saves the created ImeListView and updates it when get IME refreshing event. Add ImeMenuTrayTest.RefreshImeWithListViewCreated test. BUG=640432 TEST=Verified on local build. ==========
https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:143: class ImeBubbleWrapper { On 2016/08/23 16:38:33, James Cook wrote: > Why do you need this class? Could the ImeListView* be stored in ImeMenuTray? This class is not unnecessary if storing ImeListView* directly. Just thought it would be better to wrap the bubble stuff together. Removed this class as it's more easy to save and access ImeListView* in ImeMenuTray. https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:145: ImeBubbleWrapper(ImeMenuTray* tray) : tray_(tray) {} On 2016/08/23 16:38:32, James Cook wrote: > nit: explicit Removed this class. https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:165: bubble_view->AddChildView(list_view_.get()); On 2016/08/23 16:38:33, James Cook wrote: > AddChildView() takes ownership of list_view_, so list_view_ should not be a > unique_ptr<>. This will probably cause a double-delete when the ImeBubbleWrapper > is destroyed. Done. https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:173: bubble_view->AddChildView(button_view_.get()); On 2016/08/23 16:38:33, James Cook wrote: > ditto Done. https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:174: bubble_wrapper_.reset(new ash::TrayBubbleWrapper(tray_, bubble_view)); On 2016/08/23 16:38:32, James Cook wrote: > nit: no ash:: Done. https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:179: if (bubble_wrapper_.get()) { On 2016/08/23 16:38:32, James Cook wrote: > nit: no {} Done. https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:193: }; On 2016/08/23 16:38:32, James Cook wrote: > DISALLOW_COPY_AND_ASSIGN Removed this class. https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:326: return (bubble_ && bubble_->bubble_view()); On 2016/08/23 16:38:32, James Cook wrote: > nit: no extra () Done. https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc (right): https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:45: bool IsBubbleShown() { return (GetTray()->IsBubbleShownForTesting()); } On 2016/08/23 16:38:33, James Cook wrote: > nit: no () Done. https://codereview.chromium.org/2271483003/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:53: TEST_F(ImeMenuTrayTest, ImeMenuTrayVisibility) { On 2016/08/23 16:38:33, James Cook wrote: > Please add a test for the IME menu refreshing. Added test: RefreshImeWithListViewCreated.
shuchen@chromium.org changed reviewers: - shuchen@chromium.org
LGTM with nits. https://codereview.chromium.org/2271483003/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc (right): https://codereview.chromium.org/2271483003/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:52: bool IsTrayImeListValid(const std::vector<IMEInfo>& list, nit: How about |expected_list| or |expected_imes| instead of |list|? https://codereview.chromium.org/2271483003/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:59: std::vector<std::string> ime_ids; nit: |expected_ime_ids| or similar https://codereview.chromium.org/2271483003/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:60: for (auto ime : list) { nit: Always use const auto& or auto& or const IMEInfo& for ranged-for loops. This version copies the IMEInfo on every loop iteration, which it doesn't need to do. https://codereview.chromium.org/2271483003/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:63: for (auto ime : ime_map) { ditto https://codereview.chromium.org/2271483003/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:176: } Nice test. Easy to read. https://codereview.chromium.org/2271483003/diff/60001/ash/common/system/tray/... File ash/common/system/tray/ime_info.h (right): https://codereview.chromium.org/2271483003/diff/60001/ash/common/system/tray/... ash/common/system/tray/ime_info.h:18: IMEInfo(bool selected, nit: Don't change this. Using the members explicitly is self-documenting (it's obvious what each value is). See ToTT "Avoid the Long Parameter List" https://drive.google.com/a/google.com/file/d/0B-KFDiTrQe9lUER4YkE5MldGM1k/view https://codereview.chromium.org/2271483003/diff/60001/ash/test/test_system_tr... File ash/test/test_system_tray_delegate.cc (right): https://codereview.chromium.org/2271483003/diff/60001/ash/test/test_system_tr... ash/test/test_system_tray_delegate.cc:71: ime_list_.clear(); Does ime_list_ = list; work? https://codereview.chromium.org/2271483003/diff/60001/ash/test/test_system_tr... ash/test/test_system_tray_delegate.cc:153: for (size_t i = 0; i < ime_list_.size(); i++) { Does *list = ime_list_; work?
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2271483003/#ps100001 (title: "Addressed comments.")
https://codereview.chromium.org/2271483003/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc (right): https://codereview.chromium.org/2271483003/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:52: bool IsTrayImeListValid(const std::vector<IMEInfo>& list, On 2016/08/24 16:03:53, James Cook wrote: > nit: How about |expected_list| or |expected_imes| instead of |list|? Done. https://codereview.chromium.org/2271483003/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:59: std::vector<std::string> ime_ids; On 2016/08/24 16:03:52, James Cook wrote: > nit: |expected_ime_ids| or similar Done. https://codereview.chromium.org/2271483003/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:60: for (auto ime : list) { On 2016/08/24 16:03:53, James Cook wrote: > nit: Always use const auto& or auto& or const IMEInfo& for ranged-for loops. > This version copies the IMEInfo on every loop iteration, which it doesn't need > to do. Done. https://codereview.chromium.org/2271483003/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:63: for (auto ime : ime_map) { On 2016/08/24 16:03:52, James Cook wrote: > ditto Done. https://codereview.chromium.org/2271483003/diff/60001/ash/common/system/tray/... File ash/common/system/tray/ime_info.h (right): https://codereview.chromium.org/2271483003/diff/60001/ash/common/system/tray/... ash/common/system/tray/ime_info.h:18: IMEInfo(bool selected, On 2016/08/24 16:03:53, James Cook wrote: > nit: Don't change this. Using the members explicitly is self-documenting (it's > obvious what each value is). See ToTT "Avoid the Long Parameter List" > https://drive.google.com/a/google.com/file/d/0B-KFDiTrQe9lUER4YkE5MldGM1k/view Removed this constructor. https://codereview.chromium.org/2271483003/diff/60001/ash/test/test_system_tr... File ash/test/test_system_tray_delegate.cc (right): https://codereview.chromium.org/2271483003/diff/60001/ash/test/test_system_tr... ash/test/test_system_tray_delegate.cc:71: ime_list_.clear(); On 2016/08/24 16:03:53, James Cook wrote: > Does ime_list_ = list; work? Done. https://codereview.chromium.org/2271483003/diff/60001/ash/test/test_system_tr... ash/test/test_system_tray_delegate.cc:153: for (size_t i = 0; i < ime_list_.size(); i++) { On 2016/08/24 16:03:53, James Cook wrote: > Does *list = ime_list_; work? Done.
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2271483003/#ps120001 (title: "fix patch failure.")
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_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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...
Message was sent while issue was closed.
Description was changed from ========== Updates the IME list when the IME has refreshed. When the IME refreshes, we need to update the view of the list in IME menu. Saves the created ImeListView and updates it when get IME refreshing event. Add ImeMenuTrayTest.RefreshImeWithListViewCreated test. BUG=640432 TEST=Verified on local build. ========== to ========== Updates the IME list when the IME has refreshed. When the IME refreshes, we need to update the view of the list in IME menu. Saves the created ImeListView and updates it when get IME refreshing event. Add ImeMenuTrayTest.RefreshImeWithListViewCreated test. BUG=640432 TEST=Verified 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 ========== Updates the IME list when the IME has refreshed. When the IME refreshes, we need to update the view of the list in IME menu. Saves the created ImeListView and updates it when get IME refreshing event. Add ImeMenuTrayTest.RefreshImeWithListViewCreated test. BUG=640432 TEST=Verified on local build. ========== to ========== Updates the IME list when the IME has refreshed. When the IME refreshes, we need to update the view of the list in IME menu. Saves the created ImeListView and updates it when get IME refreshing event. Add ImeMenuTrayTest.RefreshImeWithListViewCreated test. BUG=640432 TEST=Verified on local build. Committed: https://crrev.com/c6f28a1effd401fd7e2e03707e0dd84f5346185b Cr-Commit-Position: refs/heads/master@{#414329} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/c6f28a1effd401fd7e2e03707e0dd84f5346185b Cr-Commit-Position: refs/heads/master@{#414329}
Message was sent while issue was closed.
ImeMenuTrayTest.RefreshImeWithListViewCreated started failing after this CL. Would you take a look? https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2281473002/ by johnme@chromium.org. The reason for reverting is: ImeMenuTrayTest.RefreshImeWithListViewCreated is failing consistently on https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2....
Message was sent while issue was closed.
On 2016/08/25 10:59:36, johnme wrote: > A revert of this CL (patchset #7 id:120001) has been created in > https://codereview.chromium.org/2281473002/ by mailto:johnme@chromium.org. > > The reason for reverting is: ImeMenuTrayTest.RefreshImeWithListViewCreated is > failing consistently on > https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2.... You're probably seeing the double-delete here because the new test does not close the menu before it ends. I patched in this CL locally. There is a real double-delete (and crash) if you quit Chrome while the IME tray menu is open. This does not happen with the main system tray menu. I suggest changing the new test to close the menu and relanding this patch. Then I would write a new patch that adds similar test that opens the menu and leaves it open, which should double-delete. Then you can fix the underlying problem. (By the way, how do I get the IME tray button to show up? I tried adding --opt-in-ime-menu but the checkbox did not show up in the language settings UI. I had to patch the code to force the button to be visible.)
Message was sent while issue was closed.
On 2016/08/25 17:57:21, James Cook wrote: > On 2016/08/25 10:59:36, johnme wrote: > > A revert of this CL (patchset #7 id:120001) has been created in > > https://codereview.chromium.org/2281473002/ by mailto:johnme@chromium.org. > > > > The reason for reverting is: ImeMenuTrayTest.RefreshImeWithListViewCreated is > > failing consistently on > > > https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2.... > > You're probably seeing the double-delete here because the new test does not > close the menu before it ends. > > I patched in this CL locally. There is a real double-delete (and crash) if you > quit Chrome while the IME tray menu is open. This does not happen with the main > system tray menu. > > I suggest changing the new test to close the menu and relanding this patch. Then > I would write a new patch that adds similar test that opens the menu and leaves > it open, which should double-delete. Then you can fix the underlying problem. > > (By the way, how do I get the IME tray button to show up? I tried adding > --opt-in-ime-menu but the checkbox did not show up in the language settings UI. > I had to patch the code to force the button to be visible.) Really thanks for the help. I'll add close button in this patch. The run-time flag to enable the feature: --enable-features=OptInImeMenu. It's also shown on about://flags page with desc "Enable opt-in IME menu".
Message was sent while issue was closed.
On 2016/08/25 18:17:10, Azure Wei wrote: > On 2016/08/25 17:57:21, James Cook wrote: > > On 2016/08/25 10:59:36, johnme wrote: > > > A revert of this CL (patchset #7 id:120001) has been created in > > > https://codereview.chromium.org/2281473002/ by mailto:johnme@chromium.org. > > > > > > The reason for reverting is: ImeMenuTrayTest.RefreshImeWithListViewCreated > is > > > failing consistently on > > > > > > https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2.... > > > > You're probably seeing the double-delete here because the new test does not > > close the menu before it ends. > > > > I patched in this CL locally. There is a real double-delete (and crash) if you > > quit Chrome while the IME tray menu is open. This does not happen with the > main > > system tray menu. > > > > I suggest changing the new test to close the menu and relanding this patch. > Then > > I would write a new patch that adds similar test that opens the menu and > leaves > > it open, which should double-delete. Then you can fix the underlying problem. > > > > (By the way, how do I get the IME tray button to show up? I tried adding > > --opt-in-ime-menu but the checkbox did not show up in the language settings > UI. > > I had to patch the code to force the button to be visible.) > > Really thanks for the help. I'll add close button in this patch. > The run-time flag to enable the feature: --enable-features=OptInImeMenu. It's > also shown on about://flags page with desc "Enable opt-in IME menu". You're welcome. You may find it helpful to #include "base/debug/stack_trace.h" and put base::debug::StackTrace().Print(); in the destructor for your class and for SystemTray and compare where they are deleted. I use this technique because it's faster than gdb.
Message was sent while issue was closed.
On 2016/08/25 18:25:17, James Cook wrote: > On 2016/08/25 18:17:10, Azure Wei wrote: > > On 2016/08/25 17:57:21, James Cook wrote: > > > On 2016/08/25 10:59:36, johnme wrote: > > > > A revert of this CL (patchset #7 id:120001) has been created in > > > > https://codereview.chromium.org/2281473002/ by mailto:johnme@chromium.org. > > > > > > > > The reason for reverting is: ImeMenuTrayTest.RefreshImeWithListViewCreated > > is > > > > failing consistently on > > > > > > > > > > https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2.... > > > > > > You're probably seeing the double-delete here because the new test does not > > > close the menu before it ends. > > > > > > I patched in this CL locally. There is a real double-delete (and crash) if > you > > > quit Chrome while the IME tray menu is open. This does not happen with the > > main > > > system tray menu. > > > > > > I suggest changing the new test to close the menu and relanding this patch. > > Then > > > I would write a new patch that adds similar test that opens the menu and > > leaves > > > it open, which should double-delete. Then you can fix the underlying > problem. > > > > > > (By the way, how do I get the IME tray button to show up? I tried adding > > > --opt-in-ime-menu but the checkbox did not show up in the language settings > > UI. > > > I had to patch the code to force the button to be visible.) > > > > Really thanks for the help. I'll add close button in this patch. > > The run-time flag to enable the feature: --enable-features=OptInImeMenu. It's > > also shown on about://flags page with desc "Enable opt-in IME menu". > > You're welcome. You may find it helpful to #include "base/debug/stack_trace.h" > and put base::debug::StackTrace().Print(); in the destructor for your class and > for SystemTray and compare where they are deleted. I use this technique because > it's faster than gdb. I don't know why I can pass the new test (without crash) locally. How to repo the crash on local build?
Message was sent while issue was closed.
On 2016/08/25 18:33:20, Azure Wei wrote: > On 2016/08/25 18:25:17, James Cook wrote: > > On 2016/08/25 18:17:10, Azure Wei wrote: > > > On 2016/08/25 17:57:21, James Cook wrote: > > > > On 2016/08/25 10:59:36, johnme wrote: > > > > > A revert of this CL (patchset #7 id:120001) has been created in > > > > > https://codereview.chromium.org/2281473002/ by > mailto:johnme@chromium.org. > > > > > > > > > > The reason for reverting is: > ImeMenuTrayTest.RefreshImeWithListViewCreated > > > is > > > > > failing consistently on > > > > > > > > > > > > > > > https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2.... > > > > > > > > You're probably seeing the double-delete here because the new test does > not > > > > close the menu before it ends. > > > > > > > > I patched in this CL locally. There is a real double-delete (and crash) if > > you > > > > quit Chrome while the IME tray menu is open. This does not happen with the > > > main > > > > system tray menu. > > > > > > > > I suggest changing the new test to close the menu and relanding this > patch. > > > Then > > > > I would write a new patch that adds similar test that opens the menu and > > > leaves > > > > it open, which should double-delete. Then you can fix the underlying > > problem. > > > > > > > > (By the way, how do I get the IME tray button to show up? I tried adding > > > > --opt-in-ime-menu but the checkbox did not show up in the language > settings > > > UI. > > > > I had to patch the code to force the button to be visible.) > > > > > > Really thanks for the help. I'll add close button in this patch. > > > The run-time flag to enable the feature: --enable-features=OptInImeMenu. > It's > > > also shown on about://flags page with desc "Enable opt-in IME menu". > > > > You're welcome. You may find it helpful to #include "base/debug/stack_trace.h" > > and put base::debug::StackTrace().Print(); in the destructor for your class > and > > for SystemTray and compare where they are deleted. I use this technique > because > > it's faster than gdb. > > I don't know why I can pass the new test (without crash) locally. How to repo > the crash on local build? You might not be able to repro it depending on the memory layout and allocator. I was able to repro the crash when shutting down chrome using a debug build. The simplest thing to do might be to put print statements in the destructor and see how many times it is called. If you need a more powerful tool, you can build the leak-sanitizer build that the bot uses. Instructions at https://www.chromium.org/developers/testing/leaksanitizer - it's just a few GN args.
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2285543002/ by jianli@chromium.org. The reason for reverting is: Caused the following ash_unittest failure: ImeMenuTrayTest.RefreshImeWithListViewCreated https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
