Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15643006:
Files.app: Added DragSelector class. (Closed)
|
Created:
7 years, 6 months ago by hirono Modified:
7 years, 6 months ago CC:
chromium-reviews, rginda+watch_chromium.org, arv+watch_chromium.org, dcheng Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFiles.app: Added DragSelector class.
DragSelector class enables user to select multiple files by dragging. By
calling DragSelector#startDragSelection(event), drag selection is
started. DragSelector class added handlers of mouse events, shows drag handler
and selects files by the mouse operation.
In order to call startDragSelection, users of DragSelector class must handle
'dragStart' event of the list and decide whether to call startDragSelection or
not according to the event. (e.g. whether the start point is hit to background
of the list or not)
If the drag selection is started with modifier keys (shift or ctrl),
DragSelector stores the selection state at the beginning of drag selection. If
an item included in drag handle once and then it is get out of the handler, the
selection state of the item is restored by the stored initial selection state.
BUG=224832
TEST=Added a call of startDragSelection to FileTransferManager, and
do drag operation on the file list.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203913
Patch Set 1 #
Total comments: 38
Patch Set 2 : Addressed comment. #
Total comments: 6
Patch Set 3 : Addressed comments. #
Total comments: 17
Patch Set 4 : Fixed comments #Patch Set 5 : Addressed Yoshiki's comments. #
Messages
Total messages: 16 (0 generated)
Could you check this CL to add new class for drag selection?
The call of startDragSelection is added by the next CL.
In order to test this class, please add the following line to the
file_transfer_controller.js.
(at line 298)
onDragStart_: function(list, event) {
+ var dragSelector = new DragSelector();
+ dragSelector.startDragSelection(list, event);
// Nothing selected.
if (!this.selectedEntries_.length) {
Thank you very much!
On 2013/05/31 07:45:47, hirono wrote:
> Could you check this CL to add new class for drag selection?
>
> The call of startDragSelection is added by the next CL.
> In order to test this class, please add the following line to the
> file_transfer_controller.js.
>
> (at line 298)
> onDragStart_: function(list, event) {
> + var dragSelector = new DragSelector();
> + dragSelector.startDragSelection(list, event);
> // Nothing selected.
> if (!this.selectedEntries_.length) {
>
> Thank you very much!
Works super smooth, i like it! Mostly nits. Besides, when dragging outside of
the list, the selection rect gets a little bit off.
https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/css/file_manager.css (right): https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/css/file_manager.css:1024: border-radius: 0; border-radius: 0 redundant? https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/css/file_manager.css:1026: padding: 0; padding: 0 redundant? https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/drag_selector.js (right): https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:16: * @type {cr.ui.list?} Shouldn't be list? -> List https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:22: * @private @private after @type https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:23: * @type {HtmlElement?} Isn't '?' redundant? Objects do not need ? for nullable afaik. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:37: * @type {number?} For example, here ? is necessary, since it is not an object. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:56: this.onMouseMove_ = DragSelector.prototype.onMouseMove_.bind(this); This looks tricky. For that we use: this.onMouseMoveBound_ = DragSelector... https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:62: * This function must be called from handlers of dragstart event. When @jsdoc is long, you may want to add an empty line after the description for better readability. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:71: // Set the target of drag selection of -> of the https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:72: this.target_ = list; Please add empty lines in large blocks of code for better readability. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:79: // Prevent default action Add periods at the end of sentences. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:81: // Handle modifier keys. Please change to more descriptive comment, eg. what the code actually does. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:82: if (!event.modifiers & Event.SHIFT_MASK && Does it work as intended? I can start selection using dragging even without the modifiers. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:103: * Handle the mousemove evnet. evnet -> event https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:112: var borderMetrics = { Metrics -> Bounds / Rect / Geometry? (optional) border -> selection? https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:125: var leadIndex = -1; Can we add a comment, eg. Collect items within the selection rect. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:142: selectionFlag[index] = selectionFlag[index] | 1; 1, 2 -> enum? https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:149: // Update selection selection -> the selection https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:152: var index = parseInt(name); Is parseInt necessary here?
On 2013/05/31 08:15:31, mtomasz wrote: > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > File chrome/browser/resources/file_manager/css/file_manager.css (right): > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/css/file_manager.css:1024: border-radius: > 0; > border-radius: 0 redundant? > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/css/file_manager.css:1026: padding: 0; > padding: 0 redundant? > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > File chrome/browser/resources/file_manager/js/drag_selector.js (right): > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:16: * @type > {cr.ui.list?} > Shouldn't be list? -> List > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:22: * @private > @private after @type > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:23: * @type > {HtmlElement?} > Isn't '?' redundant? Objects do not need ? for nullable afaik. > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:37: * @type {number?} > For example, here ? is necessary, since it is not an object. > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:56: this.onMouseMove_ > = DragSelector.prototype.onMouseMove_.bind(this); > This looks tricky. For that we use: > this.onMouseMoveBound_ = DragSelector... > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:62: * This function > must be called from handlers of dragstart event. > When @jsdoc is long, you may want to add an empty line after the description for > better readability. > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:71: // Set the target > of drag selection > of -> of the > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:72: this.target_ = > list; > Please add empty lines in large blocks of code for better readability. > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:79: // Prevent default > action > Add periods at the end of sentences. > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:81: // Handle modifier > keys. > Please change to more descriptive comment, eg. what the code actually does. > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:82: if > (!event.modifiers & Event.SHIFT_MASK && > Does it work as intended? I can start selection using dragging even without the > modifiers. > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:103: * Handle the > mousemove evnet. > evnet -> event > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:112: var borderMetrics > = { > Metrics -> Bounds / Rect / Geometry? > (optional) border -> selection? > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:125: var leadIndex = > -1; > Can we add a comment, eg. Collect items within the selection rect. > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:142: > selectionFlag[index] = selectionFlag[index] | 1; > 1, 2 -> enum? > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:149: // Update > selection > selection -> the selection > > https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/drag_selector.js:152: var index = > parseInt(name); > Is parseInt necessary here? The problem that is occurred when dragging outside of the list is caused by assigning leadIndex as -1. I fixed this problem and addressed comments. Thank you very much!
https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/css/file_manager.css (right): https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/css/file_manager.css:1024: border-radius: 0; On 2013/05/31 08:15:31, mtomasz wrote: > border-radius: 0 redundant? This is overrides cr.ui.List's style. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/css/file_manager.css:1026: padding: 0; On 2013/05/31 08:15:31, mtomasz wrote: > padding: 0 redundant? ditto. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/drag_selector.js (right): https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:16: * @type {cr.ui.list?} On 2013/05/31 08:15:31, mtomasz wrote: > Shouldn't be list? -> List Done. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:22: * @private On 2013/05/31 08:15:31, mtomasz wrote: > @private after @type Done. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:23: * @type {HtmlElement?} On 2013/05/31 08:15:31, mtomasz wrote: > Isn't '?' redundant? Objects do not need ? for nullable afaik. Done. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:37: * @type {number?} On 2013/05/31 08:15:31, mtomasz wrote: > For example, here ? is necessary, since it is not an object. Done. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:56: this.onMouseMove_ = DragSelector.prototype.onMouseMove_.bind(this); On 2013/05/31 08:15:31, mtomasz wrote: > This looks tricky. For that we use: > this.onMouseMoveBound_ = DragSelector... Done. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:62: * This function must be called from handlers of dragstart event. On 2013/05/31 08:15:31, mtomasz wrote: > When @jsdoc is long, you may want to add an empty line after the description for > better readability. Done. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:71: // Set the target of drag selection On 2013/05/31 08:15:31, mtomasz wrote: > of -> of the Done. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:72: this.target_ = list; On 2013/05/31 08:15:31, mtomasz wrote: > Please add empty lines in large blocks of code for better readability. Done. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:79: // Prevent default action On 2013/05/31 08:15:31, mtomasz wrote: > Add periods at the end of sentences. Done. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:81: // Handle modifier keys. On 2013/05/31 08:15:31, mtomasz wrote: > Please change to more descriptive comment, eg. what the code actually does. Done. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:82: if (!event.modifiers & Event.SHIFT_MASK && On 2013/05/31 08:15:31, mtomasz wrote: > Does it work as intended? I can start selection using dragging even without the > modifiers. These modifier keys just specified that this selection is additional. (start drag selection with keeping the original selection) I'm planning on starting dragging selection without the modifiers. Whether drag selection should start or not is decided where the dragging started. (e.g. the drag selection starts in the case that the dragging started on the background of the list) This judgement will be implemented by the next CL. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:103: * Handle the mousemove evnet. On 2013/05/31 08:15:31, mtomasz wrote: > evnet -> event Done. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:112: var borderMetrics = { On 2013/05/31 08:15:31, mtomasz wrote: > Metrics -> Bounds / Rect / Geometry? > (optional) border -> selection? Done. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:125: var leadIndex = -1; On 2013/05/31 08:15:31, mtomasz wrote: > Can we add a comment, eg. Collect items within the selection rect. Done. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:142: selectionFlag[index] = selectionFlag[index] | 1; On 2013/05/31 08:15:31, mtomasz wrote: > 1, 2 -> enum? Done. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:149: // Update selection On 2013/05/31 08:15:31, mtomasz wrote: > selection -> the selection Done. https://codereview.chromium.org/15643006/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/drag_selector.js:152: var index = parseInt(name); On 2013/05/31 08:15:31, mtomasz wrote: > Is parseInt necessary here? Yes, actually, I had used the name directly at first. The name assigned by for..in loop is string and it cannot be found by this.originalSelection_.indexOf(name).
> Works super smooth, i like it! Mostly nits. Besides, when dragging outside of > the list, the selection rect gets a little bit off. How about this? https://codereview.chromium.org/15643006/diff/9003/chrome/browser/resources/f... File chrome/browser/resources/file_manager/js/drag_selector.js (right): https://codereview.chromium.org/15643006/diff/9003/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/drag_selector.js:97: if (!event.modifiers & Event.SHIFT_MASK && Shouldn't it be if !(event.modifiers & Event.SHIFT_MASK) && !(...)? https://codereview.chromium.org/15643006/diff/9003/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/drag_selector.js:177: if (flag == DragSelector.SelectionFlag_.IN_LAST_SELECTION) { Can you please describe the algorithm? In particular, this code is executed, if the item has been selected before. According to the comment, it is not included in currentSelection. How do you know about it? I can't see where it is checked. Can an selection item has both flags? IN_LAST_SELECTION and IN_CURRENT_SELECTION? https://codereview.chromium.org/15643006/diff/9003/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/drag_selector.js:178: // This is included in lastSelection but currentSelection. but -> but not in?
Fixed. Thank you very much! https://codereview.chromium.org/15643006/diff/9003/chrome/browser/resources/f... File chrome/browser/resources/file_manager/js/drag_selector.js (right): https://codereview.chromium.org/15643006/diff/9003/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/drag_selector.js:97: if (!event.modifiers & Event.SHIFT_MASK && On 2013/06/03 07:41:14, mtomasz wrote: > Shouldn't it be if !(event.modifiers & Event.SHIFT_MASK) && !(...)? Thank you very much! I completely missed this. And event.modifiers have been undefined. I fixed this so as to refer other properties. https://codereview.chromium.org/15643006/diff/9003/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/drag_selector.js:177: if (flag == DragSelector.SelectionFlag_.IN_LAST_SELECTION) { On 2013/06/03 07:41:14, mtomasz wrote: > Can you please describe the algorithm? In particular, this code is executed, if > the item has been selected before. According to the comment, it is not included > in currentSelection. How do you know about it? I can't see where it is checked. > > Can an selection item has both flags? IN_LAST_SELECTION and > IN_CURRENT_SELECTION? Yes, I added a comment and fixed the definition of SelectionFlag_ so that it looks like bit flag. https://codereview.chromium.org/15643006/diff/9003/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/drag_selector.js:178: // This is included in lastSelection but currentSelection. On 2013/06/03 07:41:14, mtomasz wrote: > but -> but not in? Done.
awesome! lgtm with some nits! https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/drag_selector.js (right): https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:176: // flag may be one of followings: nit: flag -> Flag / The flag https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:182: // If flag equals to (IN_LAST_SELECTION | IN_CURRENT_SELECTION), nit: flag -> the flag https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:183: // this is included in both the last selection and the current selection. nit: this -> then this item / then the item https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:187: // If flag equals to IN_LAST_SELECTION, nit: flag -> the flag https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:194: // this is included in currentSelection but no in lastSelection. nit: no in -> not in
Fixed. Thank you! https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/drag_selector.js (right): https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:176: // flag may be one of followings: On 2013/06/04 01:01:21, mtomasz wrote: > nit: flag -> Flag / The flag Done. https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:182: // If flag equals to (IN_LAST_SELECTION | IN_CURRENT_SELECTION), On 2013/06/04 01:01:21, mtomasz wrote: > nit: flag -> the flag Done. https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:183: // this is included in both the last selection and the current selection. On 2013/06/04 01:01:21, mtomasz wrote: > nit: this -> then this item / then the item Done. https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:187: // If flag equals to IN_LAST_SELECTION, On 2013/06/04 01:01:21, mtomasz wrote: > nit: flag -> the flag Done. https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:194: // this is included in currentSelection but no in lastSelection. On 2013/06/04 01:01:21, mtomasz wrote: > nit: no in -> not in Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/15643006/30001
https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/drag_selector.js (right): https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:46: this.lastSelection_ = null; nit: I think it is better to assign '[]' instead of 'null'. (making this non-nullable) https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:53: this.originalSelection_ = null; ditto https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:145: var itemMetrics = this.target_.getHeightsForIndex_(i); getHeightsForIndex_() is private. Please make it public before use it. (and cr.ui.List doesn't seem to have this method?)
https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/drag_selector.js (right): https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:145: var itemMetrics = this.target_.getHeightsForIndex_(i); On 2013/06/04 01:16:01, yoshiki wrote: > (and cr.ui.List doesn't seem to have this method?) Sorry, I was wrong. I did grep 'getHeightForIndex_'. getHeightsForIndex_() exists truly.
Fixed. Thank you! https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/drag_selector.js (right): https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:46: this.lastSelection_ = null; On 2013/06/04 01:16:01, yoshiki wrote: > nit: I think it is better to assign '[]' instead of 'null'. (making this > non-nullable) Done. https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:53: this.originalSelection_ = null; On 2013/06/04 01:16:01, yoshiki wrote: > ditto Done. https://codereview.chromium.org/15643006/diff/23001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/drag_selector.js:145: var itemMetrics = this.target_.getHeightsForIndex_(i); On 2013/06/04 01:50:42, yoshiki wrote: > On 2013/06/04 01:16:01, yoshiki wrote: > > (and cr.ui.List doesn't seem to have this method?) > Sorry, I was wrong. I did grep 'getHeightForIndex_'. getHeightsForIndex_() > exists truly. I added a TODO comment.
Thanks, LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/15643006/44002
Message was sent while issue was closed.
Change committed as 203913 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
