Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(133)

Issue 550883002: Fix SpeechUIModel lifetime issue in Chrome's AppListViewDelegate when switching profiles (Closed)

Created:
6 years, 3 months ago by tapted
Modified:
6 years, 3 months ago
Reviewers:
James Cook, Jun Mukai
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, kalyank, sadrul, ben+ash_chromium.org, tfarina
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Fix SpeechUIModel lifetime issue in Chrome's AppListViewDelegate when switching profiles The AppListView observes the SpeechUIModel directly, so SpeechUIModel must outlive profile changes within Chrome's AppListViewDelegate. Profile changes only recreate the app_list::ContentView and subviews, which updates app_list::AppListModel observers, but not app_list::SpeechUIModel observers. This comes up when switching profiles in the desktop app list. To fix, this change gives SpeechUIModel a default constructor to simplify setting the initial speech recognition state, which needs the profile. Then, decouples the lifetime of the SpeechUIModel from the Profile in Chrome's AppListViewDelegate. BUG=405827 TBR=jamescook@chromium.org Committed: https://crrev.com/966607ad9fc89b31dfc991ab65ba941132e2d2bf Cr-Commit-Position: refs/heads/master@{#293878}

Patch Set 1 #

Patch Set 2 : clear state for SetProfile(NULL) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+24 lines, -21 lines) Patch
M ash/shell/app_list.cc View 1 chunk +1 line, -2 lines 0 comments Download
M athena/home/app_list_view_delegate.cc View 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/ui/app_list/app_list_view_delegate.cc View 1 4 chunks +13 lines, -11 lines 0 comments Download
M ui/app_list/speech_ui_model.h View 1 chunk +2 lines, -1 line 0 comments Download
M ui/app_list/speech_ui_model.cc View 1 chunk +6 lines, -3 lines 0 comments Download
M ui/app_list/test/app_list_test_view_delegate.cc View 1 chunk +1 line, -2 lines 0 comments Download

Messages

Total messages: 12 (5 generated)
tapted
Hi Jun, could you please take a look? There is actually some test coverage coming ...
6 years, 3 months ago (2014-09-08 06:36:53 UTC) #2
Jun Mukai
lgtm
6 years, 3 months ago (2014-09-08 16:33:50 UTC) #3
tapted
+jamescook TBR for ash/shell/app_list.cc OWNERS (simple refactoring change)
6 years, 3 months ago (2014-09-09 00:06:45 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tapted@chromium.org/550883002/20001
6 years, 3 months ago (2014-09-09 00:28:50 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tapted@chromium.org/550883002/20001
6 years, 3 months ago (2014-09-09 05:00:04 UTC) #10
commit-bot: I haz the power
Committed patchset #2 (id:20001) as f4c865d9ebf12146f0ef3906ede05e6d73eb5b34
6 years, 3 months ago (2014-09-09 06:50:25 UTC) #11
commit-bot: I haz the power
6 years, 3 months ago (2014-09-10 03:51:27 UTC) #12
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/966607ad9fc89b31dfc991ab65ba941132e2d2bf
Cr-Commit-Position: refs/heads/master@{#293878}

Powered by Google App Engine
This is Rietveld 408576698