|
|
Created:
5 years, 3 months ago by achuithb Modified:
5 years, 3 months ago Reviewers:
oshima CC:
chromium-reviews, oshima+watch_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. |
DescriptionAccessibilityManager must be deleted after ash Shell, but before InputMethodManager.
session_state_observer_ needs to be deleted before the Shell goes away, so we do it in OnAppTerminating.
BUG=chromium:515097
Committed: https://crrev.com/ecf113f67c8ff43e8eddeb2585c98bbc31ed79e2
Cr-Commit-Position: refs/heads/master@{#348130}
Patch Set 1 #Patch Set 2 : SessionStateDelegate #
Total comments: 2
Patch Set 3 : OnAppTerminating #
Total comments: 3
Patch Set 4 : RemoveShellObserver #
Messages
Total messages: 43 (17 generated)
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1316793004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1316793004/1
achuith@chromium.org changed reviewers: + oshima@chromium.org
This fixes the shutdown ordering.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
yep, this is the right way to fix. lgtm
looks like you have to reset the session state observer before shell is destroyed.
On 2015/09/03 16:40:43, oshima wrote: > looks like you have to reset the session state observer before shell is > destroyed. Yup, I'll fix it tomorrow; it's already pretty late here. Thank you for the review!
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1316793004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1316793004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1316793004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1316793004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1316793004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1316793004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Finally got it to not crash. Oshima-san, PTAL.
https://codereview.chromium.org/1316793004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right): https://codereview.chromium.org/1316793004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_manager.cc:946: void AccessibilityManager::SessionEnded() { Is it possible to use ShellObserver::OnAppTerminating to reset the observer instead?
PTAL, Oshima-san https://codereview.chromium.org/1316793004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right): https://codereview.chromium.org/1316793004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_manager.cc:946: void AccessibilityManager::SessionEnded() { On 2015/09/08 21:06:03, oshima wrote: > Is it possible to use ShellObserver::OnAppTerminating to reset the observer > instead? Done. https://codereview.chromium.org/1316793004/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right): https://codereview.chromium.org/1316793004/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:67: ash::Shell::GetInstance()->AddShellObserver( I can't put this in AccessibilityManager ctor because Shell hasn't been created yet.
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1316793004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1316793004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with one nit https://codereview.chromium.org/1316793004/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right): https://codereview.chromium.org/1316793004/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:70: ~AccessibilityDelegateImpl() override {} can you remove the observer here?
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1316793004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1316793004/100001
https://codereview.chromium.org/1316793004/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right): https://codereview.chromium.org/1316793004/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:70: ~AccessibilityDelegateImpl() override {} On 2015/09/09 19:35:06, oshima wrote: > can you remove the observer here? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by achuith@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/1316793004/#ps100001 (title: "RemoveShellObserver")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1316793004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1316793004/100001
On 2015/09/09 19:35:06, oshima wrote: > lgtm with one nit > ty
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ecf113f67c8ff43e8eddeb2585c98bbc31ed79e2 Cr-Commit-Position: refs/heads/master@{#348130}
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ecf113f67c8ff43e8eddeb2585c98bbc31ed79e2 Cr-Commit-Position: refs/heads/master@{#348130} |