Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
251193005:
Enabled volume buttons when TouchView is active (Closed)
|
Created:
6 years, 7 months ago by bruthig Modified:
6 years, 7 months ago CC:
chromium-reviews, kalyank, sadrul, ben+ash_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionEnabled volume buttons when TouchView is active
BUG=364584
TEST=MaximizeModeControllerTest.AllowsVolumeControl
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268181
Patch Set 1 #
Total comments: 12
Patch Set 2 : Style fixes as per Jons comments from Patch Set 1 #
Total comments: 3
Patch Set 3 : Addressed Jon's comments from Patch Set 2 #
Total comments: 16
Patch Set 4 : Addressed Rob's comments from patch set 3 #
Total comments: 16
Patch Set 5 : Addressed Rob's comments for patch set 4 #Patch Set 6 : Addressed Rob's comments for patch set 55 #Patch Set 7 : Merged changes with trunk #
Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/251193005/diff/1/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/251193005/diff/1/ash/ash.gyp#newcode805 ash/ash.gyp:805: 'test/dummy_volume_control_delegate.cc', Place in alphabetical order by file name. So the cc file above the h file. https://codereview.chromium.org/251193005/diff/1/ash/test/dummy_volume_contro... File ash/test/dummy_volume_control_delegate.cc (right): https://codereview.chromium.org/251193005/diff/1/ash/test/dummy_volume_contro... ash/test/dummy_volume_control_delegate.cc:39: int DummyVolumeControlDelegate::handle_volume_mute_count() const { Accessor methods like this can be done in the header file. https://codereview.chromium.org/251193005/diff/1/ash/test/dummy_volume_contro... File ash/test/dummy_volume_control_delegate.h (right): https://codereview.chromium.org/251193005/diff/1/ash/test/dummy_volume_contro... ash/test/dummy_volume_control_delegate.h:18: virtual bool HandleVolumeMute(const ui::Accelerator& accelerator) OVERRIDE; Above a block of overridden methods leave a comment for which class they extend: // ash::VolumeControlDelegate: https://codereview.chromium.org/251193005/diff/1/ash/test/dummy_volume_contro... ash/test/dummy_volume_control_delegate.h:22: int handle_volume_mute_count() const; Overridden methods should come last. So these should be placed above https://codereview.chromium.org/251193005/diff/1/ash/test/dummy_volume_contro... ash/test/dummy_volume_control_delegate.h:28: const bool consume_; We don't really use const on class member variables. This one is actually set in the constructor, so it could become a normal member variable. For constants we defined them within the cc file in an anonymous namespace. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names https://codereview.chromium.org/251193005/diff/1/ash/wm/maximize_mode/maximiz... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/251193005/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:348: EXPECT_EQ(0, volume_delegate->handle_volume_down_count()); Prerequisite to the test, use ASSERT
https://codereview.chromium.org/251193005/diff/1/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/251193005/diff/1/ash/ash.gyp#newcode805 ash/ash.gyp:805: 'test/dummy_volume_control_delegate.cc', On 2014/04/29 14:42:43, jonross wrote: > Place in alphabetical order by file name. So the cc file above the h file. Done. https://codereview.chromium.org/251193005/diff/1/ash/test/dummy_volume_contro... File ash/test/dummy_volume_control_delegate.cc (right): https://codereview.chromium.org/251193005/diff/1/ash/test/dummy_volume_contro... ash/test/dummy_volume_control_delegate.cc:39: int DummyVolumeControlDelegate::handle_volume_mute_count() const { On 2014/04/29 14:42:43, jonross wrote: > Accessor methods like this can be done in the header file. Done. https://codereview.chromium.org/251193005/diff/1/ash/test/dummy_volume_contro... File ash/test/dummy_volume_control_delegate.h (right): https://codereview.chromium.org/251193005/diff/1/ash/test/dummy_volume_contro... ash/test/dummy_volume_control_delegate.h:18: virtual bool HandleVolumeMute(const ui::Accelerator& accelerator) OVERRIDE; On 2014/04/29 14:42:43, jonross wrote: > Above a block of overridden methods leave a comment for which class they extend: > > // ash::VolumeControlDelegate: Done. https://codereview.chromium.org/251193005/diff/1/ash/test/dummy_volume_contro... ash/test/dummy_volume_control_delegate.h:22: int handle_volume_mute_count() const; On 2014/04/29 14:42:43, jonross wrote: > Overridden methods should come last. So these should be placed above Done. https://codereview.chromium.org/251193005/diff/1/ash/test/dummy_volume_contro... ash/test/dummy_volume_control_delegate.h:28: const bool consume_; On 2014/04/29 14:42:43, jonross wrote: > We don't really use const on class member variables. This one is actually set in > the constructor, so it could become a normal member variable. > > For constants we defined them within the cc file in an anonymous namespace. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names Done. https://codereview.chromium.org/251193005/diff/1/ash/wm/maximize_mode/maximiz... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/251193005/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:348: EXPECT_EQ(0, volume_delegate->handle_volume_down_count()); On 2014/04/29 14:42:43, jonross wrote: > Prerequisite to the test, use ASSERT Done.
One nit, and one naming comment. Otherwise it looks like you can bring flackr@ into the review. https://codereview.chromium.org/251193005/diff/20001/ash/test/dummy_volume_co... File ash/test/dummy_volume_control_delegate.h (right): https://codereview.chromium.org/251193005/diff/20001/ash/test/dummy_volume_co... ash/test/dummy_volume_control_delegate.h:13: class DummyVolumeControlDelegate : public ash::VolumeControlDelegate { Not a big fan of calling this a 'Dummy' Really feels more like a substitute for what can be done with GMock, except that we don't use those. There are other unittests with call counters, see if we have a naming convention used for those that might be more appropriate. https://codereview.chromium.org/251193005/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_event_blocker.cc (right): https://codereview.chromium.org/251193005/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_event_blocker.cc:64: keyEvent->key_code() != ui::VKEY_VOLUME_UP) { Since this is a partial fix of the bug, add a todo for what we actually want. Mention the changes to the event that you are waiting for, and how this will restrict the keys further then. Format: // TODO(bruthig):
https://codereview.chromium.org/251193005/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_event_blocker.cc (right): https://codereview.chromium.org/251193005/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_event_blocker.cc:64: keyEvent->key_code() != ui::VKEY_VOLUME_UP) { On 2014/04/29 17:32:09, jonross wrote: > Since this is a partial fix of the bug, add a todo for what we actually want. > Mention the changes to the event that you are waiting for, and how this will > restrict the keys further then. Format: > > // TODO(bruthig): Done.
Generally looks good. Can you update the issue subject and description? Subject and first line of the description should be a single sentence summary with normal sentence capitalization, i.e. "Enable volume buttons during maximize mode" In the description, you can follow this with a paragraph describing more detail if necessary, then the BUG and TEST lines, but TEST= should only name the tests which verify this fix, so in this case TEST=MaximizeModeControllerTest.AllowsVolumeKeys. https://codereview.chromium.org/251193005/diff/40001/ash/test/volume_control_... File ash/test/volume_control_delegate_stub.cc (right): https://codereview.chromium.org/251193005/diff/40001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.cc:10: : consume_(consume), indent ':' 4 spaces, everything, keep variables lined up (i.e. all following lines indented 6). https://codereview.chromium.org/251193005/diff/40001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.cc:19: const ui::Accelerator& accelerator) { Here and below, indent method parameters 4 spaces. https://codereview.chromium.org/251193005/diff/40001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.cc:40: nit: no extra newline at end of file. https://codereview.chromium.org/251193005/diff/40001/ash/test/volume_control_... File ash/test/volume_control_delegate_stub.h (right): https://codereview.chromium.org/251193005/diff/40001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. Copyright style no longer has the (c): http://www.chromium.org/developers/coding-style#TOC-File-headers https://codereview.chromium.org/251193005/diff/40001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_event_blocker.cc (right): https://codereview.chromium.org/251193005/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_event_blocker.cc:11: #include "ui/events/keycodes/keyboard_codes_posix.h" This is only for posix OS's, include "ui/events/keycodes/keyboard_codes.h" which will bring in the right keycodes file for the OS. https://codereview.chromium.org/251193005/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_event_blocker.cc:62: // TODO(bruthig): This is a partial fix to issue 364584. nit: use a link to the issue, i.e. http://crbug.com/364584. Also, generally the first line of the todo should be what there is to do. I.e. TODO(bruthig): Don't allow remapped volume function keys (i.e. F9, F10) in maximized mode, only physical volume media keys, http://crbug.com/364584. https://codereview.chromium.org/251193005/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_event_blocker.cc:68: ui::KeyEvent* keyEvent = static_cast<ui::KeyEvent*>(event); s/keyEvent/key_event i.e. unix_hacker_style names for local variables.
https://codereview.chromium.org/251193005/diff/40001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_event_blocker.cc (right): https://codereview.chromium.org/251193005/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_event_blocker.cc:62: // TODO(bruthig): This is a partial fix to issue 364584. On 2014/04/29 21:43:54, flackr wrote: > nit: use a link to the issue, i.e. http://crbug.com/364584. Also, generally the > first line of the todo should be what there is to do. I.e. TODO(bruthig): Don't > allow remapped volume function keys (i.e. F9, F10) in maximized mode, only > physical volume media keys, http://crbug.com/364584. Also, if you're planning on closing the main bug, then create a new bug for this special case and reference it here.
https://codereview.chromium.org/251193005/diff/40001/ash/test/volume_control_... File ash/test/volume_control_delegate_stub.cc (right): https://codereview.chromium.org/251193005/diff/40001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.cc:10: : consume_(consume), On 2014/04/29 21:43:54, flackr wrote: > indent ':' 4 spaces, everything, keep variables lined up (i.e. all following > lines indented 6). Done. https://codereview.chromium.org/251193005/diff/40001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.cc:19: const ui::Accelerator& accelerator) { On 2014/04/29 21:43:54, flackr wrote: > Here and below, indent method parameters 4 spaces. Done. https://codereview.chromium.org/251193005/diff/40001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.cc:40: On 2014/04/29 21:43:54, flackr wrote: > nit: no extra newline at end of file. Done. https://codereview.chromium.org/251193005/diff/40001/ash/test/volume_control_... File ash/test/volume_control_delegate_stub.h (right): https://codereview.chromium.org/251193005/diff/40001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/04/29 21:43:54, flackr wrote: > Copyright style no longer has the (c): > http://www.chromium.org/developers/coding-style#TOC-File-headers Done. https://codereview.chromium.org/251193005/diff/40001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_event_blocker.cc (right): https://codereview.chromium.org/251193005/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_event_blocker.cc:11: #include "ui/events/keycodes/keyboard_codes_posix.h" On 2014/04/29 21:43:54, flackr wrote: > This is only for posix OS's, include "ui/events/keycodes/keyboard_codes.h" which > will bring in the right keycodes file for the OS. Done. https://codereview.chromium.org/251193005/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_event_blocker.cc:62: // TODO(bruthig): This is a partial fix to issue 364584. On 2014/04/29 21:49:24, flackr wrote: > On 2014/04/29 21:43:54, flackr wrote: > > nit: use a link to the issue, i.e. http://crbug.com/364584. Also, generally > the > > first line of the todo should be what there is to do. I.e. TODO(bruthig): > Don't > > allow remapped volume function keys (i.e. F9, F10) in maximized mode, only > > physical volume media keys, http://crbug.com/364584. > > Also, if you're planning on closing the main bug, then create a new bug for this > special case and reference it here. Done. https://codereview.chromium.org/251193005/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_event_blocker.cc:62: // TODO(bruthig): This is a partial fix to issue 364584. On 2014/04/29 21:43:54, flackr wrote: > nit: use a link to the issue, i.e. http://crbug.com/364584. Also, generally the > first line of the todo should be what there is to do. I.e. TODO(bruthig): Don't > allow remapped volume function keys (i.e. F9, F10) in maximized mode, only > physical volume media keys, http://crbug.com/364584. Done. https://codereview.chromium.org/251193005/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_event_blocker.cc:68: ui::KeyEvent* keyEvent = static_cast<ui::KeyEvent*>(event); On 2014/04/29 21:43:54, flackr wrote: > s/keyEvent/key_event > i.e. unix_hacker_style names for local variables. Done.
https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... File ash/test/volume_control_delegate_stub.cc (right): https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.cc:16: VolumeControlDelegateStub::~VolumeControlDelegateStub() {} nit: We tend to only use {} for empty inlined functions. Put the closing '}' on the next line. https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... File ash/test/volume_control_delegate_stub.h (right): https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.h:13: // A simple TestDouble for a VolumeControlDelegate TestDouble? https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.h:16: // these methods. The predefined value is defined upon construction. No need to mention the predefined consume value. here. https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.h:17: class VolumeControlDelegateStub : public ash::VolumeControlDelegate { For consistency, it looks like the other mock implementations of delegates used for tests are called TestXDelegate, so how about TestVolumeControlDelegate? https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.h:44: bool consume_; Can comment what consume means here instead. https://codereview.chromium.org/251193005/diff/60001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_event_blocker.cc (right): https://codereview.chromium.org/251193005/diff/60001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_event_blocker.cc:62: // TODO(bruthig): Fix this to block volume events from the device's To make this a bit more clear, how about "to block rewritten volume keys (i.e. F9 and F10)"? https://codereview.chromium.org/251193005/diff/60001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_event_blocker.cc:63: // keboard. https://crbug.com/368669 nit: s/keboard/keyboard
https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... File ash/test/volume_control_delegate_stub.cc (right): https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.cc:16: VolumeControlDelegateStub::~VolumeControlDelegateStub() {} On 2014/04/30 14:27:44, flackr wrote: > nit: We tend to only use {} for empty inlined functions. Put the closing '}' on > the next line. Done. https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... File ash/test/volume_control_delegate_stub.h (right): https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.h:13: // A simple TestDouble for a VolumeControlDelegate On 2014/04/30 14:27:44, flackr wrote: > TestDouble? "A test double is an object that can stand in for a real object in a test" See https://big.corp.google.com/~jmcmaster/testing/2013/08/episode-296-know-your-... https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.h:16: // these methods. The predefined value is defined upon construction. On 2014/04/30 14:27:44, flackr wrote: > No need to mention the predefined consume value. here. Done. https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.h:17: class VolumeControlDelegateStub : public ash::VolumeControlDelegate { On 2014/04/30 14:27:44, flackr wrote: > For consistency, it looks like the other mock implementations of delegates used > for tests are called TestXDelegate, so how about TestVolumeControlDelegate? For consistency I've renamed it to TestVolumeControlDelegate but I may add this to my radar for some 20% work https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.h:17: class VolumeControlDelegateStub : public ash::VolumeControlDelegate { On 2014/04/30 14:27:44, flackr wrote: > For consistency, it looks like the other mock implementations of delegates used > for tests are called TestXDelegate, so how about TestVolumeControlDelegate? Done. https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.h:44: bool consume_; On 2014/04/30 14:27:44, flackr wrote: > Can comment what consume means here instead. Done. https://codereview.chromium.org/251193005/diff/60001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_event_blocker.cc (right): https://codereview.chromium.org/251193005/diff/60001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_event_blocker.cc:62: // TODO(bruthig): Fix this to block volume events from the device's On 2014/04/30 14:27:44, flackr wrote: > To make this a bit more clear, how about "to block rewritten volume keys (i.e. > F9 and F10)"? Done. https://codereview.chromium.org/251193005/diff/60001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_event_blocker.cc:63: // keboard. https://crbug.com/368669 On 2014/04/30 14:27:44, flackr wrote: > nit: s/keboard/keyboard Done.
LTGM On 2014/04/30 15:36:15, bruthig wrote: > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > File ash/test/volume_control_delegate_stub.cc (right): > > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > ash/test/volume_control_delegate_stub.cc:16: > VolumeControlDelegateStub::~VolumeControlDelegateStub() {} > On 2014/04/30 14:27:44, flackr wrote: > > nit: We tend to only use {} for empty inlined functions. Put the closing '}' > on > > the next line. > > Done. > > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > File ash/test/volume_control_delegate_stub.h (right): > > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > ash/test/volume_control_delegate_stub.h:13: // A simple TestDouble for a > VolumeControlDelegate > On 2014/04/30 14:27:44, flackr wrote: > > TestDouble? > > "A test double is an object that can stand in for a real object in a test" > > See > https://big.corp.google.com/~jmcmaster/testing/2013/08/episode-296-know-your-... > > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > ash/test/volume_control_delegate_stub.h:16: // these methods. The predefined > value is defined upon construction. > On 2014/04/30 14:27:44, flackr wrote: > > No need to mention the predefined consume value. here. > > Done. > > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > ash/test/volume_control_delegate_stub.h:17: class VolumeControlDelegateStub : > public ash::VolumeControlDelegate { > On 2014/04/30 14:27:44, flackr wrote: > > For consistency, it looks like the other mock implementations of delegates > used > > for tests are called TestXDelegate, so how about TestVolumeControlDelegate? > > For consistency I've renamed it to TestVolumeControlDelegate but I may add this > to my radar for some 20% work > > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > ash/test/volume_control_delegate_stub.h:17: class VolumeControlDelegateStub : > public ash::VolumeControlDelegate { > On 2014/04/30 14:27:44, flackr wrote: > > For consistency, it looks like the other mock implementations of delegates > used > > for tests are called TestXDelegate, so how about TestVolumeControlDelegate? > > Done. > > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > ash/test/volume_control_delegate_stub.h:44: bool consume_; > On 2014/04/30 14:27:44, flackr wrote: > > Can comment what consume means here instead. > > Done. > > https://codereview.chromium.org/251193005/diff/60001/ash/wm/maximize_mode/max... > File ash/wm/maximize_mode/maximize_mode_event_blocker.cc (right): > > https://codereview.chromium.org/251193005/diff/60001/ash/wm/maximize_mode/max... > ash/wm/maximize_mode/maximize_mode_event_blocker.cc:62: // TODO(bruthig): Fix > this to block volume events from the device's > On 2014/04/30 14:27:44, flackr wrote: > > To make this a bit more clear, how about "to block rewritten volume keys (i.e. > > F9 and F10)"? > > Done. > > https://codereview.chromium.org/251193005/diff/60001/ash/wm/maximize_mode/max... > ash/wm/maximize_mode/maximize_mode_event_blocker.cc:63: // keboard. > https://crbug.com/368669 > On 2014/04/30 14:27:44, flackr wrote: > > nit: s/keboard/keyboard > > Done.
On 2014/04/30 15:39:32, jonross wrote: > LTGM > > On 2014/04/30 15:36:15, bruthig wrote: > > > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > > File ash/test/volume_control_delegate_stub.cc (right): > > > > > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > > ash/test/volume_control_delegate_stub.cc:16: > > VolumeControlDelegateStub::~VolumeControlDelegateStub() {} > > On 2014/04/30 14:27:44, flackr wrote: > > > nit: We tend to only use {} for empty inlined functions. Put the closing '}' > > on > > > the next line. > > > > Done. > > > > > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > > File ash/test/volume_control_delegate_stub.h (right): > > > > > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > > ash/test/volume_control_delegate_stub.h:13: // A simple TestDouble for a > > VolumeControlDelegate > > On 2014/04/30 14:27:44, flackr wrote: > > > TestDouble? > > > > "A test double is an object that can stand in for a real object in a test" > > > > See > > > https://big.corp.google.com/~jmcmaster/testing/2013/08/episode-296-know-your-... > > > > > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > > ash/test/volume_control_delegate_stub.h:16: // these methods. The predefined > > value is defined upon construction. > > On 2014/04/30 14:27:44, flackr wrote: > > > No need to mention the predefined consume value. here. > > > > Done. > > > > > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > > ash/test/volume_control_delegate_stub.h:17: class VolumeControlDelegateStub : > > public ash::VolumeControlDelegate { > > On 2014/04/30 14:27:44, flackr wrote: > > > For consistency, it looks like the other mock implementations of delegates > > used > > > for tests are called TestXDelegate, so how about TestVolumeControlDelegate? > > > > For consistency I've renamed it to TestVolumeControlDelegate but I may add > this > > to my radar for some 20% work > > > > > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > > ash/test/volume_control_delegate_stub.h:17: class VolumeControlDelegateStub : > > public ash::VolumeControlDelegate { > > On 2014/04/30 14:27:44, flackr wrote: > > > For consistency, it looks like the other mock implementations of delegates > > used > > > for tests are called TestXDelegate, so how about TestVolumeControlDelegate? > > > > Done. > > > > > https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... > > ash/test/volume_control_delegate_stub.h:44: bool consume_; > > On 2014/04/30 14:27:44, flackr wrote: > > > Can comment what consume means here instead. > > > > Done. > > > > > https://codereview.chromium.org/251193005/diff/60001/ash/wm/maximize_mode/max... > > File ash/wm/maximize_mode/maximize_mode_event_blocker.cc (right): > > > > > https://codereview.chromium.org/251193005/diff/60001/ash/wm/maximize_mode/max... > > ash/wm/maximize_mode/maximize_mode_event_blocker.cc:62: // TODO(bruthig): Fix > > this to block volume events from the device's > > On 2014/04/30 14:27:44, flackr wrote: > > > To make this a bit more clear, how about "to block rewritten volume keys > (i.e. > > > F9 and F10)"? > > > > Done. > > > > > https://codereview.chromium.org/251193005/diff/60001/ash/wm/maximize_mode/max... > > ash/wm/maximize_mode/maximize_mode_event_blocker.cc:63: // keboard. > > https://crbug.com/368669 > > On 2014/04/30 14:27:44, flackr wrote: > > > nit: s/keboard/keyboard > > > > Done. LGTM
LGTM with nit https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... File ash/test/volume_control_delegate_stub.h (right): https://codereview.chromium.org/251193005/diff/60001/ash/test/volume_control_... ash/test/volume_control_delegate_stub.h:13: // A simple TestDouble for a VolumeControlDelegate On 2014/04/30 15:36:16, bruthig wrote: > On 2014/04/30 14:27:44, flackr wrote: > > TestDouble? > > "A test double is an object that can stand in for a real object in a test" > > See > https://big.corp.google.com/~jmcmaster/testing/2013/08/episode-296-know-your-... Ah okay, I hadn't heard the term before and the TitleCase made me think it was a chromium class / type. Can you call it a test touble rather than a TestDouble?
oshima@chromium.org: Please review changes in
lgtm
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bruthig@chromium.org/251193005/100001
Message was sent while issue was closed.
Change committed as 268181 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
