Deprecating high-conflict accelerators

Deprecating high-conflict accelerators

This CL designs a framework for deprecated accelerators.
In this CL we deprecate the following accelerators:
- Lock Screen: Ctrl+Shift+L --> Search+L
- Task Manager: Shift+Esc --> Search+Esc

The old accelerators are still enabled, but the user is shown a
notification message telling him about the new accelerator.
UMA stats are recorded for both old and new accelerators.
Keyboard overlay has been updated.

BUG=492454, 486198, 498565, 405317, 503351
TEST=manually, ash_unittests --gtest_filter=DeprecatedAcceleratorTester.*,
     ash_unittests --gtest_filter=AcceleratorTableTest.CheckDeprecatedAccelerators

Committed: https://crrev.com/c2b81c9fca4abc10cafdc75d2e7b4f15314e2e5b
Cr-Commit-Position: refs/heads/master@{#335410}

Committed: https://crrev.com/cd2bb98dd450f65d857700df192a34ee79d618bf
Cr-Commit-Position: refs/heads/master@{#347865}

[afakhry, 2015-06-11 23:55:04]

afakhry@chromium.org changed reviewers:
+ mpearson@chromium.org, oshima@chromium.org, thestig@chromium.org,
xiyuan@chromium.org

[afakhry, 2015-06-11 23:55:04]

Hi, Could you please review this CL as follows?


thestig:
  chrome/browser/ui/views/accelerator_table.cc
xiyuan:
  chrome/browser/resources/chromeos/keyboard_overlay_data.js
oshima:
  ash/accelerators/DEPS
  ash/accelerators/accelerator_controller.cc
  ash/accelerators/accelerator_controller.h
  ash/accelerators/accelerator_table.cc
  ash/accelerators/accelerator_table.h
  ash/system/system_notifier.cc
  ash/system/system_notifier.h
mpearson:
  tools/metrics/histograms/histograms.xml

[Lei Zhang, 2015-06-12 00:06:54]

https://codereview.chromium.org/1177773002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1177773002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:15653: +        Shift+Esc is deprecated.
Please use Search+Esc instead.
Is this suppose to be CrOS only? Which key is the search key on Windows?

[afakhry, 2015-06-12 00:40:07]

Patchset #4 (id:60001) has been deleted

[afakhry, 2015-06-12 00:41:58]

https://codereview.chromium.org/1177773002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1177773002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:15653: +        Shift+Esc is deprecated.
Please use Search+Esc instead.
On 2015/06/12 00:06:54, Lei Zhang wrote:
> Is this suppose to be CrOS only? Which key is the search key on Windows?

Yes, thank you. Somehow I missed that. Done.

[Lei Zhang, 2015-06-12 00:49:12]

https://codereview.chromium.org/1177773002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/accelerator_table.cc (right):

https://codereview.chromium.org/1177773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/accelerator_table.cc:138: #if !defined(OS_CHROMEOS)
Just move this down to line ~164 since that's where all the other !OS_CHROMEOS
shortcuts hang out.

Also, I didn't look very hard, but where's the code that handles SEARCH + ESC ?

[afakhry, 2015-06-12 17:26:42]

https://codereview.chromium.org/1177773002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/accelerator_table.cc (right):

https://codereview.chromium.org/1177773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/accelerator_table.cc:138: #if !defined(OS_CHROMEOS)
On 2015/06/12 00:49:11, Lei Zhang wrote:
> Just move this down to line ~164 since that's where all the other !OS_CHROMEOS
> shortcuts hang out.
> 
> Also, I didn't look very hard, but where's the code that handles SEARCH + ESC
?

Done.

SEARCH + ESC is in ash/accelerators/accelerator_table.cc and will be handled by
ash::AcceleratorController rather than the BrowserView.

[xiyuan, 2015-06-12 17:48:37]

keyboard_overlay_data.js LGTM

[Mark P, 2015-06-12 18:01:44]

https://codereview.chromium.org/1177773002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1177773002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:608: +    users use the old vs the new
one.
This second sentence needs to be revise.  It needs to
- explicitly state when an item is emitted to the histogram
- don't state incorrect things.  This histogram does not tell how many users use
the old versus the new one (unless you're referring to the "User Counts"
selection from histogram dashboard, which I doubt; regardless, the histogram
description should be correct regardless of what view / grouping of the data the
user is using.).  It tells us some sort of thing about frequency/counts of use
(which should be made clear simply by fixing the first bullet).

ditto below

[afakhry, 2015-06-12 22:05:54]

Fixed the comment in histograms.xml and added two new unit tests.
@oshima: please find the unit tests in:
ash/accelerators/accelerator_controller_unittest.cc
ash/accelerators/accelerator_table_unittest.cc

https://codereview.chromium.org/1177773002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1177773002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:608: +    users use the old vs the new
one.
On 2015/06/12 18:01:44, Mark P wrote:
> This second sentence needs to be revise.  It needs to
> - explicitly state when an item is emitted to the histogram
> - don't state incorrect things.  This histogram does not tell how many users
use
> the old versus the new one (unless you're referring to the "User Counts"
> selection from histogram dashboard, which I doubt; regardless, the histogram
> description should be correct regardless of what view / grouping of the data
the
> user is using.).  It tells us some sort of thing about frequency/counts of use
> (which should be made clear simply by fixing the first bullet).
> 
> ditto below

You're right. I didn't mean the number of users but rather the number of times
each accelerator type (deprecated or new) is used.

Done.

[Lei Zhang, 2015-06-12 22:08:08]

https://codereview.chromium.org/1177773002/diff/140001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1177773002/diff/140001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15652: +        <message
name="IDS_DEPRECATED_LOCK_SCREEN_MSG"
BTW, why is this in chrome/ and not in say, ash/resources/ash_resources.grd or
ash/ash_strings.grd? It's not actually used in chrome/

[Mark P, 2015-06-12 22:13:25]

histograms.xml lgtm

[afakhry, 2015-06-12 22:36:21]

https://codereview.chromium.org/1177773002/diff/140001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1177773002/diff/140001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15652: +        <message
name="IDS_DEPRECATED_LOCK_SCREEN_MSG"
On 2015/06/12 22:08:07, Lei Zhang wrote:
> BTW, why is this in chrome/ and not in say, ash/resources/ash_resources.grd or
> ash/ash_strings.grd? It's not actually used in chrome/

Makes sense, I wasn't aware that there's an ash_strings.grd.

Done. Moved them to ash_chromeos_strings.grdp since all are ChromeOS deprecated
keys.

[afakhry, 2015-06-15 19:10:49]

oshima@ and thestig@: friendly ping.

[Lei Zhang, 2015-06-15 19:46:03]

chrome/ lgtm with the whitespace changes reverted.

https://codereview.chromium.org/1177773002/diff/160001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1177773002/diff/160001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15648: +      </if>
What's going on here? I think you just added some whitespace.

[afakhry, 2015-06-15 20:47:10]

Reverted whitespace from generated_resources.grd

https://codereview.chromium.org/1177773002/diff/160001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1177773002/diff/160001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15648: +      </if>
On 2015/06/15 19:46:03, Lei Zhang wrote:
> What's going on here? I think you just added some whitespace.

Done.

[oshima, 2015-06-16 05:54:18]

I have a few more comments about overall design. Let's discuss on Wed.

https://codereview.chromium.org/1177773002/diff/180001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1177773002/diff/180001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:104: // This key has been deprecated on
CrOS. It is instead included below in the
indent

https://codereview.chromium.org/1177773002/diff/180001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:209: const DeprecatedAcceleratorData
kDeprecatedAccelerators[] = {
instead of defining both here, it's be better to separate new and old
accelerator info so that we can just remove old ones once we're done.

https://codereview.chromium.org/1177773002/diff/180001/ash/accelerators/accel...
File ash/accelerators/accelerator_table_unittest.cc (right):

https://codereview.chromium.org/1177773002/diff/180001/ash/accelerators/accel...
ash/accelerators/accelerator_table_unittest.cc:83: }
nuke {}

https://codereview.chromium.org/1177773002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/accelerator_table.cc (right):

https://codereview.chromium.org/1177773002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/accelerator_table.cc:161: { ui::VKEY_ESCAPE,
ui::EF_SHIFT_DOWN, IDC_TASK_MANAGER },
I believe this will make it impossible for web pages to steal shift+esc.
Is it intentional?

[afakhry, 2015-06-16 22:30:33]

https://codereview.chromium.org/1177773002/diff/180001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1177773002/diff/180001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:104: // This key has been deprecated on
CrOS. It is instead included below in the
On 2015/06/16 05:54:17, oshima wrote:
> indent

That was done by 'git cl format ash', not sure why! If I don't do it I get a
warning when uploading.

I'll do it anyway.

https://codereview.chromium.org/1177773002/diff/180001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:209: const DeprecatedAcceleratorData
kDeprecatedAccelerators[] = {
On 2015/06/16 05:54:17, oshima wrote:
> instead of defining both here, it's be better to separate new and old
> accelerator info so that we can just remove old ones once we're done.
> 

Let's discuss tomorrow, then.

https://codereview.chromium.org/1177773002/diff/180001/ash/accelerators/accel...
File ash/accelerators/accelerator_table_unittest.cc (right):

https://codereview.chromium.org/1177773002/diff/180001/ash/accelerators/accel...
ash/accelerators/accelerator_table_unittest.cc:83: }
On 2015/06/16 05:54:17, oshima wrote:
> nuke {}

Done.

https://codereview.chromium.org/1177773002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/accelerator_table.cc (right):

https://codereview.chromium.org/1177773002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/accelerator_table.cc:161: { ui::VKEY_ESCAPE,
ui::EF_SHIFT_DOWN, IDC_TASK_MANAGER },
On 2015/06/16 05:54:17, oshima wrote:
> I believe this will make it impossible for web pages to steal shift+esc.
> Is it intentional?

This is intentional. Having this accelerator on ChromeOS will create an issue
that Shift+Esc will be handled by BrowserView whenever the browser window is
shown. This will skip our newly introduced deprecated accelerator handling and
notification, we don't want that. On ChromeOS, there's no need to have two
places handling the same accelerator, so moving it only for non-chromeos
systems.

[oshima, 2015-06-17 05:06:30]

https://codereview.chromium.org/1177773002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/accelerator_table.cc (right):

https://codereview.chromium.org/1177773002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/accelerator_table.cc:161: { ui::VKEY_ESCAPE,
ui::EF_SHIFT_DOWN, IDC_TASK_MANAGER },
On 2015/06/16 22:30:33, afakhry wrote:
> On 2015/06/16 05:54:17, oshima wrote:
> > I believe this will make it impossible for web pages to steal shift+esc.
> > Is it intentional?
> 
> This is intentional. Having this accelerator on ChromeOS will create an issue
> that Shift+Esc will be handled by BrowserView whenever the browser window is
> shown. This will skip our newly introduced deprecated accelerator handling and
> notification, we don't want that. On ChromeOS, there's no need to have two
> places handling the same accelerator, so moving it only for non-chromeos
> systems.

But this will change the current behavior, which I don't think what we want.
Let's discuss this tomorrow.

[afakhry, 2015-06-18 02:11:59]

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

[afakhry, 2015-06-18 02:12:00]

@oshima: Simplified the design as discussed, and modified the tests accordingly.

@thestig: Could you please re-review:
    chrome/browser/ui/views/frame/browser_view.cc?

@sky@: Could you please review:
    ui/views/focus/focus_manager.cc
    ui/views/focus/focus_manager.h
    ui/views/focus/focus_manager_delegate.h?

Thanks.

[Lei Zhang, 2015-06-18 02:36:31]

On 2015/06/18 02:12:00, afakhry wrote:
> @thestig: Could you please re-review:
>     chrome/browser/ui/views/frame/browser_view.cc?

still lgtm

[sky, 2015-06-18 15:25:19]

https://codereview.chromium.org/1177773002/diff/220001/ui/views/focus/focus_m...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/1177773002/diff/220001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:264: bool IsAcceleratorDeprecated(const
ui::Accelerator& accelerator) const;
Feels wrong to put this logic in FocusManager when it doesn't care about
deprecated accelerators at all. Is there another place closer to where you the
logic is that this code could live in?

[afakhry, 2015-06-19 17:26:54]

https://codereview.chromium.org/1177773002/diff/220001/ui/views/focus/focus_m...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/1177773002/diff/220001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:264: bool IsAcceleratorDeprecated(const
ui::Accelerator& accelerator) const;
On 2015/06/18 15:25:18, sky wrote:
> Feels wrong to put this logic in FocusManager when it doesn't care about
> deprecated accelerators at all. Is there another place closer to where you the
> logic is that this code could live in?

Deprecated accelerators are in ash. The FocusManager already has a delegate
(implemented in ash as AshFocusManagerFactory::Delegate) so that it can Process
ash accelerators (in ProcessAccelerator()) and act as the connection between
views and ash.

What do you recommend?

[oshima, 2015-06-19 18:47:56]

On 2015/06/19 17:26:54, afakhry wrote:
>
https://codereview.chromium.org/1177773002/diff/220001/ui/views/focus/focus_m...
> File ui/views/focus/focus_manager.h (right):
> 
>
https://codereview.chromium.org/1177773002/diff/220001/ui/views/focus/focus_m...
> ui/views/focus/focus_manager.h:264: bool IsAcceleratorDeprecated(const
> ui::Accelerator& accelerator) const;
> On 2015/06/18 15:25:18, sky wrote:
> > Feels wrong to put this logic in FocusManager when it doesn't care about
> > deprecated accelerators at all. Is there another place closer to where you
the
> > logic is that this code could live in?
> 
> Deprecated accelerators are in ash. The FocusManager already has a delegate
> (implemented in ash as AshFocusManagerFactory::Delegate) so that it can
Process
> ash accelerators (in ProcessAccelerator()) and act as the connection between
> views and ash.
> 
> What do you recommend?

One approach is to create an utility function / class in c/b/ui/ash to check if
it's deprecated, and that class can delegate to
impl in ash/accelerator. Since it doesn't depend on any state, plain function
call would be sufficient.

It's not optimal, but we can at least isolate the deprecation code so that we
can easily remove when migration is (fully) done.

I could not think of other alternatives that won't require big refactoring.

sky@ WDYT?

[afakhry, 2015-06-19 19:45:06]

Moved the IsAcceleratorDeprecated() to c/b/ui/ash/ash_util.h/cc

[sky, 2015-06-19 21:00:49]

Yes, I much prefer this approach.

https://codereview.chromium.org/1177773002/diff/240001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/ash_util.cc (right):

https://codereview.chromium.org/1177773002/diff/240001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/ash_util.cc:58: ash::AcceleratorController* controller =
Do we really allow this to be null?

https://codereview.chromium.org/1177773002/diff/240001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/ash_util.h (right):

https://codereview.chromium.org/1177773002/diff/240001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/ash_util.h:26: bool IsAcceleratorDeprecated(const
ui::Accelerator accelerator);
const ui::Accelerator&

https://codereview.chromium.org/1177773002/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1177773002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1426: if (event.type ==
blink::WebInputEvent::RawKeyDown)
nit: combine these two ifs.

[oshima, 2015-06-19 21:21:45]

https://codereview.chromium.org/1177773002/diff/240001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1177773002/diff/240001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:104: { true, ui::VKEY_ESCAPE,
ui::EF_COMMAND_DOWN, SHOW_TASK_MANAGER },
Shouldn't we use VKEY_LWIN ?

https://codereview.chromium.org/1177773002/diff/240001/ash/system/system_noti...
File ash/system/system_notifier.cc (right):

https://codereview.chromium.org/1177773002/diff/240001/ash/system/system_noti...
ash/system/system_notifier.cc:73: const char kNotifierAcceleratorController[] =
"ash.accelerator-controller";
kNotifierDeprecatedAccelerator ?

https://codereview.chromium.org/1177773002/diff/240001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/ash_util.cc (right):

https://codereview.chromium.org/1177773002/diff/240001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/ash_util.cc:56: return false;
this shouldn't be nullptr too.

[afakhry, 2015-06-19 22:14:20]

Handling oshima's and sky's comments.

https://codereview.chromium.org/1177773002/diff/240001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1177773002/diff/240001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:104: { true, ui::VKEY_ESCAPE,
ui::EF_COMMAND_DOWN, SHOW_TASK_MANAGER },
On 2015/06/19 21:21:45, oshima wrote:
> Shouldn't we use VKEY_LWIN ?

No VKEY_LWIN is the key code and we already have VKEY_ESCAPE as the key for this
shortcut .. we have to use the COMMAND_DOWN as a modifier (which maps to the
Search key on CrOS).

https://codereview.chromium.org/1177773002/diff/240001/ash/system/system_noti...
File ash/system/system_notifier.cc (right):

https://codereview.chromium.org/1177773002/diff/240001/ash/system/system_noti...
ash/system/system_notifier.cc:73: const char kNotifierAcceleratorController[] =
"ash.accelerator-controller";
On 2015/06/19 21:21:45, oshima wrote:
> kNotifierDeprecatedAccelerator ?

Done.

https://codereview.chromium.org/1177773002/diff/240001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/ash_util.cc (right):

https://codereview.chromium.org/1177773002/diff/240001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/ash_util.cc:56: return false;
On 2015/06/19 21:21:45, oshima wrote:
> this shouldn't be nullptr too.

Done. However there's a similar test for this case above in IsNativeViewInAsh();

https://codereview.chromium.org/1177773002/diff/240001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/ash_util.cc:58: ash::AcceleratorController* controller =
On 2015/06/19 21:00:48, sky wrote:
> Do we really allow this to be null?

Done.

https://codereview.chromium.org/1177773002/diff/240001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/ash_util.h (right):

https://codereview.chromium.org/1177773002/diff/240001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/ash_util.h:26: bool IsAcceleratorDeprecated(const
ui::Accelerator accelerator);
On 2015/06/19 21:00:48, sky wrote:
> const ui::Accelerator&

Done.

https://codereview.chromium.org/1177773002/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1177773002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1426: if (event.type ==
blink::WebInputEvent::RawKeyDown)
On 2015/06/19 21:00:48, sky wrote:
> nit: combine these two ifs.

Actually the second if is only for *is_keyboard_shortcut = true; not for return
false;

[sky, 2015-06-19 23:02:24]

LGTM

[oshima, 2015-06-19 23:04:20]

lgtm

https://codereview.chromium.org/1177773002/diff/240001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1177773002/diff/240001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:104: { true, ui::VKEY_ESCAPE,
ui::EF_COMMAND_DOWN, SHOW_TASK_MANAGER },
On 2015/06/19 22:14:19, afakhry wrote:
> On 2015/06/19 21:21:45, oshima wrote:
> > Shouldn't we use VKEY_LWIN ?
> 
> No VKEY_LWIN is the key code and we already have VKEY_ESCAPE as the key for
this
> shortcut .. we have to use the COMMAND_DOWN as a modifier (which maps to the
> Search key on CrOS).

Ah, sorry for my confusion.

[afakhry, 2015-06-19 23:05:25]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2015-06-19 23:05:26]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
mpearson@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1177773002/#ps260001
(title: "Handling oshima's and sky's comments")

[commit-bot: I haz the power, 2015-06-19 23:06:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177773002/260001

[commit-bot: I haz the power, 2015-06-19 23:43:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-19 23:43:25]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[afakhry, 2015-06-20 00:44:34]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2015-06-20 00:44:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, xiyuan@chromium.org, oshima@chromium.org,
mpearson@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1177773002/#ps280001
(title: "Preventing zero-sized arrays on non-CrOs systems, making deprecated
accelerators CrOs-only for now")

[commit-bot: I haz the power, 2015-06-20 00:46:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177773002/280001

[commit-bot: I haz the power, 2015-06-20 01:09:46]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2015-06-20 01:10:53]

Patchset 14 (id:??) landed as
https://crrev.com/c2b81c9fca4abc10cafdc75d2e7b4f15314e2e5b
Cr-Commit-Position: refs/heads/master@{#335410}

[afakhry, 2015-09-08 18:34:39]

oshima@chromium.org: Could you please take a look at the changes I made upon
request from the UI team. in particular the below files:

ash/shell/shell_delegate_impl.h
ash/shell/shell_delegate_impl.cc
ash/shell_delegate.h
ash/test/test_shell_delegate.h
ash/test/test_shell_delegate.cc
chrome/browser/ui/ash/chrome_shell_delegate.h
chrome/browser/ui/ash/chrome_shell_delegate.cc

[oshima, 2015-09-08 19:06:47]

https://codereview.chromium.org/1177773002/diff/380001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/1177773002/diff/380001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.cc:178: browser,
GURL("https://support.google.com/chromebook/answer/183101"),
define constant with a comprehensive name.

https://codereview.chromium.org/1177773002/diff/380001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.cc:184: gfx::Image
ChromeShellDelegate::GetDeprecatedAcceleratorImage() const {
for ash specific code, you can just use gfx::ImageSkia, which is more efficient.

[afakhry, 2015-09-08 21:47:35]

https://codereview.chromium.org/1177773002/diff/380001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/1177773002/diff/380001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.cc:178: browser,
GURL("https://support.google.com/chromebook/answer/183101"),
On 2015/09/08 19:06:47, oshima wrote:
> define constant with a comprehensive name.

Done.

https://codereview.chromium.org/1177773002/diff/380001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.cc:184: gfx::Image
ChromeShellDelegate::GetDeprecatedAcceleratorImage() const {
On 2015/09/08 19:06:46, oshima wrote:
> for ash specific code, you can just use gfx::ImageSkia, which is more
efficient.

As we discussed Notification ctor needs an Image.

[oshima, 2015-09-08 21:50:18]

lgtm

[afakhry, 2015-09-08 23:16:13]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2015-09-08 23:16:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, xiyuan@chromium.org, mpearson@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1177773002/#ps400001
(title: "oshima's comments")

[commit-bot: I haz the power, 2015-09-08 23:17:00]

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

[commit-bot: I haz the power, 2015-09-09 00:02:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-09 00:02:57]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[afakhry, 2015-09-09 00:30:43]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2015-09-09 00:31:48]

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

[commit-bot: I haz the power, 2015-09-09 09:03:51]

Committed patchset #20 (id:400001)

[commit-bot: I haz the power, 2015-09-09 09:05:37]

Patchset 20 (id:??) landed as
https://crrev.com/cd2bb98dd450f65d857700df192a34ee79d618bf
Cr-Commit-Position: refs/heads/master@{#347865}