Don't check ash accelerators in browser with mash.

Don't check ash accelerators in browser with mash.

The chrome browser is crashing when chrome::IsAcceleratorDeprecated() is
called when running mash. This is due to ash::Shell not existing in the
browser process.

The chrome browser process shouldn't be handling ash accelerators in
mash, it should be done by the mus process. Bail out early from the
method if running in mash instead.

BUG=605218
Committed: https://crrev.com/5d7c79e257fecde79e9a5f98621a68f283c52198
Cr-Commit-Position: refs/heads/master@{#388807}

[kylechar, 2016-04-20 20:11:04]

kylechar@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2016-04-20 20:26:49]

https://codereview.chromium.org/1902393003/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1902393003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:1377: if
(chrome::IsRunningInMash())
This seems too early. It's only ash accelerators we don't want to hit, not all
accelerators.

[kylechar, 2016-04-20 20:35:59]

On 2016/04/20 20:26:49, sky wrote:
>
https://codereview.chromium.org/1902393003/diff/1/chrome/browser/ui/views/fra...
> File chrome/browser/ui/views/frame/browser_view.cc (right):
> 
>
https://codereview.chromium.org/1902393003/diff/1/chrome/browser/ui/views/fra...
> chrome/browser/ui/views/frame/browser_view.cc:1377: if
> (chrome::IsRunningInMash())
> This seems too early. It's only ash accelerators we don't want to hit, not all
> accelerators.

Yeah, I'm not exactly sure how accelerators here need to be handled. So if I
understand correctly we still want to handle things like ctrl+t (new tab) in the
browser process?

The easy thing to do is move the check so that it's something like:

if (!chrome::IsRunningInMash() && chrome::IsAcceleratorDeprecated(accelerator))
{

The call to chrome::IsAcceleratorDeprecated is the only problematic bit so it
will fix the crash. Ash accelerators would then be registered in mus instead of
in the browser and things should just work.

[sky, 2016-04-20 20:37:47]

How about moving the conditional to IsAcceleratorDeprecated then?

  -Scott

On Wed, Apr 20, 2016 at 1:35 PM,  <kylechar@chromium.org> wrote:
> On 2016/04/20 20:26:49, sky wrote:
>>
>
https://codereview.chromium.org/1902393003/diff/1/chrome/browser/ui/views/fra...
>> File chrome/browser/ui/views/frame/browser_view.cc (right):
>>
>>
>
https://codereview.chromium.org/1902393003/diff/1/chrome/browser/ui/views/fra...
>> chrome/browser/ui/views/frame/browser_view.cc:1377: if
>> (chrome::IsRunningInMash())
>> This seems too early. It's only ash accelerators we don't want to hit, not
>> all
>> accelerators.
>
> Yeah, I'm not exactly sure how accelerators here need to be handled. So if I
> understand correctly we still want to handle things like ctrl+t (new tab) in
> the
> browser process?
>
> The easy thing to do is move the check so that it's something like:
>
> if (!chrome::IsRunningInMash() &&
> chrome::IsAcceleratorDeprecated(accelerator))
> {
>
> The call to chrome::IsAcceleratorDeprecated is the only problematic bit so
> it
> will fix the crash. Ash accelerators would then be registered in mus instead
> of
> in the browser and things should just work.
>
> https://codereview.chromium.org/1902393003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kylechar, 2016-04-21 12:53:51]

On 2016/04/20 20:37:47, sky wrote:
> How about moving the conditional to IsAcceleratorDeprecated then?
> 
>   -Scott
> 
> On Wed, Apr 20, 2016 at 1:35 PM,  <mailto:kylechar@chromium.org> wrote:
> > On 2016/04/20 20:26:49, sky wrote:
> >>
> >
>
https://codereview.chromium.org/1902393003/diff/1/chrome/browser/ui/views/fra...
> >> File chrome/browser/ui/views/frame/browser_view.cc (right):
> >>
> >>
> >
>
https://codereview.chromium.org/1902393003/diff/1/chrome/browser/ui/views/fra...
> >> chrome/browser/ui/views/frame/browser_view.cc:1377: if
> >> (chrome::IsRunningInMash())
> >> This seems too early. It's only ash accelerators we don't want to hit, not
> >> all
> >> accelerators.
> >
> > Yeah, I'm not exactly sure how accelerators here need to be handled. So if I
> > understand correctly we still want to handle things like ctrl+t (new tab) in
> > the
> > browser process?
> >
> > The easy thing to do is move the check so that it's something like:
> >
> > if (!chrome::IsRunningInMash() &&
> > chrome::IsAcceleratorDeprecated(accelerator))
> > {
> >
> > The call to chrome::IsAcceleratorDeprecated is the only problematic bit so
> > it
> > will fix the crash. Ash accelerators would then be registered in mus instead
> > of
> > in the browser and things should just work.
> >
> > https://codereview.chromium.org/1902393003/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

That sounds fine to me. Done.

[kylechar, 2016-04-21 16:27:27]

Description was changed from

==========
Bail out early when checking pre-accelerators in mash.

The chrome browser is crashing when chrome::IsAcceleratorDeprecated() is
called when running mash. This is due to ash::Shell not existing in the
browser process.

The chrome browser process shouldn't be handling these accelerators in
mash, it should be done by the mus process. Bail out early from the
method if running in mash instead.

BUG=605218
==========

to

==========
Don't check ash accelerators in broswer with mash.

The chrome browser is crashing when chrome::IsAcceleratorDeprecated() is
called when running mash. This is due to ash::Shell not existing in the
browser process.

The chrome browser process shouldn't be handling ash accelerators in
mash, it should be done by the mus process. Bail out early from the
method if running in mash instead.

BUG=605218
==========

[kylechar, 2016-04-21 16:27:35]

Description was changed from

==========
Don't check ash accelerators in broswer with mash.

The chrome browser is crashing when chrome::IsAcceleratorDeprecated() is
called when running mash. This is due to ash::Shell not existing in the
browser process.

The chrome browser process shouldn't be handling ash accelerators in
mash, it should be done by the mus process. Bail out early from the
method if running in mash instead.

BUG=605218
==========

to

==========
Don't check ash accelerators in broswer with mash.

The chrome browser is crashing when chrome::IsAcceleratorDeprecated() is
called when running mash. This is due to ash::Shell not existing in the
browser process.

The chrome browser process shouldn't be handling ash accelerators in
mash, it should be done by the mus process. Bail out early from the
method if running in mash instead.

BUG=605218
==========

[kylechar, 2016-04-21 16:27:58]

Description was changed from

==========
Don't check ash accelerators in broswer with mash.

The chrome browser is crashing when chrome::IsAcceleratorDeprecated() is
called when running mash. This is due to ash::Shell not existing in the
browser process.

The chrome browser process shouldn't be handling ash accelerators in
mash, it should be done by the mus process. Bail out early from the
method if running in mash instead.

BUG=605218
==========

to

==========
Don't check ash accelerators in browser with mash.

The chrome browser is crashing when chrome::IsAcceleratorDeprecated() is
called when running mash. This is due to ash::Shell not existing in the
browser process.

The chrome browser process shouldn't be handling ash accelerators in
mash, it should be done by the mus process. Bail out early from the
method if running in mash instead.

BUG=605218
==========

[sky, 2016-04-21 16:48:48]

LGTM

[kylechar, 2016-04-21 16:54:52]

The CQ bit was checked by kylechar@chromium.org

[commit-bot: I haz the power, 2016-04-21 16:55:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1902393003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1902393003/20001

[commit-bot: I haz the power, 2016-04-21 17:43:41]

Description was changed from

==========
Don't check ash accelerators in browser with mash.

The chrome browser is crashing when chrome::IsAcceleratorDeprecated() is
called when running mash. This is due to ash::Shell not existing in the
browser process.

The chrome browser process shouldn't be handling ash accelerators in
mash, it should be done by the mus process. Bail out early from the
method if running in mash instead.

BUG=605218
==========

to

==========
Don't check ash accelerators in browser with mash.

The chrome browser is crashing when chrome::IsAcceleratorDeprecated() is
called when running mash. This is due to ash::Shell not existing in the
browser process.

The chrome browser process shouldn't be handling ash accelerators in
mash, it should be done by the mus process. Bail out early from the
method if running in mash instead.

BUG=605218
==========

[commit-bot: I haz the power, 2016-04-21 17:43:42]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-22 19:35:54]

Description was changed from

==========
Don't check ash accelerators in browser with mash.

The chrome browser is crashing when chrome::IsAcceleratorDeprecated() is
called when running mash. This is due to ash::Shell not existing in the
browser process.

The chrome browser process shouldn't be handling ash accelerators in
mash, it should be done by the mus process. Bail out early from the
method if running in mash instead.

BUG=605218
==========

to

==========
Don't check ash accelerators in browser with mash.

The chrome browser is crashing when chrome::IsAcceleratorDeprecated() is
called when running mash. This is due to ash::Shell not existing in the
browser process.

The chrome browser process shouldn't be handling ash accelerators in
mash, it should be done by the mus process. Bail out early from the
method if running in mash instead.

BUG=605218

Committed: https://crrev.com/5d7c79e257fecde79e9a5f98621a68f283c52198
Cr-Commit-Position: refs/heads/master@{#388807}
==========

[commit-bot: I haz the power, 2016-04-22 19:35:55]

Patchset 2 (id:??) landed as
https://crrev.com/5d7c79e257fecde79e9a5f98621a68f283c52198
Cr-Commit-Position: refs/heads/master@{#388807}