Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8890017:
Fixed renaming in the file manager. (Closed)
|
Created:
9 years ago by SeRya Modified:
9 years ago Reviewers:
dgozman CC:
chromium-reviews, rginda+watch_chromium.org, arv (Not doing code reviews), achuith+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFixed renaming in the file manager.
BUG=chromium-os:23770
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113928
Patch Set 1 : Removing unnecessary changes. #
Total comments: 4
Patch Set 2 : Fixing renaming from menu. #
Total comments: 2
Patch Set 3 : Code review fixes. #Patch Set 4 : Merge #Messages
Total messages: 13 (0 generated)
LGTM with comments addressed http://codereview.chromium.org/8890017/diff/2002/chrome/browser/resources/fil... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/8890017/diff/2002/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:1706: GridItem.prototype.__proto__ = cr.ui.ListItem.prototype; This is already set above. http://codereview.chromium.org/8890017/diff/2002/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:1760: fileName.appendChild(this.document_.createTextNode( You can use instead: fileName.textContent = this.getLabelForRootPath_.(entry.name) Same below.
I also spotted a bug in renaming from context menu. Fixed now. Also reorganized code a bit. Please, review again. http://codereview.chromium.org/8890017/diff/2002/chrome/browser/resources/fil... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/8890017/diff/2002/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:1706: GridItem.prototype.__proto__ = cr.ui.ListItem.prototype; On 2011/12/09 11:14:43, dgozman wrote: > This is already set above. Removed. http://codereview.chromium.org/8890017/diff/2002/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:1760: fileName.appendChild(this.document_.createTextNode( On 2011/12/09 11:14:43, dgozman wrote: > You can use instead: > fileName.textContent = this.getLabelForRootPath_.(entry.name) > > Same below. Fixed and extracted to a method to avoid code duplication.
Still LGTM http://codereview.chromium.org/8890017/diff/12001/chrome/browser/resources/fi... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/8890017/diff/12001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:3303: FileManager.prototype.onTableMouseDown_ = function(event) { Same as onGridMouseDown_.
http://codereview.chromium.org/8890017/diff/12001/chrome/browser/resources/fi... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/8890017/diff/12001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:3303: FileManager.prototype.onTableMouseDown_ = function(event) { On 2011/12/09 15:20:34, dgozman wrote: > Same as onGridMouseDown_. Fixed.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/serya@chromium.org/8890017/11004
Try job failure for 8890017-11004 (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/serya@chromium.org/8890017/11004
Can't apply patch for file chrome/browser/resources/file_manager/js/file_manager.js. While running patch -p1 --forward --force; patching file chrome/browser/resources/file_manager/js/file_manager.js Hunk #2 succeeded at 1134 (offset -76 lines). Hunk #3 succeeded at 1145 (offset -76 lines). Hunk #4 succeeded at 1166 (offset -76 lines). Hunk #5 succeeded at 1177 (offset -76 lines). Hunk #6 succeeded at 1220 (offset -76 lines). Hunk #7 succeeded at 1306 (offset -76 lines). Hunk #8 FAILED at 1755. Hunk #9 FAILED at 1884. Hunk #10 succeeded at 2532 (offset -128 lines). Hunk #11 succeeded at 2551 (offset -128 lines). Hunk #12 succeeded at 3278 (offset -149 lines). Hunk #13 succeeded at 3333 (offset -149 lines). Hunk #14 succeeded at 3356 (offset -149 lines). Hunk #15 succeeded at 3374 (offset -149 lines). Hunk #16 succeeded at 3394 (offset -149 lines). 2 out of 16 hunks FAILED -- saving rejects to file chrome/browser/resources/file_manager/js/file_manager.js.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/serya@chromium.org/8890017/8003
Try job failure for 8890017-8003 (retry) on linux_rel for step "ui_tests". It's a second try, previously, step "ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/serya@chromium.org/8890017/8003
Change committed as 113928 |
