Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
350943003:
Support global keyboard commands on Chrome OS. (Closed)
|
Created:
6 years, 6 months ago by David Tseng Modified:
6 years, 5 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionSupport global keyboard commands on Chrome OS.
Currently, AcceleratorController delivers accelerators to GlobalCommandsListenerChromeOS (which is an AcceleratorTarget). We respect the same conditions found within AcceleratorController to disallow shortcuts in specific instances (e.g. login screen locked screen, empty windows). AcceleratorController is owned by the ash::Shell and knows about existing keyboard shortcuts/actions.
BUG=336761
TEST=interactive_ui_tests --gtest_filter=GlobalCommandsApiTest.*
And, manually, on a desktop build of Chrome with GYP_DEFINES=chromeos=1...
with a component extension:
Verified that
- a global shortcut (ctrl+shift+0) works with focus in
1. web content
2. shelf
3. toolbars (omni box, View(s)).
Verified that processing does not occur in the following cases (similarly to most other ash accelerators) as expected:
- OOBE
- locked screen
- desktop with no windows open
- sign in screen (after logging out).
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282110
Patch Set 1 #Patch Set 2 : Updates to prevent propagation and enable global commands on cros by default. #Patch Set 3 : Formatting. #
Total comments: 17
Patch Set 4 : Address various comments. #
Total comments: 28
Patch Set 5 : Respond to oshima's comments. #Patch Set 6 : JS related changes. #Patch Set 7 : Address oshima's comments. #
Total comments: 1
Patch Set 8 : Omit media keys test. #Patch Set 9 : Test updates #Messages
Total messages: 52 (0 generated)
Initial impl. Please take a look. Keyboard commands appear to not work while within the status tray/shelf area, OOBE, locked screen, and more. Shortcuts only work within web contents (including find dialog).
Hmm... We need an altered approach. More on that later. The first line of business, though, is figuring out why the GlobalShortcutListenerChromeOS instance isn't being requested. I kind of suspect you might be over that problem (and the CL description is just out of date), but if not then that's our biggest problem right now. You need to set the enable global flag (feature switch) to be enabled in the code and once you install an extension (that has a keybinding/browser action) you should be able to navigate to: chrome://extensions/configureCommands and see a combo box for a shortcut that flips the keybinding between local & global. If you don't have at least one extension that sets it to global then the code will never request the GlobalShortcutListenerChromeOS instance. You can take a quicker route and have an extension that defines a global shortcut (I believe the one I gave you does that), but at a minimum I'd verify you see the combobox on the page above, because if not then none of the code down the line will hit. Now, the other big problem is that you seem to be trying to handle shortcuts that are local in nature and not global, which is why they don't work except in web contents/find dialog. What is needed instead is the ChromeOS equivalent of RegisterHotKey on Windows. See: http://msdn.microsoft.com/en-us/library/windows/desktop/ms646309(v=vs.85).aspx ... or XGrabKey on X11 (http://tronche.com/gui/x/xlib/input/XGrabKey.html), as implemented here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... In other words, we need a function that sets a system-wide shortcut listener, that works when Chrome *doesn't* have focus. The Accelerator handling in Chrome is designed to work on keys that pass through the keyboard handling chain of Chrome (requiring Chrome to have focus), but we should be dealing with (handling) shortcuts that would normally go to other apps. I have a couple of code comments coming as well.
Actually, I may have replied prematurely. I just discovered the call to AcceleratorController on ChromeOS does handle global shortcuts, so you might be on the right track. Sorry for the confusion.
OK, a second reading of the code revealed further review comments were not necessary. On the face of it, it looks like this is along the lines of what is needed. So the most pressing problem we need to figure out is why the code only handles some of the shortcuts that are global. In other words, why are the shortcuts that you press within the tray/shelf area not getting passed through the AcceleratorController. And note that I never would expect the shortcuts to be active on the *lock screen*, but for other use cases it shouldn't matter what app/widget is active, we need to handle the shortcut. I don't know the AcceleratorController well enough, but if this is a limitation of that object (that you'll never get all global shortcuts) then we might need to think about a more low-level approach, like the XGrabKey equivalent for ChromeOS. I'm not sure what that is, though.
PTAL. I got this to work with the component extension I was trying this with. I made two notable changes in the latest patchset: - enable global commands on cros by default (feature switches.cc). In brief, after doing this, commands started working globally. '--global-commands' flag wasn't taking effect for some reason before. - I refactored some code in accelerator_controller to allow us to observe the restrictions on accelerators imposed by Chrome OS (for example, when you're on the login screen, no windows are open, locked screen, etc). I verified that: - without any restrictions, global shortcuts work everywhere (including login screen). - with restrictions on (as in this cl), global shortcuts only work: -- when there's at least one browser window open -- with focus in any of [shelf, browser window, toolbars]. On Wed, Jun 25, 2014 at 4:28 AM, <finnur@chromium.org> wrote: > OK, a second reading of the code revealed further review comments were not > necessary. On the face of it, it looks like this is along the lines of > what is > needed. > > So the most pressing problem we need to figure out is why the code only > handles > some of the shortcuts that are global. In other words, why are the > shortcuts > that you press within the tray/shelf area not getting passed through the > AcceleratorController. And note that I never would expect the shortcuts to > be > active on the *lock screen*, but for other use cases it shouldn't matter > what > app/widget is active, we need to handle the shortcut. > > I don't know the AcceleratorController well enough, but if this is a > limitation > of that object (that you'll never get all global shortcuts) then we might > need > to think about a more low-level approach, like the XGrabKey equivalent for > ChromeOS. I'm not sure what that is, though. > > > https://codereview.chromium.org/350943003/ > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
First a few comments I have after reading the CL description (before I dig into the code)... 1) First two lines have quite bit redundancy built in. :) 2) We have automated tests that currently exclude ChromeOS, which should be enabled as part of this CL. 3) The "does not work" list makes it look like the CL is incomplete. I would make it more clear that global commands should not work in those scenarios -- and that's to be expected.
Code comments below. For tests, see: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... ... and ... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... Can you do me a favor and try enabling the latter test for all platforms and see if it passes on the try bots? https://codereview.chromium.org/350943003/diff/40001/ash/accelerators/acceler... File ash/accelerators/accelerator_controller.h (right): https://codereview.chromium.org/350943003/diff/40001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.h:42: // processing. I think we should flesh out this comment a little more, as in why are we doing this and whether the restriction is global (applies to everything or limited to some targets). https://codereview.chromium.org/350943003/diff/40001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.h:43: enum AcceleratorProcessRestriction { nit: Process is ambiguous. Processing? https://codereview.chromium.org/350943003/diff/40001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.h:87: // Gets any restriction for the given accelerator and action. nit: "Returns which restriction applies to a given ..." https://codereview.chromium.org/350943003/diff/40001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.h:88: AcceleratorProcessRestriction GetAcceleratorProcessRestriction( ditto: Ambiguous word "Process". https://codereview.chromium.org/350943003/diff/40001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.h:90: ui::Accelerator accelerator); const ui::Accelerator& ? https://codereview.chromium.org/350943003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/350943003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:73: RESTRICTION_PREVENT_PROCESSING_AND_PROPAGATION; What happens if code A registers accelerator foo, your code then registers foo, and code B then registers foo. Is your code still guaranteed to see it? If not, in what scenarios does it not? https://codereview.chromium.org/350943003/diff/40001/extensions/common/featur... File extensions/common/feature_switch.cc (left): https://codereview.chromium.org/350943003/diff/40001/extensions/common/featur... extensions/common/feature_switch.cc:32: #endif Actually, there's no point in keeping this around anymore. Please delete this switch and the corresponding command line flag (switches::kGlobalCommands).
First, for your overall comments... 1) First two lines have quite bit redundancy built in. :) Killed line two. 2) We have automated tests that currently exclude ChromeOS, which should be enabled as part of this CL. Awesome; was looking at testing within an ExtensionBrowserTest. Got the test to pass on Chrome OS as of the latest patchset. 3) The "does not work" list makes it look like the CL is incomplete. I would make it more clear that global commands should not work in those scenarios -- and that's to be expected. Makes sense; rephrased as "Verified that...".
https://codereview.chromium.org/350943003/diff/40001/ash/accelerators/acceler... File ash/accelerators/accelerator_controller.h (right): https://codereview.chromium.org/350943003/diff/40001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.h:42: // processing. On 2014/06/26 10:37:35, Finnur wrote: > I think we should flesh out this comment a little more, as in why are we doing > this and whether the restriction is global (applies to everything or limited to > some targets). It is a non-enforced restriction at the moment. Any target registered with AcceleratorController should respect the restriction, but it is currently only enforced by each target. Unfortunately, the class isn't well flushed out because it also acts as an AcceleratorTarget; at some point, we should break up the class, but I don't think that's within scope of this cl? I left a better comment. https://codereview.chromium.org/350943003/diff/40001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.h:43: enum AcceleratorProcessRestriction { On 2014/06/26 10:37:35, Finnur wrote: > nit: Process is ambiguous. Processing? Yup :); getting wordy, but it does make more sense. https://codereview.chromium.org/350943003/diff/40001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.h:87: // Gets any restriction for the given accelerator and action. On 2014/06/26 10:37:35, Finnur wrote: > nit: "Returns which restriction applies to a given ..." Done. https://codereview.chromium.org/350943003/diff/40001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.h:88: AcceleratorProcessRestriction GetAcceleratorProcessRestriction( On 2014/06/26 10:37:35, Finnur wrote: > ditto: Ambiguous word "Process". Done, everywhere. https://codereview.chromium.org/350943003/diff/40001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.h:90: ui::Accelerator accelerator); On 2014/06/26 10:37:35, Finnur wrote: > const ui::Accelerator& ? Done. https://codereview.chromium.org/350943003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/350943003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:73: RESTRICTION_PREVENT_PROCESSING_AND_PROPAGATION; On 2014/06/26 10:37:35, Finnur wrote: > What happens if code A registers accelerator foo, your code then registers foo, > and code B then registers foo. > > Is your code still guaranteed to see it? If not, in what scenarios does it not? The latter, when target B: - prevents propagation - prevents processing and propagation https://codereview.chromium.org/350943003/diff/40001/extensions/common/featur... File extensions/common/feature_switch.cc (left): https://codereview.chromium.org/350943003/diff/40001/extensions/common/featur... extensions/common/feature_switch.cc:32: #endif On 2014/06/26 10:37:35, Finnur wrote: > Actually, there's no point in keeping this around anymore. Please delete this > switch and the corresponding command line flag (switches::kGlobalCommands). I wasn't sure if it was needed to (sometimes) disable global commands from the command line. Done.
+ oshima for ash/... I think it's ready for an OWNER to look at.
Extensions stuff LGTM, with some questions and a couple of nits. I'll leave it to oshima@ to judge the Ash change. https://codereview.chromium.org/350943003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/350943003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:73: RESTRICTION_PREVENT_PROCESSING_AND_PROPAGATION; Let me rephrase the question... Are there scenarios (ignoring lock screen/OOBE/etc) where an extension registers accelerator foo as a global accelerator but doesn't get it because another target has registered it? If so, what are those scenarios? https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:124: #if defined(OS_WIN) || defined(OS_CHROMEOS) Yay! I was hoping something like this would do the trick! https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:173: #if defined(OS_WIN) What was the outcome of the experiment to run this on all platforms? https://codereview.chromium.org/350943003/diff/80001/chrome/common/extensions... File chrome/common/extensions/command.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/common/extensions... chrome/common/extensions/command.cc:540: extension_data->SetBoolean("scope_ui_visible", true); Please remove this boolean assignment as well, and delete lines 290 and 267 from this file: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... And, as a sanity test, make sure the Chrome/Global combo box on the config UI for shortcuts still works after that change.
https://codereview.chromium.org/350943003/diff/80001/ash/accelerators/acceler... File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/350943003/diff/80001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.cc:1120: const ui::Accelerator& accelerator) { accelerator is not used here. Maybe this is what you really want? GetAcceleratorProcessingRestriction(const ui::Accelerator& accelerator) { std::map<ui::Accelerator, int>::const_iterator iter = accelerators_.find(accelerator); int action = iter->second; .... } https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:52: return controller->IsRegistered(accelerator); AcceleratorManager has DCHECKs for duplicated accelerators. What's the expected behavior if two extensions requested the same accelerator? And if we're going to allow extensions to override chromeos accelerator, what's the implication for keyboard overlay? I guess we need to at least show that shortcut is taken by an extension? https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:70: controller->GetAcceleratorProcessingRestriction(-1, accelerator); shouldn't this always return NONE? (related to my comment above)
First responding to oshima; thanks for the quick review. https://codereview.chromium.org/350943003/diff/80001/ash/accelerators/acceler... File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/350943003/diff/80001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.cc:1120: const ui::Accelerator& accelerator) { On 2014/06/27 18:40:40, oshima wrote: > accelerator is not used here. Maybe this is what you really want? > > GetAcceleratorProcessingRestriction(const ui::Accelerator& accelerator) { > std::map<ui::Accelerator, int>::const_iterator iter = > accelerators_.find(accelerator); > int action = iter->second; > .... > > } I removed the accelerator parameter. Perhaps the naming is unclear, but the function simply captures the scenarios in which accelerators should not be processed or not propagated. (e.g. in login screen). https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:52: return controller->IsRegistered(accelerator); On 2014/06/27 18:40:41, oshima wrote: > AcceleratorManager has DCHECKs for duplicated accelerators. What's the expected > behavior if two extensions requested the same accelerator? I'll let finnur handle this case (it's not specific to Chrome OS) and not within scope of this cl. > > And if we're going to allow extensions to override chromeos accelerator, what's > the implication for keyboard overlay? I guess we need to at least show that > shortcut is taken by an extension? Well, I check IsReservedAccelerator during registration. IsReservedAccelerator appears to at least have the 'show keyboard overlay' action; were there other concerns? An extension trying to register would fail at install time (it wouldn't even install). Also, global shortcuts are currently limited to ctrl+shift+[0-9]. https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:70: controller->GetAcceleratorProcessingRestriction(-1, accelerator); On 2014/06/27 18:40:41, oshima wrote: > shouldn't this always return NONE? (related to my comment above) Why? When an extension is running within the locked screen and you press a global shortcut registered by an extension, this will return prevent processing (but allow other targets to ahndle the accelerator).
https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:52: return controller->IsRegistered(accelerator); > An extension trying to register would fail at install time > (it wouldn't even install). Almost. :) The expected behavior when extension A and extension B request the same accelerator is for the first one (A) to win and the second (B) to get installed but no shortcut assigned for that action. If the user decides extension B should get that shortcut, the shortcut assignment for A should get unregistered before B registers (which is what the DCHECK will catch). The answer is a little different for Media key shortcuts (play/pause/volume/etc) because those are non-exclusive and should go to all apps that register for it and that should not DCHECK (it doesn't right?). > And if we're going to allow extensions to override chromeos accelerator For the most part, the answer here has been "We don't allow any automatic overriding by an extension but yes, if the user chooses to do so after installing the extension then we don't object". There are probably some exceptions where we'd object, but those we should evaluate on case-by-case basis. The argument has so far been: If the user is proficient enough to set the shortcut, he/she should be proficient enough to undo it. https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:70: controller->GetAcceleratorProcessingRestriction(-1, accelerator); Funny. I actually had the opposite reaction to this code. I thought... "shouldn't it NEVER return RESTRICTION_NONE?!" I had to read the function twice to figure out what would happen. :)
https://codereview.chromium.org/350943003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/350943003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:73: RESTRICTION_PREVENT_PROCESSING_AND_PROPAGATION; On 2014/06/27 11:10:57, Finnur wrote: > Let me rephrase the question... > > Are there scenarios (ignoring lock screen/OOBE/etc) where an extension registers > accelerator foo as a global accelerator but doesn't get it because another > target has registered it? If so, what are those scenarios? Are we talking about in theory or in practice? https://code.google.com/p/chromium/codesearch#search/&q=accelerator_controlle... are all references that try to get the AcceleratorController instance owned by ash::Shell. I don't see any other targets getting added. If, in the future, there were, it's possible for your scenario to play out, but at the moment, I'm saying no. To be clear, (as I'm somewhat new to this code as well), the AcceleratorController hooks up to the ui module's eventing system by: AcceleratorController <- AcceleratorDelegate <- AcceleratorFilter <- ash::Shell::AddPreTargetHandler (which is an impl of ui::EventTarget). ash::Shell is I think added as an EventTarget of a root window at some point early on. As I understand the event propagation (which might be wrong), the root window receives events first and a pre target handler receives the events even before the target itself. In writing this, I guess another pre target handler could stop the event from propagating, so maybe your scenario could play out that way. https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:173: #if defined(OS_WIN) On 2014/06/27 11:10:57, Finnur wrote: > What was the outcome of the experiment to run this on all platforms? It does pass; I'll enable the test for Chrome OS as well. The comment made it seem like there's explicit action required of other platforms? (e.g. what about linux or is it just the test not sending keys through X). https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:70: controller->GetAcceleratorProcessingRestriction(-1, accelerator); On 2014/06/27 22:23:44, Finnur wrote: > Funny. I actually had the opposite reaction to this code. I thought... > "shouldn't it NEVER return RESTRICTION_NONE?!" I had to read the function twice > to figure out what would happen. :) I'm happy to change the naming/logic given suggestions. I considered pulling the specific checks (e.g. for locked screen) without actions here, but weighed the cost of future maintenance as an overriding factor. The previous code implicitly delt with the three states without being that clear IMO. https://codereview.chromium.org/350943003/diff/80001/chrome/common/extensions... File chrome/common/extensions/command.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/common/extensions... chrome/common/extensions/command.cc:540: extension_data->SetBoolean("scope_ui_visible", true); On 2014/06/27 11:10:57, Finnur wrote: > Please remove this boolean assignment as well, and delete lines 290 and 267 from > this file: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > And, as a sanity test, make sure the Chrome/Global combo box on the config UI > for shortcuts still works after that change. Removed ln 267 and 291, and removed the boolean assignment here.
Still LGTM https://codereview.chromium.org/350943003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/350943003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:73: RESTRICTION_PREVENT_PROCESSING_AND_PROPAGATION; > Are we talking about in theory or in practice? If it only happens in theory then there's not much of a worry now, but we might have to deal with it at some point in the future. https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:173: #if defined(OS_WIN) Yeah, when I wrote that comment, the other platforms weren't ready and then other people implemented a solution for Linux and Mac (but didn't enable the test). I was somewhat confident that Linux would work, but less sure about Mac. Glad to hear they all work. Can you enable this test for all platforms then? (that pass)
https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:52: return controller->IsRegistered(accelerator); On 2014/06/27 22:23:44, Finnur wrote: > > An extension trying to register would fail at install time > > (it wouldn't even install). > > Almost. :) The expected behavior when extension A and extension B request the > same accelerator is for the first one (A) to win and the second (B) to get > installed but no shortcut assigned for that action. If the user decides > extension B should get that shortcut, the shortcut assignment for A should get > unregistered before B registers (which is what the DCHECK will catch). > > The answer is a little different for Media key shortcuts (play/pause/volume/etc) > because those are non-exclusive and should go to all apps that register for it > and that should not DCHECK (it doesn't right?). > > > And if we're going to allow extensions to override chromeos accelerator > > For the most part, the answer here has been "We don't allow any automatic > overriding by an extension but yes, if the user chooses to do so after > installing the extension then we don't object". There are probably some > exceptions where we'd object, but those we should evaluate on case-by-case > basis. How/where this is controlled? by permission? > > The argument has so far been: If the user is proficient enough to set the > shortcut, he/she should be proficient enough to undo it. Have you discussed this with ChromeOS PM/UX (kuscher/jennchen)? https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:52: return controller->IsRegistered(accelerator); On 2014/06/27 21:47:16, David Tseng wrote: > On 2014/06/27 18:40:41, oshima wrote: > > AcceleratorManager has DCHECKs for duplicated accelerators. What's the > expected > > behavior if two extensions requested the same accelerator? > > I'll let finnur handle this case (it's not specific to Chrome OS) and not within > scope of this cl. Not sure if I understand. This CL is for ChromeOS, isn't it? > > > > And if we're going to allow extensions to override chromeos accelerator, > what's > > the implication for keyboard overlay? I guess we need to at least show that > > shortcut is taken by an extension? > > Well, I check IsReservedAccelerator during registration. IsReservedAccelerator > appears to at least have the 'show keyboard overlay' action; were there other > concerns? An extension trying to register would fail at install time (it > wouldn't even install). We do allow web contents to consume accelerators used by ash, except for "reserved". Checking "IsReserved" won't prevent from using the accelerator. Maybe IsRegistered() is what you want? If not, I may be missing something. Can you tell me where this is enforced? > Also, global shortcuts are currently limited to ctrl+shift+[0-9]. and ChromeOS is using ctrl-shift-9 and 0 https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:61: this); I assume the accelerators registered by an extension get unregistered when the extension goes away, correct? and I also assume that the extension will the unregister only if it was successfully registered by that extension? https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:70: controller->GetAcceleratorProcessingRestriction(-1, accelerator); On 2014/06/28 14:17:50, David Tseng wrote: > On 2014/06/27 22:23:44, Finnur wrote: > > Funny. I actually had the opposite reaction to this code. I thought... > > "shouldn't it NEVER return RESTRICTION_NONE?!" I had to read the function > twice > > to figure out what would happen. :) > > I'm happy to change the naming/logic given suggestions. > > I considered pulling the specific checks (e.g. for locked screen) without > actions here, but weighed the cost of future maintenance as an overriding > factor. > > The previous code implicitly delt with the three states without being that clear > IMO. Hmm, passing -1 is not intuitive. Here is my recommendation. * Make GetAcceleratoProcessingRestriction private * Define new public method GetCurrentAcceleratorRestriction() and call GetAcceleratorProcessingRestriction from there.
https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:52: return controller->IsRegistered(accelerator); On 2014/07/01 17:48:25, oshima wrote: > On 2014/06/27 21:47:16, David Tseng wrote: > > On 2014/06/27 18:40:41, oshima wrote: > > > AcceleratorManager has DCHECKs for duplicated accelerators. What's the > > expected > > > behavior if two extensions requested the same accelerator? > > > > I'll let finnur handle this case (it's not specific to Chrome OS) and not > within > > scope of this cl. > > Not sure if I understand. This CL is for ChromeOS, isn't it? > The commands API has already shipped for all platforms (including Chrome OS) for non-global accelerators. @finnur can address the scenario where two extensions attempt to request the same accelerator (non-global or global). > > > > > > And if we're going to allow extensions to override chromeos accelerator, > > what's > > > the implication for keyboard overlay? I guess we need to at least show that > > > shortcut is taken by an extension? > > > > Well, I check IsReservedAccelerator during registration. IsReservedAccelerator > > appears to at least have the 'show keyboard overlay' action; were there other > > concerns? An extension trying to register would fail at install time (it > > wouldn't even install). > > We do allow web contents to consume accelerators used by ash, except for > "reserved". Checking "IsReserved" won't prevent from using the accelerator. > Maybe IsRegistered() is what you want? If not, I may be missing something. > Can you tell me where this is enforced? > Good point; changed to check for IsRegistered. > > Also, global shortcuts are currently limited to ctrl+shift+[0-9]. > > and ChromeOS is using ctrl-shift-9 and 0 Is this captured by IsRegistered? Seems like we should have a discussion among finnur, an appropriate Chrome OS folks and I about possibilities here. First, is if extensions should be allowed to override accelerators (subset perhaps?). https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:70: controller->GetAcceleratorProcessingRestriction(-1, accelerator); On 2014/07/01 17:48:25, oshima wrote: > On 2014/06/28 14:17:50, David Tseng wrote: > > On 2014/06/27 22:23:44, Finnur wrote: > > > Funny. I actually had the opposite reaction to this code. I thought... > > > "shouldn't it NEVER return RESTRICTION_NONE?!" I had to read the function > > twice > > > to figure out what would happen. :) > > > > I'm happy to change the naming/logic given suggestions. > > > > I considered pulling the specific checks (e.g. for locked screen) without > > actions here, but weighed the cost of future maintenance as an overriding > > factor. > > > > The previous code implicitly delt with the three states without being that > clear > > IMO. > > Hmm, passing -1 is not intuitive. Here is my recommendation. > > * Make GetAcceleratoProcessingRestriction private > * Define new public method GetCurrentAcceleratorRestriction() and call > GetAcceleratorProcessingRestriction from there. > Done.
https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:52: return controller->IsRegistered(accelerator); On 2014/07/01 17:48:25, oshima wrote: > On 2014/06/27 21:47:16, David Tseng wrote: > > On 2014/06/27 18:40:41, oshima wrote: > > > AcceleratorManager has DCHECKs for duplicated accelerators. What's the > > expected > > > behavior if two extensions requested the same accelerator? > > > > I'll let finnur handle this case (it's not specific to Chrome OS) and not > within > > scope of this cl. > > Not sure if I understand. This CL is for ChromeOS, isn't it? > > > > > > > And if we're going to allow extensions to override chromeos accelerator, > > what's > > > the implication for keyboard overlay? I guess we need to at least show that > > > shortcut is taken by an extension? > > > > Well, I check IsReservedAccelerator during registration. IsReservedAccelerator > > appears to at least have the 'show keyboard overlay' action; were there other > > concerns? An extension trying to register would fail at install time (it > > wouldn't even install). > > We do allow web contents to consume accelerators used by ash, except for > "reserved". Checking "IsReserved" won't prevent from using the accelerator. > Maybe IsRegistered() is what you want? If not, I may be missing something. > Can you tell me where this is enforced? > > > Also, global shortcuts are currently limited to ctrl+shift+[0-9]. > > and ChromeOS is using ctrl-shift-9 and 0 @finnur, looks like the tests do fail after I added the check for IsRegistered (since Chrome OS binds ctrl+shift+0 which the tests use). Any opinions on an alternate set of modifiers? Stepping back, perhaps we should meet with the key stakeholders to discuss.
Thanks, CL looks good, with one more question below. https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:52: return controller->IsRegistered(accelerator); On 2014/07/01 18:34:42, David Tseng wrote: > On 2014/07/01 17:48:25, oshima wrote: > > On 2014/06/27 21:47:16, David Tseng wrote: > > > On 2014/06/27 18:40:41, oshima wrote: > > > > AcceleratorManager has DCHECKs for duplicated accelerators. What's the > > > expected > > > > behavior if two extensions requested the same accelerator? > > > > > > I'll let finnur handle this case (it's not specific to Chrome OS) and not > > within > > > scope of this cl. > > > > Not sure if I understand. This CL is for ChromeOS, isn't it? > > > > The commands API has already shipped for all platforms (including Chrome OS) for > non-global accelerators. @finnur can address the scenario where two extensions > attempt to request the same accelerator (non-global or global). Ah, sorry, I somehow mixed up with my question about cros shortcut. please disregard. https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:61: this); On 2014/07/01 17:48:25, oshima wrote: > I assume the accelerators registered by an extension get unregistered when the > extension goes away, correct? and I also assume that the extension will the > unregister only if it was successfully registered by that extension? Can you answer this question? I want to make sure that an extension won't accidentally unregister the accelerator defined by cros, or other extensions. Can you add test case for this, especially there is an overlap between cros shortcut and extension shotcuts?
https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:52: return controller->IsRegistered(accelerator); Not sure exactly what to disregard, so I'll address what has been stated -- feel free to ignore my answer if the question is not valid any longer. :) > Not sure if I understand. This CL is for ChromeOS, isn't it? Correct. This CL brings ChromeOS to the same level as other platforms, but the concern you raised is not ChromeOS-specific and has already been addressed in a cross-platform way in other CLs. In short, extension double-registration of a shortcut shouldn't be a concern. > Have you discussed this with ChromeOS PM/UX (kuscher/jennchen)? The UX person that oversaw this effort was Glen Murphy. This has been the functionality on all other platforms from the start and on ChromeOS you can do this today with non-global shortcuts, so there's no additional risk being introduced by David. And changing it would not just be way beyond the scope of this CL, but also undesirable. The point of the extension system is to allow the user to modify the behavior of the browser. If someone can come up with a superior/drastically different way to say... find a string on a page, the user should be able to assign Ctrl+F to that functionality to suppress our Find dialog and use an extension provided one. Also, if the user never uses a shortcut for what we intended it to be used, they can override it and use it for something completely unrelated. Those two points are the two main one in why we allow overriding basic Chrome functionality shortcuts. And, it is worth reiterating: Extensions can not steal shortcuts already in use, but the user can override. The Ctrl+Shift+0 and Ctrl+Shift+9 situation on ChromeOS is a bit unfortunate, but I'm OK with not allowing an extension to automatically assign neither Ctrl+Shift+0 nor Ctrl+Shift+9 on ChromeOS as long as we document that fact. Doc is here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... You'd have to change the tests that fail to use 1 and 2 (or something) instead of 0 and 9 and double check that the user is still able to assign Ctrl+Shift+0 and 9 to an extension command (and perhaps verify that in a test). https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:61: this); I can't speak for the ChromeOS accelerators, but on Windows the way it works is the accelerator assignment is added to the front of a vector that keeps track of who is registered. When an extension is unregistered, its entry at the front of that vector is removed so the second entry in line is the one that takes over. If the extension was overriding Ctrl+F then the Find dialog starts appearing again after the extension is unregistered. https://codereview.chromium.org/350943003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/350943003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:173: #if defined(OS_WIN) || defined(OS_CHROMEOS) Enable for more platforms? :)
lgtm https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:52: return controller->IsRegistered(accelerator); On 2014/07/02 12:41:25, Finnur wrote: > Not sure exactly what to disregard, so I'll address what has been stated -- feel > free to ignore my answer if the question is not valid any longer. :) > > > Not sure if I understand. This CL is for ChromeOS, isn't it? > > Correct. This CL brings ChromeOS to the same level as other platforms, but the > concern you raised is not ChromeOS-specific and has already been addressed in a > cross-platform way in other CLs. In short, extension double-registration of a > shortcut shouldn't be a concern. > > > Have you discussed this with ChromeOS PM/UX (kuscher/jennchen)? > > The UX person that oversaw this effort was Glen Murphy. This has been the > functionality on all other platforms from the start and on ChromeOS you can do > this today with non-global shortcuts, so there's no additional risk being > introduced by David. And changing it would not just be way beyond the scope of > this CL, but also undesirable. The point of the extension system is to allow the > user to modify the behavior of the browser. If someone can come up with a > superior/drastically different way to say... find a string on a page, the user > should be able to assign Ctrl+F to that functionality to suppress our Find > dialog and use an extension provided one. Also, if the user never uses a > shortcut for what we intended it to be used, they can override it and use it for > something completely unrelated. Those two points are the two main one in why we > allow overriding basic Chrome functionality shortcuts. And, it is worth > reiterating: Extensions can not steal shortcuts already in use, but the user can > override. > > > The Ctrl+Shift+0 and Ctrl+Shift+9 situation on ChromeOS is a bit unfortunate, > but I'm OK with not allowing an extension to automatically assign neither > Ctrl+Shift+0 nor Ctrl+Shift+9 on ChromeOS as long as we document that fact. I had the question only because the original CL did allow overriding cros shortcut. This is no longer my concern.
https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/350943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:124: #if defined(OS_WIN) || defined(OS_CHROMEOS) On 2014/06/27 11:10:57, Finnur (OOO - back Jul 14th) wrote: > Yay! I was hoping something like this would do the trick! Sorry for the late update on this (just back from OOO). This test works fine since it registers ctrl+shift+1. The other test, though, times out because it attempts to register media keys (which are taken by ash). In effort to get this cl in, I've omitted Chrome OS from that test. In forced app mode, on ash at least, overriding of media keys appears to be allowed, but this is not the case in general.
The CQ bit was checked by dtseng@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/350943003/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/350943003/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
> This test works fine since it registers ctrl+shift+1. The other test, though, > times out because it attempts to register media keys (which are taken by ash). > In effort to get this cl in, I've omitted Chrome OS from that test. > > In forced app mode, on ash at least, overriding of media keys appears to be > allowed, but this is not the case in general. It is fine to check in without it, but we should think about this case. It doesn't make sense to me to bar global commands from overriding Media keys when you can override them with non-global commands today.
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/350943003/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by dtseng@chromium.org
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/350943003/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26172) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/30619)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26210)
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/350943003/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/350943003/240001
Message was sent while issue was closed.
Change committed as 282110 |
