Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
476913003:
Modify DeviceHandler to handle deviceFail event and add a message. (Closed)
|
Created:
6 years, 4 months ago by iseki Modified:
6 years, 3 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, yoshiki+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionModify DeviceHandler to handle deviceFail event and add a message.
BUG=317998
TEST=manually
Committed: https://crrev.com/b39db9f1b80b40bd04acdf621f515b95c26d990d
Cr-Commit-Position: refs/heads/master@{#291670}
Patch Set 1 #Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 7
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #Patch Set 7 : git branch #
Messages
Total messages: 26 (0 generated)
Please take a look.
https://codereview.chromium.org/476913003/diff/20001/chrome/app/chromeos_stri... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/476913003/diff/20001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:3775: Format this device. Button label does not have a period. See #3759. https://codereview.chromium.org/476913003/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/device_handler.js (right): https://codereview.chromium.org/476913003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:353: else if (event.status === 'error_unknown_filesystem') { else should be put at #352 after '}'. https://codereview.chromium.org/476913003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:418: if (DeviceHandler.Notification.DEVICE_FAIL.buttonLabel) { Could you add new notification type (Notification.DEVICE_FAIL_UNKNOWN) instead changing buttonLabel? Because DeviceHandler.Notification.DEVICE_FAIL is global instance, I'd like keep it readonly. https://codereview.chromium.org/476913003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:441: 'DEVICE_UNKNOWN_BUTTON_LABEL' The depth of indent should be 4 when wrapping a line. https://codereview.chromium.org/476913003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:470: event.volumeId = this.navigationVolumes_[path]; Could you follow #470 and #471 after #465? Currently there is no reason to split it into two parts.
Thank you for reviewing. https://codereview.chromium.org/476913003/diff/20001/chrome/app/chromeos_stri... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/476913003/diff/20001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:3775: Format this device. On 2014/08/22 06:59:24, hirono wrote: > Button label does not have a period. See #3759. Done. https://codereview.chromium.org/476913003/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/device_handler.js (right): https://codereview.chromium.org/476913003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:353: else if (event.status === 'error_unknown_filesystem') { On 2014/08/22 06:59:24, hirono wrote: > else should be put at #352 after '}'. Done. https://codereview.chromium.org/476913003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:418: if (DeviceHandler.Notification.DEVICE_FAIL.buttonLabel) { On 2014/08/22 06:59:24, hirono wrote: > Could you add new notification type (Notification.DEVICE_FAIL_UNKNOWN) instead > changing buttonLabel? > Because DeviceHandler.Notification.DEVICE_FAIL is global instance, I'd like keep > it readonly. Done. https://codereview.chromium.org/476913003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:441: 'DEVICE_UNKNOWN_BUTTON_LABEL' On 2014/08/22 06:59:24, hirono wrote: > The depth of indent should be 4 when wrapping a line. Done. https://codereview.chromium.org/476913003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:470: event.volumeId = this.navigationVolumes_[path]; On 2014/08/22 06:59:24, hirono wrote: > Could you follow #470 and #471 after #465? > Currently there is no reason to split it into two parts. Done.
https://codereview.chromium.org/476913003/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/device_handler.js (right): https://codereview.chromium.org/476913003/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:143: 'deviceFailUnknown', The ID should be deviceFail. When the notification opens, it closes other notifications having the same ID automatically. https://codereview.chromium.org/476913003/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:451: DeviceHandler.Notification.DEVICE_FAIL_UNKNOWN.hide(volume.devicePath); The hide method call is needed? https://codereview.chromium.org/476913003/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:465: var path = id.substr(pos+1); nit: We insert a space around an operator.
Thank you for your review. https://codereview.chromium.org/476913003/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/device_handler.js (right): https://codereview.chromium.org/476913003/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:143: 'deviceFailUnknown', On 2014/08/25 05:03:26, hirono wrote: > The ID should be deviceFail. > When the notification opens, it closes other notifications having the same ID > automatically. Done. https://codereview.chromium.org/476913003/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:143: 'deviceFailUnknown', On 2014/08/25 05:03:26, hirono wrote: > The ID should be deviceFail. > When the notification opens, it closes other notifications having the same ID > automatically. Done. https://codereview.chromium.org/476913003/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:451: DeviceHandler.Notification.DEVICE_FAIL_UNKNOWN.hide(volume.devicePath); On 2014/08/25 05:03:26, hirono wrote: > The hide method call is needed? Done. https://codereview.chromium.org/476913003/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:465: var path = id.substr(pos+1); On 2014/08/25 05:03:26, hirono wrote: > nit: We insert a space around an operator. Done.
https://codereview.chromium.org/476913003/diff/60001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/device_handler.js (right): https://codereview.chromium.org/476913003/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:366: event.volumeMetadata.volumeId; nit: The amount of indent is 4. https://codereview.chromium.org/476913003/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:447: message = volume.deviceLabel ? The lines are executed even if the this.mountStatus_[volume.devicePath] is neither CHILD_ERROR nor ONLY_PARENT_ERROR. Why don't you call show method in the switch statement? https://codereview.chromium.org/476913003/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:473: default: The default: is not needed. Also please consider to replace the switch statement with a if statement.
Thank you for your review :) https://codereview.chromium.org/476913003/diff/60001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/device_handler.js (right): https://codereview.chromium.org/476913003/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:366: event.volumeMetadata.volumeId; On 2014/08/25 06:31:42, hirono wrote: > nit: The amount of indent is 4. Done. https://codereview.chromium.org/476913003/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:447: message = volume.deviceLabel ? On 2014/08/25 06:31:42, hirono wrote: > The lines are executed even if the this.mountStatus_[volume.devicePath] is > neither CHILD_ERROR nor ONLY_PARENT_ERROR. > > Why don't you call show method in the switch statement? Done. https://codereview.chromium.org/476913003/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:473: default: On 2014/08/25 06:31:42, hirono wrote: > The default: is not needed. > > Also please consider to replace the switch statement with a if statement. Done.
https://codereview.chromium.org/476913003/diff/80001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/device_handler.js (right): https://codereview.chromium.org/476913003/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:450: DeviceHandler.Notification.DEVICE_FAIL.show(volume.devicePath, message); nit: It is better to move the line into the switch statement as well as DEVICE_UNKNOWN_MESSAGE, for consistency. https://codereview.chromium.org/476913003/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:463: chrome.notifications.clear(id, function() {}); Please move the clear call into the if statement. Because onNotificationButtonClicked events is published for all the notifications created by the application. If the notification is not created by the DeviceHandler class, we would not like to modify it here.
Thank you for reviewing :) https://codereview.chromium.org/476913003/diff/80001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/device_handler.js (right): https://codereview.chromium.org/476913003/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:450: DeviceHandler.Notification.DEVICE_FAIL.show(volume.devicePath, message); On 2014/08/25 07:09:40, hirono wrote: > nit: It is better to move the line into the switch statement as well as > DEVICE_UNKNOWN_MESSAGE, for consistency. Done. https://codereview.chromium.org/476913003/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/device_handler.js:463: chrome.notifications.clear(id, function() {}); On 2014/08/25 07:09:40, hirono wrote: > Please move the clear call into the if statement. Because > onNotificationButtonClicked events is published for all the notifications > created by the application. If the notification is not created by the > DeviceHandler class, we would not like to modify it here. Done.
lgtm! Thank you!
The CQ bit was checked by iseki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iseki@chromium.org/476913003/100001
The CQ bit was unchecked by iseki@chromium.org
Please take a look.
lgtm
The CQ bit was checked by iseki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iseki@chromium.org/476913003/100001
On 2014/08/25 08:28:12, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/iseki@chromium.org/476913003/100001 Please add TEST= line to the bug description.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by iseki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iseki@chromium.org/476913003/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Message was sent while issue was closed.
Committed patchset #7 (120001) as d0a72fc2631d3448bf2c35609b84a99d077f7d0a
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b39db9f1b80b40bd04acdf621f515b95c26d990d Cr-Commit-Position: refs/heads/master@{#291670} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
