Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2165773002:
[Chromecast] Fix crash on keypress in cast_shell with Ozone X11 (Closed)
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} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
