Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22021003:
Move the initialization code of volume item to VolumeItem constructor. (Closed)
|
Created:
7 years, 4 months ago by yoshiki Modified:
7 years, 4 months ago Reviewers:
mtomasz CC:
chromium-reviews, rginda+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMove the initialization code of volume item to VolumeItem constructor.
The initialization of volume item has been done in VolumeList.renderRoot_(), but as I wrote in the TODO comment in volume_list.js, it should be done in the constructor of VolumeItem. This patch moves it as the comment.
This patch shouldn't change any functionality.
BUG=none
TEST=Volume list works correctly on mounting/unmounting volume. Folder shortcut can be created and removed.
R=mtomasz@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215823
Patch Set 1 #
Total comments: 15
Patch Set 2 : addressed comment #Patch Set 3 : addressed comment #Messages
Total messages: 9 (0 generated)
@mtomasz: PTAL. Thanks.
Nice cleanup. lgtm with some small comments. https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/volume_list.js (right): https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:148: if (this.decorated_) It looks tricky but it seems that we have to do it this way... https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:201: } else { nit: Can setPath be called more than once? https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:210: * Associate a context menu with this item. I think this is confusing. This method sometimes will not set the context menu after calling it. I think we should move the 'if' back to the calling place and keep this method simple - just setting the context menu passed as an argument. WDYT? https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:313: item.addEventListener(cr.ui.TouchHandler.EventType.TOUCH_START, handleClick); Is this necessary? Isn't touching emitting click events? https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:337: this.getListItemByIndex(i).setContextMenu(this.contextMenu_); nit: I think the if logic here was a cleaner design.
https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/volume_list.js (right): https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:148: if (this.decorated_) On 2013/08/06 01:33:06, mtomasz wrote: > It looks tricky but it seems that we have to do it this way... This is because of a design mismatch between cr.ui.List and cr.ui.define(). I changed slightly to check this.className instead for readability. https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:201: } else { On 2013/08/06 01:33:06, mtomasz wrote: > nit: Can setPath be called more than once? Shouldn't be. Removed the code around and added the warning in case of double-calling. https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:210: * Associate a context menu with this item. On 2013/08/06 01:33:06, mtomasz wrote: > I think this is confusing. This method sometimes will not set the context menu > after calling it. I think we should move the 'if' back to the calling place and > keep this method simple - just setting the context menu passed as an argument. > WDYT? This method is called from 2 places, so moving 'if' back to caller is increasing the code amount. Instead, to prevent the confusion, I changed the name to 'maybeSetContextMenu()'. Note that I'm planning to check in the patch which makes the menu hidden when there are no visible menu items in the menu. Which the patch, we can controls the visibility of the menu from file_manager_commands.js, and we can remove the 'if'-code here. https://codereview.chromium.org/22154002/ https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:313: item.addEventListener(cr.ui.TouchHandler.EventType.TOUCH_START, handleClick); On 2013/08/06 01:33:06, mtomasz wrote: > Is this necessary? Isn't touching emitting click events? I'm not sure but it has been here for a long time. Since this is a re-factoring patch, I keep it in this patch. And I'll check if it is necessary, and will remove it if unnecessary in a separate patch.
lgtm with 3 nits. thanks! https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/volume_list.js (right): https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:210: * Associate a context menu with this item. On 2013/08/06 04:42:20, yoshiki wrote: > On 2013/08/06 01:33:06, mtomasz wrote: > > I think this is confusing. This method sometimes will not set the context menu > > after calling it. I think we should move the 'if' back to the calling place > and > > keep this method simple - just setting the context menu passed as an argument. > > WDYT? > > This method is called from 2 places, so moving 'if' back to caller is increasing > the code amount. Instead, to prevent the confusion, I changed the name to > 'maybeSetContextMenu()'. > > > Note that I'm planning to check in the patch which makes the menu hidden when > there are no visible menu items in the menu. Which the patch, we can controls > the visibility of the menu from file_manager_commands.js, and we can remove the > 'if'-code here. > https://codereview.chromium.org/22154002/ SGTM. Hiding menu with no items is a great idea. As for now, please just add a short comment to the method description when the menu is added, when not. https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:217: rootType != RootType.DRIVE && rootType != RootType.DOWNLOADS) nit: Add () around for readability? https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:313: item.addEventListener(cr.ui.TouchHandler.EventType.TOUCH_START, handleClick); On 2013/08/06 04:42:20, yoshiki wrote: > On 2013/08/06 01:33:06, mtomasz wrote: > > Is this necessary? Isn't touching emitting click events? > > I'm not sure but it has been here for a long time. Since this is a re-factoring > patch, I keep it in this patch. And I'll check if it is necessary, and will > remove it if unnecessary in a separate patch. nit: Please add a TODO, so we won't forget.
https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/volume_list.js (right): https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:210: * Associate a context menu with this item. On 2013/08/06 04:52:23, mtomasz wrote: > On 2013/08/06 04:42:20, yoshiki wrote: > > On 2013/08/06 01:33:06, mtomasz wrote: > > > I think this is confusing. This method sometimes will not set the context > menu > > > after calling it. I think we should move the 'if' back to the calling place > > and > > > keep this method simple - just setting the context menu passed as an > argument. > > > WDYT? > > > > This method is called from 2 places, so moving 'if' back to caller is > increasing > > the code amount. Instead, to prevent the confusion, I changed the name to > > 'maybeSetContextMenu()'. > > > > > > Note that I'm planning to check in the patch which makes the menu hidden when > > there are no visible menu items in the menu. Which the patch, we can controls > > the visibility of the menu from file_manager_commands.js, and we can remove > the > > 'if'-code here. > > https://codereview.chromium.org/22154002/ > > SGTM. Hiding menu with no items is a great idea. As for now, please just add a > short comment to the method description when the menu is added, when not. Done. https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:217: rootType != RootType.DRIVE && rootType != RootType.DOWNLOADS) On 2013/08/06 04:52:23, mtomasz wrote: > nit: Add () around for readability? Done. https://codereview.chromium.org/22021003/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:313: item.addEventListener(cr.ui.TouchHandler.EventType.TOUCH_START, handleClick); On 2013/08/06 04:52:23, mtomasz wrote: > On 2013/08/06 04:42:20, yoshiki wrote: > > On 2013/08/06 01:33:06, mtomasz wrote: > > > Is this necessary? Isn't touching emitting click events? > > > > I'm not sure but it has been here for a long time. Since this is a > re-factoring > > patch, I keep it in this patch. And I'll check if it is necessary, and will > > remove it if unnecessary in a separate patch. > > nit: Please add a TODO, so we won't forget. Done.
Thanks for reviewing. Going to commit.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/22021003/16001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Message was sent while issue was closed.
Committed patchset #3 manually as r215823 (presubmit successful). |
