Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
472603004:
Add DeviceEventRouter class. (Closed)
|
Created:
6 years, 4 months ago by hirono Modified:
6 years, 4 months ago Reviewers:
mtomasz CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, yoshiki+watch_chromium.org, rginda+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd DeviceEventRouter class.
The class is extracted from EventRouter of Files.app.
It is going to manage device states and dispatch the device events.
BUG=360946, 396258
TEST=add unit tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291384
Patch Set 1 : #
Total comments: 43
Patch Set 2 : #Patch Set 3 : Remove PostDelayedTask viertual function #
Total comments: 14
Patch Set 4 : . #
Total comments: 8
Patch Set 5 : Fixed. #
Total comments: 6
Patch Set 6 : . #
Total comments: 6
Patch Set 7 : . #
Messages
Total messages: 18 (0 generated)
PTAL the CL? This is the first patch of revised version of crrev.com/476063002. Thank you!
https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/device_event_router.cc (right): https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:28: PostDelayedTask(base::Bind(&DeviceEventRouter::StartupDelayed, PostDelayedTask seems to be not implemented. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:41: if (is_startup_ || is_suspended_) { is_suspended_ is true while we're are not suspended anymore. How about renaming to "resuming"? https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/device_event_router.h (right): https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: no (c). https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:18: enum DeviceState { nit: AFAIK we usually put extra \n after opening and before closing a namespace unless its short. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:55: virtual void OnHardUnplugged(const std::string& device_path) OVERRIDE {} nit: Shall implementation be in the .cc file? https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:66: // Returns external storage is disabled or not. nit: We usually put \n between methods if there is a comment for a method, so it's easier to see that the comment applies to which method. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:69: virtual void PostDelayedTask(const base::Closure& callback, Do we need it? We could just normally post a task and use RunLoop->RunUntilIdle() in tests to wait until they are executed. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:82: // Whether the profile was just startupped or not. nit: startupped -> starting up nit: is_startup_ -> is_starting_up_? WDYT? https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc (right): https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. nit: 214 https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:11: #include "base/prefs/pref_service.h" pref_service not used? https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:12: #include "base/strings/utf_string_conversions.h" utf_string_conversion not used? https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:14: #include "chrome/browser/chromeos/file_manager/volume_manager.h" volume_manager not used? https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:16: #include "chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h" file_system provider not used? https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:18: #include "chrome/common/pref_names.h" pref_names not used? https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:20: #include "chromeos/dbus/fake_power_manager_client.h" if fake_power_manager_client used? https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:22: #include "components/storage_monitor/storage_info.h" is storage_info used? https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:23: #include "content/public/test/test_browser_thread_bundle.h" bundle not used? https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:24: #include "extensions/browser/extension_registry.h" extension_registry not used? https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:40: class DeviceEventRouterImpl : public DeviceEventRouter { Comment for the class and methods missing. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:74: }; How about adding DISALLOW_COPY_AND_ASSIGN?
Thank you! https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/device_event_router.cc (right): https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:28: PostDelayedTask(base::Bind(&DeviceEventRouter::StartupDelayed, On 2014/08/20 06:31:01, mtomasz wrote: > PostDelayedTask seems to be not implemented. Yes, the method is implemented in sub-classes. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:41: if (is_startup_ || is_suspended_) { On 2014/08/20 06:31:01, mtomasz wrote: > is_suspended_ is true while we're are not suspended anymore. How about renaming > to "resuming"? Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/device_event_router.h (right): https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/08/20 06:31:01, mtomasz wrote: > nit: no (c). Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:18: enum DeviceState { On 2014/08/20 06:31:01, mtomasz wrote: > nit: > AFAIK we usually put extra \n after opening and before closing a namespace > unless its short. > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:55: virtual void OnHardUnplugged(const std::string& device_path) OVERRIDE {} On 2014/08/20 06:31:01, mtomasz wrote: > nit: Shall implementation be in the .cc file? Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:66: // Returns external storage is disabled or not. On 2014/08/20 06:31:01, mtomasz wrote: > nit: We usually put \n between methods if there is a comment for a method, so > it's easier to see that the comment applies to which method. > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:69: virtual void PostDelayedTask(const base::Closure& callback, On 2014/08/20 06:31:01, mtomasz wrote: > Do we need it? We could just normally post a task and use > RunLoop->RunUntilIdle() in tests to wait until they are executed. I checked, but the method looks deprecated. https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/... It also does not work for delayed tasks. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:82: // Whether the profile was just startupped or not. On 2014/08/20 06:31:02, mtomasz wrote: > nit: startupped -> starting up > nit: is_startup_ -> is_starting_up_? > WDYT? Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc (right): https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/08/20 06:31:02, mtomasz wrote: > nit: 214 Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:11: #include "base/prefs/pref_service.h" On 2014/08/20 06:31:02, mtomasz wrote: > pref_service not used? Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:12: #include "base/strings/utf_string_conversions.h" On 2014/08/20 06:31:02, mtomasz wrote: > utf_string_conversion not used? Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:14: #include "chrome/browser/chromeos/file_manager/volume_manager.h" On 2014/08/20 06:31:02, mtomasz wrote: > volume_manager not used? Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:16: #include "chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h" On 2014/08/20 06:31:02, mtomasz wrote: > file_system provider not used? Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:18: #include "chrome/common/pref_names.h" On 2014/08/20 06:31:02, mtomasz wrote: > pref_names not used? Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:20: #include "chromeos/dbus/fake_power_manager_client.h" On 2014/08/20 06:31:02, mtomasz wrote: > if fake_power_manager_client used? Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:22: #include "components/storage_monitor/storage_info.h" On 2014/08/20 06:31:02, mtomasz wrote: > is storage_info used? Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:23: #include "content/public/test/test_browser_thread_bundle.h" On 2014/08/20 06:31:02, mtomasz wrote: > bundle not used? Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:24: #include "extensions/browser/extension_registry.h" On 2014/08/20 06:31:02, mtomasz wrote: > extension_registry not used? Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:40: class DeviceEventRouterImpl : public DeviceEventRouter { On 2014/08/20 06:31:02, mtomasz wrote: > Comment for the class and methods missing. Done. https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:74: }; On 2014/08/20 06:31:02, mtomasz wrote: > How about adding DISALLOW_COPY_AND_ASSIGN? Done.
https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/device_event_router.cc (right): https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:28: PostDelayedTask(base::Bind(&DeviceEventRouter::StartupDelayed, On 2014/08/20 08:07:16, hirono wrote: > On 2014/08/20 06:31:01, mtomasz wrote: > > PostDelayedTask seems to be not implemented. > > Yes, the method is implemented in sub-classes. The Impl seems not included in this CL, is it OK? https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/device_event_router.h (right): https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:69: virtual void PostDelayedTask(const base::Closure& callback, On 2014/08/20 08:07:16, hirono wrote: > On 2014/08/20 06:31:01, mtomasz wrote: > > Do we need it? We could just normally post a task and use > > RunLoop->RunUntilIdle() in tests to wait until they are executed. > > I checked, but the method looks deprecated. > https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/... > > It also does not work for delayed tasks. Indeed it is deprecated, base::RunLoop should be used instead: https://code.google.com/p/chromium/codesearch#chromium/src/base/run_loop.h&l=48 It should work with delayed tasks, at least it worked for me: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
I removed PostDelayedTask function. PTAL again? Thank you! https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/device_event_router.cc (right): https://codereview.chromium.org/472603004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:28: PostDelayedTask(base::Bind(&DeviceEventRouter::StartupDelayed, On 2014/08/20 08:20:17, mtomasz wrote: > On 2014/08/20 08:07:16, hirono wrote: > > On 2014/08/20 06:31:01, mtomasz wrote: > > > PostDelayedTask seems to be not implemented. > > > > Yes, the method is implemented in sub-classes. > > The Impl seems not included in this CL, is it OK? Yes, currently the class is used only from the unit test. The following CL modifies EventRouter and make it use the class.
https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.cc (right): https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:31: PostDelayedTask(base::Bind(&DeviceEventRouter::StartupDelayed, Is this PostDelayedTask method needed? https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:153: base::MessageLoopProxy::current()->PostDelayedTask( MessageLoopProxy is deprecated. Please use ThreadTaskRunnerHandle::Get(). https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.h (right): https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:34: explicit DeviceEventRouter(bool zero_time_delta = false); Default argument values are not allowed by our style guide. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul... https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:74: void PostDelayedTask(const base::Closure& closure, base::TimeDelta time); Is this still used? https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:81: DeviceState GetDeviceState(const std::string& device_path); Can this method be const? https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc (right): https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:190: } // namespace file_manager If empty line is in #17, then it also should be between #189 and #190.
https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.cc (right): https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:31: PostDelayedTask(base::Bind(&DeviceEventRouter::StartupDelayed, On 2014/08/21 08:32:46, mtomasz wrote: > Is this PostDelayedTask method needed? This is a helper method to call MessageLoop::PostDelayedTask with seeing zero_time_delta_ flag. https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:153: base::MessageLoopProxy::current()->PostDelayedTask( On 2014/08/21 08:32:46, mtomasz wrote: > MessageLoopProxy is deprecated. Please use ThreadTaskRunnerHandle::Get(). Done. https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.h (right): https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:34: explicit DeviceEventRouter(bool zero_time_delta = false); On 2014/08/21 08:32:47, mtomasz wrote: > Default argument values are not allowed by our style guide. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul... Done. https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:74: void PostDelayedTask(const base::Closure& closure, base::TimeDelta time); On 2014/08/21 08:32:47, mtomasz wrote: > Is this still used? This is a helper method to call MessageLoop::PostDelayedTask with seeing zero_time_delta_ flag. https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:81: DeviceState GetDeviceState(const std::string& device_path); On 2014/08/21 08:32:47, mtomasz wrote: > Can this method be const? Done. https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc (right): https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:190: } // namespace file_manager On 2014/08/21 08:32:47, mtomasz wrote: > If empty line is in #17, then it also should be between #189 and #190. Done.
https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.cc (right): https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:31: PostDelayedTask(base::Bind(&DeviceEventRouter::StartupDelayed, On 2014/08/21 08:48:32, hirono wrote: > On 2014/08/21 08:32:46, mtomasz wrote: > > Is this PostDelayedTask method needed? > > This is a helper method to call MessageLoop::PostDelayedTask with seeing > zero_time_delta_ flag. Do we really need it? All it does is to determine the delay. It can either ignore the passed time span, or use it. But, if we had the time span in a member value, then we wouldn't need this method at all. We could set value of this member value in the constructor. Now: private: bool zero_time_delta_; Suggested: private: base::TimeSpan resume_time_delta_; WDYT? https://codereview.chromium.org/472603004/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.cc (right): https://codereview.chromium.org/472603004/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:15: const base::TimeDelta kStartupTimeSpan = base::TimeDelta::FromSeconds(10); Objects of class types are not allowed as global/static variables. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static... https://codereview.chromium.org/472603004/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:154: base::MessageLoopProxy::current()->PostDelayedTask( ditto https://codereview.chromium.org/472603004/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.h (right): https://codereview.chromium.org/472603004/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:34: explicit DeviceEventRouter(bool zero_time_delta); Passing false/true to a constructor doesn't say much, and requires looking into the header file every time. How about having two constructors? One without arguments, and second with base::TimeDelta as an argument? As a result we could easily get rid of the helper method. WDYT?
https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.cc (right): https://codereview.chromium.org/472603004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:31: PostDelayedTask(base::Bind(&DeviceEventRouter::StartupDelayed, On 2014/08/22 04:50:10, mtomasz wrote: > On 2014/08/21 08:48:32, hirono wrote: > > On 2014/08/21 08:32:46, mtomasz wrote: > > > Is this PostDelayedTask method needed? > > > > This is a helper method to call MessageLoop::PostDelayedTask with seeing > > zero_time_delta_ flag. > > Do we really need it? All it does is to determine the delay. It can either > ignore the passed time span, or use it. > > But, if we had the time span in a member value, then we wouldn't need this > method at all. We could set value of this member value in the constructor. > > Now: > private: > bool zero_time_delta_; > > Suggested: > private: > base::TimeSpan resume_time_delta_; > > WDYT? Does not have strong idea. It looks 1 helper method and 1 flag, vs 3 private variables. Anyway, let me follow this. https://codereview.chromium.org/472603004/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.cc (right): https://codereview.chromium.org/472603004/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:15: const base::TimeDelta kStartupTimeSpan = base::TimeDelta::FromSeconds(10); On 2014/08/22 04:50:10, mtomasz wrote: > Objects of class types are not allowed as global/static variables. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static... Done. https://codereview.chromium.org/472603004/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:154: base::MessageLoopProxy::current()->PostDelayedTask( On 2014/08/22 04:50:10, mtomasz wrote: > ditto Done. https://codereview.chromium.org/472603004/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.h (right): https://codereview.chromium.org/472603004/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:34: explicit DeviceEventRouter(bool zero_time_delta); On 2014/08/22 04:50:10, mtomasz wrote: > Passing false/true to a constructor doesn't say much, and requires looking into > the header file every time. How about having two constructors? One without > arguments, and second with base::TimeDelta as an argument? As a result we could > easily get rid of the helper method. WDYT? Done.
https://codereview.chromium.org/472603004/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.cc (right): https://codereview.chromium.org/472603004/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:65: DCHECK_CURRENTLY_ON(BrowserThread::UI); Why are the thread checks gone? If they don't work in test, consider using base::ThreadChecker. https://codereview.chromium.org/472603004/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.cc (right): https://codereview.chromium.org/472603004/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:18: : resume_time_delta_(base::TimeDelta::FromSeconds(5)), nit: These should be constants (int). https://codereview.chromium.org/472603004/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:40: base::MessageLoopProxy::current()->PostDelayedTask( MessageLoopProxy is deprecated. Please use base::ThreadTaskRunnerHandle::Get(). https://codereview.chromium.org/472603004/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.h (right): https://codereview.chromium.org/472603004/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:33: // If |zero_time_delta| is true, it overrides time delta of delayed tasks with nit: \n after DeviceEvectRouter(); for consistency?
https://codereview.chromium.org/472603004/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.cc (right): https://codereview.chromium.org/472603004/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:65: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2014/08/22 05:48:40, mtomasz wrote: > Why are the thread checks gone? If they don't work in test, consider using > base::ThreadChecker. Done. https://codereview.chromium.org/472603004/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.cc (right): https://codereview.chromium.org/472603004/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:18: : resume_time_delta_(base::TimeDelta::FromSeconds(5)), On 2014/08/22 05:48:40, mtomasz wrote: > nit: These should be constants (int). After offline discussion, it won't fix. * resume_time_delta_ is also constant of the instance. * The value '5' is used only once, and it is named with 'resume_time_delta_'. https://codereview.chromium.org/472603004/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.cc:40: base::MessageLoopProxy::current()->PostDelayedTask( On 2014/08/22 05:48:40, mtomasz wrote: > MessageLoopProxy is deprecated. Please use base::ThreadTaskRunnerHandle::Get(). Oops, the lines were mixed from old lines. https://codereview.chromium.org/472603004/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router.h (right): https://codereview.chromium.org/472603004/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router.h:33: // If |zero_time_delta| is true, it overrides time delta of delayed tasks with On 2014/08/22 05:48:40, mtomasz wrote: > nit: \n after DeviceEvectRouter(); for consistency? Done.
small nits https://codereview.chromium.org/472603004/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc (right): https://codereview.chromium.org/472603004/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:36: // Override nit: ... -> // DeviceEventRouter overrrides. https://codereview.chromium.org/472603004/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:59: } // namespace nit: \n between #58 and #59 since there is \n in #17. https://codereview.chromium.org/472603004/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:182: EXPECT_EQ(2u, device_event_router->events.size()); It's better to use ASSERT_EQ for checking array sizes, before accessing arrays, in order to avoid crashes.
https://codereview.chromium.org/472603004/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc (right): https://codereview.chromium.org/472603004/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:36: // Override On 2014/08/22 08:09:07, mtomasz wrote: > nit: ... -> // DeviceEventRouter overrrides. Done. https://codereview.chromium.org/472603004/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:59: } // namespace On 2014/08/22 08:09:07, mtomasz wrote: > nit: \n between #58 and #59 since there is \n in #17. Done. https://codereview.chromium.org/472603004/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/device_event_router_unittest.cc:182: EXPECT_EQ(2u, device_event_router->events.size()); On 2014/08/22 08:09:07, mtomasz wrote: > It's better to use ASSERT_EQ for checking array sizes, before accessing arrays, > in order to avoid crashes. Done.
lgtm!
The CQ bit was checked by hirono@chromium.org
On 2014/08/22 09:31:05, mtomasz wrote: > lgtm! Thank you for much improvements!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/472603004/180001
Message was sent while issue was closed.
Committed patchset #7 (180001) as 291384 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
