|
|
DescriptionFix crash on keypress when running cast_shell in Ozone X11 platform
BUG=629555
Committed: https://crrev.com/a6fdeeb44c7dfdea50b34e717b3bb304c370f161
Cr-Commit-Position: refs/heads/master@{#406872}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 15 (5 generated)
halliwell@chromium.org changed reviewers: + kylechar@chromium.org, sadrul@chromium.org
I'm not sure if this addresses the root cause of the bug ... but it does stop the crash ... let me know what you think :)
https://codereview.chromium.org/2165773002/diff/1/ui/aura/window_targeter.cc File ui/aura/window_targeter.cc (right): https://codereview.chromium.org/2165773002/diff/1/ui/aura/window_targeter.cc#... ui/aura/window_targeter.cc:111: return window; Should this return nullptr?
https://codereview.chromium.org/2165773002/diff/1/ui/aura/window_targeter.cc File ui/aura/window_targeter.cc (right): https://codereview.chromium.org/2165773002/diff/1/ui/aura/window_targeter.cc#... ui/aura/window_targeter.cc:111: return window; On 2016/07/20 16:58:36, sadrul wrote: > Should this return nullptr? tbh I don't understand this code well. But I was going by the line below, where if the focus_client fails to GetFocusedWindow, this function will return 'window'.
ok. lgtm (although, have you considered actually installing a focus-client instead?) Update the CL description to mention cast_shell, because chrome, content_shell etc. in ozone-x11 do not have this crash
Description was changed from ========== Fix crash on keypress in Ozone X11 platform BUG=629555 ========== to ========== Fix crash on keypress when running cast_shell in Ozone X11 platform BUG=629555 ==========
On 2016/07/21 14:50:26, sadrul wrote: > ok. lgtm (although, have you considered actually installing a focus-client > instead?) Update the CL description to mention cast_shell, because chrome, > content_shell etc. in ozone-x11 do not have this crash Description updated. I had a quick look at installing a focus client, I'd be ok with it if it was minimal code to do so - since input and focus are not features of cast_shell on its primary platform (TV). content_shell uses TestFocusClient, which looks suitably minimal, but that's in a test-only target, which makes it illegal to depend on from where we'd need it. So I guess if TestFocusClient were refactored out into a non-test target ('Simple' or 'Basic' FocusClient?) then we could do that. Let me know if you'd prefer that approach, I'll hold off briefly on submitting this.
On 2016/07/21 15:33:57, halliwell wrote: > On 2016/07/21 14:50:26, sadrul wrote: > > ok. lgtm (although, have you considered actually installing a focus-client > > instead?) Update the CL description to mention cast_shell, because chrome, > > content_shell etc. in ozone-x11 do not have this crash > > Description updated. > > I had a quick look at installing a focus client, I'd be ok with it if it was > minimal code to do so - since input and focus are not features of cast_shell on > its primary platform (TV). content_shell uses TestFocusClient, which looks > suitably minimal, but that's in a test-only target, which makes it illegal to > depend on from where we'd need it. > > So I guess if TestFocusClient were refactored out into a non-test target > ('Simple' or 'Basic' FocusClient?) then we could do that. > > Let me know if you'd prefer that approach, I'll hold off briefly on submitting > this. You can go ahead and land this change for now. We can install a focus-client if/when cast-shell needs keyboard input at some point (or wants to deal with focus).
On 2016/07/21 15:36:21, sadrul wrote: > On 2016/07/21 15:33:57, halliwell wrote: > > On 2016/07/21 14:50:26, sadrul wrote: > > > ok. lgtm (although, have you considered actually installing a focus-client > > > instead?) Update the CL description to mention cast_shell, because chrome, > > > content_shell etc. in ozone-x11 do not have this crash > > > > Description updated. > > > > I had a quick look at installing a focus client, I'd be ok with it if it was > > minimal code to do so - since input and focus are not features of cast_shell > on > > its primary platform (TV). content_shell uses TestFocusClient, which looks > > suitably minimal, but that's in a test-only target, which makes it illegal to > > depend on from where we'd need it. > > > > So I guess if TestFocusClient were refactored out into a non-test target > > ('Simple' or 'Basic' FocusClient?) then we could do that. > > > > Let me know if you'd prefer that approach, I'll hold off briefly on submitting > > this. > > You can go ahead and land this change for now. We can install a focus-client > if/when cast-shell needs keyboard input at some point (or wants to deal with > focus). Ok, sgtm
The CQ bit was checked by halliwell@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 ========== Fix crash on keypress when running cast_shell in Ozone X11 platform BUG=629555 ========== to ========== Fix crash on keypress when running cast_shell in Ozone X11 platform BUG=629555 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fix crash on keypress when running cast_shell in Ozone X11 platform BUG=629555 ========== to ========== Fix crash on keypress when running cast_shell in Ozone X11 platform BUG=629555 Committed: https://crrev.com/a6fdeeb44c7dfdea50b34e717b3bb304c370f161 Cr-Commit-Position: refs/heads/master@{#406872} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/a6fdeeb44c7dfdea50b34e717b3bb304c370f161 Cr-Commit-Position: refs/heads/master@{#406872} |