Files.app: Start to use DeviceEventRouter.

Files.app: Start to use DeviceEventRouter.

The new class manages device states and dispatch device events property instead
of EventRouter, MountedDiskMonitor, and JavaScript side's DeviceHandler.

BUG=360946, 396258
Committed: https://crrev.com/0f3d08364420d5b3c6eaa4100799459277758f52
Cr-Commit-Position: refs/heads/master@{#291891}

[hirono, 2014-08-25 04:17:03]

PTAL the CL? Thank you!

[mtomasz, 2014-08-25 04:37:37]

https://codereview.chromium.org/490643005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/490643005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:328: :
DeviceEventRouter(base::TimeDelta::FromSeconds(0)), profile_(profile) {}
Shall 0 be passed here?

https://codereview.chromium.org/490643005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:330: //
Overrides
nit: ... -> DeviceEventRouter overrides. for consistency?

https://codereview.chromium.org/490643005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:348: Profile*
const profile_;
How about: DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/490643005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:441:
volume_manager->AddObserver(this);
Does EventRouter need to observe VolumeManager? Most (or all) observer's methods
seem to be empty. WDYT?

[hirono, 2014-08-25 06:40:58]

https://codereview.chromium.org/490643005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/490643005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:328: :
DeviceEventRouter(base::TimeDelta::FromSeconds(0)), profile_(profile) {}
On 2014/08/25 04:37:37, mtomasz wrote:
> Shall 0 be passed here?

No, it's a bug. Thank you for catching.

https://codereview.chromium.org/490643005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:330: //
Overrides
On 2014/08/25 04:37:37, mtomasz wrote:
> nit: ... -> DeviceEventRouter overrides. for consistency?

Done.

https://codereview.chromium.org/490643005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:348: Profile*
const profile_;
On 2014/08/25 04:37:36, mtomasz wrote:
> How about: DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/490643005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:441:
volume_manager->AddObserver(this);
On 2014/08/25 04:37:36, mtomasz wrote:
> Does EventRouter need to observe VolumeManager? Most (or all) observer's
methods
> seem to be empty. WDYT?

EventRouter needs to observe VolumeManager to catch mount events. Thus the mount
events are dispatched for other than devices (e.g. drive, zip), I'd like to keep
the handler in EventRouter, not in DeviceEventRouter.

[mtomasz, 2014-08-25 07:48:23]

lgtm with nits

https://codereview.chromium.org/490643005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/490643005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:441:
volume_manager->AddObserver(this);
On 2014/08/25 06:40:57, hirono wrote:
> On 2014/08/25 04:37:36, mtomasz wrote:
> > Does EventRouter need to observe VolumeManager? Most (or all) observer's
> methods
> > seem to be empty. WDYT?
> 
> EventRouter needs to observe VolumeManager to catch mount events. Thus the
mount
> events are dispatched for other than devices (e.g. drive, zip), I'd like to
keep
> the handler in EventRouter, not in DeviceEventRouter.

SGTM.

https://codereview.chromium.org/490643005/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/490643005/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:332:
file_browser_private::DeviceEvent event;
nit: How about adding a DCHECK if we are on the UI thread?

https://codereview.chromium.org/490643005/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:343: return
profile_->GetPrefs()->GetBoolean(prefs::kExternalStorageDisabled);
nit: ditto

[hirono, 2014-08-26 01:06:22]

Patchset #4 (id:100001) has been deleted

[hirono, 2014-08-26 01:06:45]

Thanks!

https://codereview.chromium.org/490643005/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/490643005/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:332:
file_browser_private::DeviceEvent event;
On 2014/08/25 07:48:23, mtomasz wrote:
> nit: How about adding a DCHECK if we are on the UI thread?

Done.

https://codereview.chromium.org/490643005/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:343: return
profile_->GetPrefs()->GetBoolean(prefs::kExternalStorageDisabled);
On 2014/08/25 07:48:23, mtomasz wrote:
> nit: ditto

Done.

[hirono, 2014-08-26 01:06:49]

The CQ bit was checked by hirono@chromium.org

[commit-bot: I haz the power, 2014-08-26 01:09:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hirono@chromium.org/490643005/120001

[hirono, 2014-08-26 01:23:02]

The CQ bit was unchecked by hirono@chromium.org

[hirono, 2014-08-26 02:31:03]

@mtomasz - I fixed one condition in DeviceHandler, and tests for the class.
Please take another look? Thank you!

[mtomasz, 2014-08-26 02:49:43]

https://codereview.chromium.org/490643005/diff/140001/chrome/test/data/file_m...
File chrome/test/data/file_manager/unit_tests/device_handler_unittest.js
(right):

https://codereview.chromium.org/490643005/diff/140001/chrome/test/data/file_m...
chrome/test/data/file_manager/unit_tests/device_handler_unittest.js:354:
shouldNotify: true
nit: Do we have any unit test with shouldNotify = false?

[hirono, 2014-08-26 03:48:27]

I added the test, and remove startup notification blocking in JavaScript code.

https://codereview.chromium.org/490643005/diff/140001/chrome/test/data/file_m...
File chrome/test/data/file_manager/unit_tests/device_handler_unittest.js
(right):

https://codereview.chromium.org/490643005/diff/140001/chrome/test/data/file_m...
chrome/test/data/file_manager/unit_tests/device_handler_unittest.js:354:
shouldNotify: true
On 2014/08/26 02:49:43, mtomasz wrote:
> nit: Do we have any unit test with shouldNotify = false?

Done.

[mtomasz, 2014-08-26 03:52:46]

lgtm!

[hirono, 2014-08-26 03:53:10]

The CQ bit was checked by hirono@chromium.org

[hirono, 2014-08-26 03:53:19]

On 2014/08/26 03:52:46, mtomasz wrote:
> lgtm!

Thank you!

[commit-bot: I haz the power, 2014-08-26 03:54:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hirono@chromium.org/490643005/160001

[commit-bot: I haz the power, 2014-08-26 04:13:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  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/46184)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/51451)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-26 04:15:38]

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

[commit-bot: I haz the power, 2014-08-26 04:15:39]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/46189)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hirono, 2014-08-26 04:21:02]

hirono@chromium.org changed reviewers:
+ benwells@chromium.org

[hirono, 2014-08-26 04:21:02]

@benwells - PTAL file_browser_private.idl? Thank you!

[benwells, 2014-08-26 07:25:11]

lgtm

[hirono, 2014-08-26 07:50:37]

The CQ bit was checked by hirono@chromium.org

[commit-bot: I haz the power, 2014-08-26 07:51:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hirono@chromium.org/490643005/180001

[commit-bot: I haz the power, 2014-08-26 08:51:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-26 10:08:46]

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

[commit-bot: I haz the power, 2014-08-26 10:08:47]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hirono, 2014-08-26 11:10:21]

The CQ bit was checked by hirono@chromium.org

[commit-bot: I haz the power, 2014-08-26 11:10:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hirono@chromium.org/490643005/180001

[commit-bot: I haz the power, 2014-08-26 12:26:05]

Committed patchset #7 (180001) as dbf74d923837e2a2208343898fe7f46c8b990242

[commit-bot: I haz the power, 2014-09-10 02:42:23]

Patchset 7 (id:??) landed as
https://crrev.com/0f3d08364420d5b3c6eaa4100799459277758f52
Cr-Commit-Position: refs/heads/master@{#291891}