Decouple EventClientImpl and root window. Check containers on the same root window as focused windo…

Decouple EventClientImpl and root window. Check containers on the same root window as focused window when testing if the focused window can receive events.  This was causing lock screen to lose focus when mouse is clicked on another root window.

This Cl also adds a debug build/linux desktop only shortcut (F11) to lock/shutdown.

BUG=152982
TEST=covered by test.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160696

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=160804

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161058

[oshima, 2012-10-06 06:57:36]

http://codereview.chromium.org/11047030/diff/2001/ash/wm/event_client_impl.cc
File ash/wm/event_client_impl.cc (right):

http://codereview.chromium.org/11047030/diff/2001/ash/wm/event_client_impl.cc...
ash/wm/event_client_impl.cc:23:
const_cast<aura::RootWindow*>(window->GetRootWindow()) : NULL;
GetContainer doesn't change the state of root window so const_cast is safe here,
I can change the argument to non const if you prefer that way. Let me know.

[Daniel Erat, 2012-10-06 14:00:18]

http://codereview.chromium.org/11047030/diff/2001/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/11047030/diff/2001/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:770: // -- see
http://crbug.com/146609.
this comment needs to be updated

http://codereview.chromium.org/11047030/diff/2001/ash/wm/event_client_impl.cc
File ash/wm/event_client_impl.cc (right):

http://codereview.chromium.org/11047030/diff/2001/ash/wm/event_client_impl.cc...
ash/wm/event_client_impl.cc:23:
const_cast<aura::RootWindow*>(window->GetRootWindow()) : NULL;
On 2012/10/06 06:57:36, oshima wrote:
> GetContainer doesn't change the state of root window so const_cast is safe
here,
> I can change the argument to non const if you prefer that way. Let me know.

can you just make GetContainer() take a const window, or is that change more
difficult?

[Daniel Erat, 2012-10-06 14:01:20]

http://codereview.chromium.org/11047030/diff/2001/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/11047030/diff/2001/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:766: case POWER_PRESSED:
i think that i'd rather that you made separate LOCK_PRESSED and LOCK_RELEASED
actions here instead of using POWER_PRESSED/_RELEASED

[oshima, 2012-10-06 17:20:58]

http://codereview.chromium.org/11047030/diff/2001/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/11047030/diff/2001/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:766: case POWER_PRESSED:
On 2012/10/06 14:01:20, Daniel Erat wrote:
> i think that i'd rather that you made separate LOCK_PRESSED and LOCK_RELEASED
> actions here instead of using POWER_PRESSED/_RELEASED

I added shutdown as well. I prefer to keep this assigned to
the same key because it's consistent with the device, but
can assign different key if you prefer that way.
Please let me know.

http://codereview.chromium.org/11047030/diff/2001/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:770: // -- see
http://crbug.com/146609.
On 2012/10/06 14:00:19, Daniel Erat wrote:
> this comment needs to be updated

This was actually updated, but probably not clear enough.
I moved & added extra comment.

http://codereview.chromium.org/11047030/diff/2001/ash/wm/event_client_impl.cc
File ash/wm/event_client_impl.cc (right):

http://codereview.chromium.org/11047030/diff/2001/ash/wm/event_client_impl.cc...
ash/wm/event_client_impl.cc:23:
const_cast<aura::RootWindow*>(window->GetRootWindow()) : NULL;
On 2012/10/06 14:00:19, Daniel Erat wrote:
> On 2012/10/06 06:57:36, oshima wrote:
> > GetContainer doesn't change the state of root window so const_cast is safe
> here,
> > I can change the argument to non const if you prefer that way. Let me know.
> 
> can you just make GetContainer() take a const window, or is that change more
> difficult?

It returns non const aura::Window*, so we can't just make it const.
We can add new one though (I added it in new patch)
Please let me know what you think.

[Daniel Erat, 2012-10-07 13:40:02]

http://codereview.chromium.org/11047030/diff/2001/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/11047030/diff/2001/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:766: case POWER_PRESSED:
On 2012/10/06 17:20:58, oshima wrote:
> On 2012/10/06 14:01:20, Daniel Erat wrote:
> > i think that i'd rather that you made separate LOCK_PRESSED and
LOCK_RELEASED
> > actions here instead of using POWER_PRESSED/_RELEASED
> 
> I added shutdown as well. I prefer to keep this assigned to
> the same key because it's consistent with the device, but
> can assign different key if you prefer that way.
> Please let me know.

I don't understand the "consistent with the device" comment.  On devices, we
have different keys for the power button and the lock button.  Neither of them
goes through this path; we get D-Bus signals from powerm when either key is
pressed or released.

What I'd suggesting is something like:

- add LOCK_PRESSED and LOCK_RELEASED, make them call LockButtonEvent, and make
them get triggered by F11 in debug builds
- make POWER_PRESSED and POWER_RELEASED just call OnPowerButtonEvent, and make
them get triggered by F12 in debug builds

Calling either OnPowerButtonEvent or OnLockButtonEvent for the same key isn't
consistent with how the real code works -- the real code either locks or shuts
down in response to power button presses depending on whether we're not locked
or are locked, and only locks in response to lock button presses.

[oshima, 2012-10-07 16:54:09]

On Sun, Oct 7, 2012 at 6:40 AM, <derat@chromium.org> wrote:

>
> http://codereview.chromium.**org/11047030/diff/2001/ash/**
>
accelerators/accelerator_**controller.cc<http://codereview.chromium.org/11047030/diff/2001/ash/accelerators/accelerator_controller.cc>
> File ash/accelerators/accelerator_**controller.cc (right):
>
> http://codereview.chromium.**org/11047030/diff/2001/ash/**
>
accelerators/accelerator_**controller.cc#newcode766<http://codereview.chromium.org/11047030/diff/2001/ash/accelerators/accelerator_controller.cc#newcode766>
> ash/accelerators/accelerator_**controller.cc:766: case POWER_PRESSED:
> On 2012/10/06 17:20:58, oshima wrote:
>
>> On 2012/10/06 14:01:20, Daniel Erat wrote:
>> > i think that i'd rather that you made separate LOCK_PRESSED and
>>
> LOCK_RELEASED
>
>> > actions here instead of using POWER_PRESSED/_RELEASED
>>
>
>  I added shutdown as well. I prefer to keep this assigned to
>> the same key because it's consistent with the device, but
>> can assign different key if you prefer that way.
>> Please let me know.
>>
>
> I don't understand the "consistent with the device" comment.  On
> devices, we have different keys for the power button and the lock
> button.


Um, we don't have lock button on the device, do we? Sorry if I'm missing
something...

- oshima


>  Neither of them goes through this path; we get D-Bus signals
> from powerm when either key is pressed or released.
>
> What I'd suggesting is something like:
>
> - add LOCK_PRESSED and LOCK_RELEASED, make them call LockButtonEvent,
> and make them get triggered by F11 in debug builds
> - make POWER_PRESSED and POWER_RELEASED just call OnPowerButtonEvent,
> and make them get triggered by F12 in debug builds
>
> Calling either OnPowerButtonEvent or OnLockButtonEvent for the same key
> isn't consistent with how the real code works -- the real code either
> locks or shuts down in response to power button presses depending on
> whether we're not locked or are locked, and only locks in response to
> lock button presses.
>
>
http://codereview.chromium.**org/11047030/<http://codereview.chromium.org/110...
>

[Daniel Erat, 2012-10-07 21:28:50]

On Sun, Oct 7, 2012 at 9:53 AM, oshima <oshima@chromium.org> wrote:
>
>
> On Sun, Oct 7, 2012 at 6:40 AM, <derat@chromium.org> wrote:
>>
>>
>>
>>
http://codereview.chromium.org/11047030/diff/2001/ash/accelerators/accelerato...
>> File ash/accelerators/accelerator_controller.cc (right):
>>
>>
>>
http://codereview.chromium.org/11047030/diff/2001/ash/accelerators/accelerato...
>> ash/accelerators/accelerator_controller.cc:766: case POWER_PRESSED:
>> On 2012/10/06 17:20:58, oshima wrote:
>>>
>>> On 2012/10/06 14:01:20, Daniel Erat wrote:
>>> > i think that i'd rather that you made separate LOCK_PRESSED and
>>
>> LOCK_RELEASED
>>>
>>> > actions here instead of using POWER_PRESSED/_RELEASED
>>
>>
>>> I added shutdown as well. I prefer to keep this assigned to
>>> the same key because it's consistent with the device, but
>>> can assign different key if you prefer that way.
>>> Please let me know.
>>
>>
>> I don't understand the "consistent with the device" comment.  On
>> devices, we have different keys for the power button and the lock
>> button.
>
>
> Um, we don't have lock button on the device, do we? Sorry if I'm missing
> something...

Some devices and keyboards have dedicated lock keys in addition to
power buttons; that's why PowerButtonController has methods for both
lock and power buttons.  We use the F13 scancode for lock.

> - oshima
>
>>
>>  Neither of them goes through this path; we get D-Bus signals
>> from powerm when either key is pressed or released.
>>
>> What I'd suggesting is something like:
>>
>> - add LOCK_PRESSED and LOCK_RELEASED, make them call LockButtonEvent,
>> and make them get triggered by F11 in debug builds
>> - make POWER_PRESSED and POWER_RELEASED just call OnPowerButtonEvent,
>> and make them get triggered by F12 in debug builds
>>
>> Calling either OnPowerButtonEvent or OnLockButtonEvent for the same key
>> isn't consistent with how the real code works -- the real code either
>> locks or shuts down in response to power button presses depending on
>> whether we're not locked or are locked, and only locks in response to
>> lock button presses.
>>
>> http://codereview.chromium.org/11047030/
>
>

[oshima, 2012-10-07 22:39:31]

On 2012/10/07 21:28:50, Daniel Erat wrote:
> On Sun, Oct 7, 2012 at 9:53 AM, oshima <mailto:oshima@chromium.org> wrote:
> >
> >
> > On Sun, Oct 7, 2012 at 6:40 AM, <mailto:derat@chromium.org> wrote:
> >>
> >>
> >>
> >>
>
http://codereview.chromium.org/11047030/diff/2001/ash/accelerators/accelerato...
> >> File ash/accelerators/accelerator_controller.cc (right):
> >>
> >>
> >>
>
http://codereview.chromium.org/11047030/diff/2001/ash/accelerators/accelerato...
> >> ash/accelerators/accelerator_controller.cc:766: case POWER_PRESSED:
> >> On 2012/10/06 17:20:58, oshima wrote:
> >>>
> >>> On 2012/10/06 14:01:20, Daniel Erat wrote:
> >>> > i think that i'd rather that you made separate LOCK_PRESSED and
> >>
> >> LOCK_RELEASED
> >>>
> >>> > actions here instead of using POWER_PRESSED/_RELEASED
> >>
> >>
> >>> I added shutdown as well. I prefer to keep this assigned to
> >>> the same key because it's consistent with the device, but
> >>> can assign different key if you prefer that way.
> >>> Please let me know.
> >>
> >>
> >> I don't understand the "consistent with the device" comment.  On
> >> devices, we have different keys for the power button and the lock
> >> button.
> >
> >
> > Um, we don't have lock button on the device, do we? Sorry if I'm missing
> > something...
> 
> Some devices and keyboards have dedicated lock keys in addition to
> power buttons; that's why PowerButtonController has methods for both
> lock and power buttons.  We use the F13 scancode for lock.

Ah I see. That's good to know. Looks like PowerButton{Observer|Controller}
are handing both power button and lock button, and I was confused by that.

> 
> > - oshima
> >
> >>
> >>  Neither of them goes through this path; we get D-Bus signals
> >> from powerm when either key is pressed or released.
> >>
> >> What I'd suggesting is something like:
> >>
> >> - add LOCK_PRESSED and LOCK_RELEASED, make them call LockButtonEvent,
> >> and make them get triggered by F11 in debug builds
> >> - make POWER_PRESSED and POWER_RELEASED just call OnPowerButtonEvent,
> >> and make them get triggered by F12 in debug builds
> >>
> >> Calling either OnPowerButtonEvent or OnLockButtonEvent for the same key
> >> isn't consistent with how the real code works -- the real code either
> >> locks or shuts down in response to power button presses depending on
> >> whether we're not locked or are locked, and only locks in response to
> >> lock button presses.
> >>
> >> http://codereview.chromium.org/11047030/
> >
> >

[oshima, 2012-10-08 07:05:58]

uploaded new patch. PTAL.

[Daniel Erat, 2012-10-08 14:41:51]

lgtm

http://codereview.chromium.org/11047030/diff/9015/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/11047030/diff/9015/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:776: // We don't do anything with
these at present on the device, but
Do you mind changing this to:

// We don't do anything with these at present on the device
// (power button events are reported to us from powerm via
// D-Bus), but ...

?

http://codereview.chromium.org/11047030/diff/9015/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/11047030/diff/9015/ash/shell.cc#newcode352
ash/shell.cc:352: const aura::Window* Shell::GetContainer(const
aura::RootWindow* root_window,
i think there was a misunderstanding earlier.  why can't you have a single
method with this signature?

aura::Window* Shell::GetContainer(const aura::RootWindow* root_window,
                                  int container_id);

it should be fine to pass a non-const root window to this method, no?

http://codereview.chromium.org/11047030/diff/9015/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/11047030/diff/9015/ash/shell.h#newcode180
ash/shell.h:180: int container_id);
nit: fix indenting

http://codereview.chromium.org/11047030/diff/9015/ash/wm/power_button_control...
File ash/wm/power_button_controller.cc (right):

http://codereview.chromium.org/11047030/diff/9015/ash/wm/power_button_control...
ash/wm/power_button_controller.cc:314: #if defined(OS_CHROMEOS)
this code seems a bit strange.  you have an OS_CHROMEOS ifdef, but i think that
this class is completely unsafe to use on non-chrome-os builds, since i believe
that delegate_ will be null there (at least, the only
PowerButtonControllerDelegate implementation that i see is
PowerButtonControllerDelegateChromeos).

a better long-term solution might be a stub PowerButtonControllerDelegate that
we use on windows and chrome os builds when not running on devices that just
exits in response to RequestShutdown() calls.

(doesn't need to happen in this change, though.)

http://codereview.chromium.org/11047030/diff/9015/ash/wm/power_button_control...
ash/wm/power_button_controller.cc:317: if (delegate)
random question: under what circumstances do we not create a ShellDelegate?

[oshima, 2012-10-08 17:52:59]

http://codereview.chromium.org/11047030/diff/9015/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/11047030/diff/9015/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:776: // We don't do anything with
these at present on the device, but
On 2012/10/08 14:41:51, Daniel Erat wrote:
> Do you mind changing this to:
> 
> // We don't do anything with these at present on the device
> // (power button events are reported to us from powerm via
> // D-Bus), but ...
> 
> ?

Done.

http://codereview.chromium.org/11047030/diff/9015/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/11047030/diff/9015/ash/shell.cc#newcode352
ash/shell.cc:352: const aura::Window* Shell::GetContainer(const
aura::RootWindow* root_window,
On 2012/10/08 14:41:51, Daniel Erat wrote:
> i think there was a misunderstanding earlier.  why can't you have a single
> method with this signature?
> 
> aura::Window* Shell::GetContainer(const aura::RootWindow* root_window,
>                                   int container_id);
> 
> it should be fine to pass a non-const root window to this method, no?

You can't unless you do const_cast in the implementation, which makes
const less useful. In UI code (AFAIK), we don't allow a const method that
returns non const objects that are part of the internal state. That's why
we have two, const and non const versino of GetChildById methods
(same for transient_parent and other places))

http://codereview.chromium.org/11047030/diff/9015/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/11047030/diff/9015/ash/shell.h#newcode180
ash/shell.h:180: int container_id);
On 2012/10/08 14:41:51, Daniel Erat wrote:
> nit: fix indenting

Done.

http://codereview.chromium.org/11047030/diff/9015/ash/wm/power_button_control...
File ash/wm/power_button_controller.cc (right):

http://codereview.chromium.org/11047030/diff/9015/ash/wm/power_button_control...
ash/wm/power_button_controller.cc:314: #if defined(OS_CHROMEOS)
On 2012/10/08 14:41:51, Daniel Erat wrote:
> this code seems a bit strange.  you have an OS_CHROMEOS ifdef, but i think
that
> this class is completely unsafe to use on non-chrome-os builds, since i
believe
> that delegate_ will be null there (at least, the only
> PowerButtonControllerDelegate implementation that i see is
> PowerButtonControllerDelegateChromeos).
> 
> a better long-term solution might be a stub PowerButtonControllerDelegate that
> we use on windows and chrome os builds when not running on devices that just
> exits in response to RequestShutdown() calls.
> 
> (doesn't need to happen in this change, though.)

Chances are this class may not make sense at all on windows (is it possible to
intercept power button on windows?), although I'm not 100% sure. We may still
want
to keep it for debugging though.

http://codereview.chromium.org/11047030/diff/9015/ash/wm/power_button_control...
ash/wm/power_button_controller.cc:317: if (delegate)
On 2012/10/08 14:41:51, Daniel Erat wrote:
> random question: under what circumstances do we not create a ShellDelegate?

There are a few unittests that create Shell with NULL delegates. 
It's unlikely for these tests to hit this code, but other places in ash
assumes this can be NULL, so I'd like to do the same.

[Daniel Erat, 2012-10-08 18:24:25]

lgtm

http://codereview.chromium.org/11047030/diff/9015/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/11047030/diff/9015/ash/shell.cc#newcode352
ash/shell.cc:352: const aura::Window* Shell::GetContainer(const
aura::RootWindow* root_window,
On 2012/10/08 17:52:59, oshima wrote:
> On 2012/10/08 14:41:51, Daniel Erat wrote:
> > i think there was a misunderstanding earlier.  why can't you have a single
> > method with this signature?
> > 
> > aura::Window* Shell::GetContainer(const aura::RootWindow* root_window,
> >                                   int container_id);
> > 
> > it should be fine to pass a non-const root window to this method, no?
> 
> You can't unless you do const_cast in the implementation, which makes
> const less useful. In UI code (AFAIK), we don't allow a const method that
> returns non const objects that are part of the internal state. That's why
> we have two, const and non const versino of GetChildById methods
> (same for transient_parent and other places))

Thanks, I missed that you were invoking a const method on the RootWindow that
returns a const Window.

http://codereview.chromium.org/11047030/diff/9015/ash/wm/power_button_control...
File ash/wm/power_button_controller.cc (right):

http://codereview.chromium.org/11047030/diff/9015/ash/wm/power_button_control...
ash/wm/power_button_controller.cc:314: #if defined(OS_CHROMEOS)
On 2012/10/08 17:52:59, oshima wrote:
> On 2012/10/08 14:41:51, Daniel Erat wrote:
> > this code seems a bit strange.  you have an OS_CHROMEOS ifdef, but i think
> that
> > this class is completely unsafe to use on non-chrome-os builds, since i
> believe
> > that delegate_ will be null there (at least, the only
> > PowerButtonControllerDelegate implementation that i see is
> > PowerButtonControllerDelegateChromeos).
> > 
> > a better long-term solution might be a stub PowerButtonControllerDelegate
that
> > we use on windows and chrome os builds when not running on devices that just
> > exits in response to RequestShutdown() calls.
> > 
> > (doesn't need to happen in this change, though.)
> 
> Chances are this class may not make sense at all on windows (is it possible to
> intercept power button on windows?), although I'm not 100% sure. We may still
> want
> to keep it for debugging though.
> 

Yeah, I was just suggesting this for debugging.

[oshima, 2012-10-08 18:31:07]

On 2012/10/08 18:24:25, Daniel Erat wrote:
> lgtm
> 
> http://codereview.chromium.org/11047030/diff/9015/ash/shell.cc
> File ash/shell.cc (right):
> 
> http://codereview.chromium.org/11047030/diff/9015/ash/shell.cc#newcode352
> ash/shell.cc:352: const aura::Window* Shell::GetContainer(const
> aura::RootWindow* root_window,
> On 2012/10/08 17:52:59, oshima wrote:
> > On 2012/10/08 14:41:51, Daniel Erat wrote:
> > > i think there was a misunderstanding earlier.  why can't you have a single
> > > method with this signature?
> > > 
> > > aura::Window* Shell::GetContainer(const aura::RootWindow* root_window,
> > >                                   int container_id);
> > > 
> > > it should be fine to pass a non-const root window to this method, no?
> > 
> > You can't unless you do const_cast in the implementation, which makes
> > const less useful. In UI code (AFAIK), we don't allow a const method that
> > returns non const objects that are part of the internal state. That's why
> > we have two, const and non const versino of GetChildById methods
> > (same for transient_parent and other places))
> 
> Thanks, I missed that you were invoking a const method on the RootWindow that
> returns a const Window.
> 
>
http://codereview.chromium.org/11047030/diff/9015/ash/wm/power_button_control...
> File ash/wm/power_button_controller.cc (right):
> 
>
http://codereview.chromium.org/11047030/diff/9015/ash/wm/power_button_control...
> ash/wm/power_button_controller.cc:314: #if defined(OS_CHROMEOS)
> On 2012/10/08 17:52:59, oshima wrote:
> > On 2012/10/08 14:41:51, Daniel Erat wrote:
> > > this code seems a bit strange.  you have an OS_CHROMEOS ifdef, but i think
> > that
> > > this class is completely unsafe to use on non-chrome-os builds, since i
> > believe
> > > that delegate_ will be null there (at least, the only
> > > PowerButtonControllerDelegate implementation that i see is
> > > PowerButtonControllerDelegateChromeos).
> > > 
> > > a better long-term solution might be a stub PowerButtonControllerDelegate
> that
> > > we use on windows and chrome os builds when not running on devices that
just
> > > exits in response to RequestShutdown() calls.
> > > 
> > > (doesn't need to happen in this change, though.)
> > 
> > Chances are this class may not make sense at all on windows (is it possible
to
> > intercept power button on windows?), although I'm not 100% sure. We may
still
> > want
> > to keep it for debugging though.
> > 
> 
> Yeah, I was just suggesting this for debugging.

SGTM

[commit-bot: I haz the power, 2012-10-08 18:31:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/11047030/18001

[commit-bot: I haz the power, 2012-10-08 20:41:54]

Change committed as 160696

[oshima, 2012-10-08 23:54:32]

On 2012/10/08 20:41:54, I haz the power (commit-bot) wrote:
> Change committed as 160696

Looks like F12 is already used for dev tools.
I changed lock to F11+SHIFT (F11+CONTROL was taken too)

[oshima, 2012-10-09 23:35:24]

Scott, can you review ui/aura changes? This is necessary to get
EventGenerator::KeyPress/Release working on Windows.

win_aura bot is busted right now, but I tested this locally and
aura_unittests/ash_unittests are passed.

[sky, 2012-10-10 03:28:07]

LGTM

[commit-bot: I haz the power, 2012-10-10 03:34:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/11047030/18003

[commit-bot: I haz the power, 2012-10-10 05:52:39]

Change committed as 161058