Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8554003:
[filebrowser] Add left panel with roots. (Closed)
|
Created:
9 years, 1 month ago by dgozman Modified:
9 years ago Reviewers:
Evan Stade, arv (Not doing code reviews), rginda, Vladislav Kaznacheev, James Hawkins, SeRya, Dmitry Zvorygin CC:
chromium-reviews, Erik does not do reviews, achuith+watch_chromium.org, mihaip+watch_chromium.org, Aaron Boodman, rginda+watch_chromium.org, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
Description[filebrowser] Add left panel with roots.
Not the final UI yet.
Additional improvements:
- file name is selected in save-as dialog at start;
- new folder moved to context menu, button deleted.
BUG=chromium-os:20168, chromium-os:22106, chromium-os:22105, chromium-os:22032, chromium-os:20547, chromium-os:20549
TEST=Manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113804
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114227
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=115122
Patch Set 1 #
Total comments: 20
Patch Set 2 : '' #
Total comments: 4
Patch Set 3 : '' #
Total comments: 18
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 22
Patch Set 6 : '' #
Total comments: 8
Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #Patch Set 11 : '' #Patch Set 12 : '' #Patch Set 13 : '' #Patch Set 14 : '' #Messages
Total messages: 39 (0 generated)
Hi guys, Please, take a look, if you have some time. Live demo: http://dgozman.spb:8003/file_manager/harness.html Thanks, Dmitry
http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/compon... File chrome/browser/resources/component_extension_resources.grd (right): http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/compon... chrome/browser/resources/component_extension_resources.grd:25: <include name="IDR_FILE_MANAGER_MAIN" file="file_manager/main.html" flattenhtml="false" type="BINDATA" /> Are you sure you want this committed? http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:504: var entriesToEnumerate = 100; Seems quite hacky. How about having a set of directories we must visit and removing the ones we already visited, and exiting when it is empty? http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:506: function onAllRootsFound() { It is only called from one place, lets inline it. http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:2535: for (var index = 0; index < this.rootEntries_.length; index++) { Consider having getRootPathFor_ return an index and using it here too. http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:2663: dirEntry.fullPath == ARCHIVE_DIRECTORY) { Why? It would be nice to have a comment here. http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... File chrome/browser/resources/file_manager/js/harness.js (right): http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/harness.js:23: util.getOrCreateDirectory(filesystem.root, '/removable/disk1', function () {}); Long line? http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... File chrome/browser/resources/file_manager/js/mock_chrome.js (right): http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/mock_chrome.js:62: * File/directory changed notification. I already added those yesterday:)
http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/compon... File chrome/browser/resources/component_extension_resources.grd (right): http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/compon... chrome/browser/resources/component_extension_resources.grd:25: <include name="IDR_FILE_MANAGER_MAIN" file="file_manager/main.html" flattenhtml="false" type="BINDATA" /> On 2011/11/22 13:16:35, Vladislav Kaznacheev wrote: > Are you sure you want this committed? No. That's needed for live demo to work. Will revert before commit. http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:504: var entriesToEnumerate = 100; On 2011/11/22 13:16:35, Vladislav Kaznacheev wrote: > Seems quite hacky. How about having a set of directories we must visit and > removing the ones we already visited, and exiting when it is empty? We do not know the whole set in advance. Even worse: we can finish enumerating first directory, and only then find out the second one. http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:506: function onAllRootsFound() { On 2011/11/22 13:16:35, Vladislav Kaznacheev wrote: > It is only called from one place, lets inline it. Done. http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:2535: for (var index = 0; index < this.rootEntries_.length; index++) { On 2011/11/22 13:16:35, Vladislav Kaznacheev wrote: > Consider having getRootPathFor_ return an index and using it here too. Done. http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:2663: dirEntry.fullPath == ARCHIVE_DIRECTORY) { On 2011/11/22 13:16:35, Vladislav Kaznacheev wrote: > Why? It would be nice to have a comment here. Comment added. http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... File chrome/browser/resources/file_manager/js/harness.js (right): http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/harness.js:23: util.getOrCreateDirectory(filesystem.root, '/removable/disk1', function () {}); On 2011/11/22 13:16:35, Vladislav Kaznacheev wrote: > Long line? Done. http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... File chrome/browser/resources/file_manager/js/mock_chrome.js (right): http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/mock_chrome.js:62: * File/directory changed notification. On 2011/11/22 13:16:35, Vladislav Kaznacheev wrote: > I already added those yesterday:) Done.
http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/compon... File chrome/browser/resources/component_extension_resources.grd (right): http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/compon... chrome/browser/resources/component_extension_resources.grd:25: <include name="IDR_FILE_MANAGER_MAIN" file="file_manager/main.html" flattenhtml="false" type="BINDATA" /> flattenhtml should be true in source repository http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:474: FileManager.prototype.requestFileSystem_ = function() { It might be better to move requestFileSystem_ to init, or at least rename to initFileSystem. Because this function should be only called once. http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:542: metrics.startInterval('EnumerateRoots'); Should be corresponding metrics.recordTime or something ilke this http://codereview.chromium.org/8554003/diff/8003/chrome/browser/resources/fil... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/8554003/diff/8003/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2313: var rescanDirectoryNeeded = false; rescanDirecoty API has changed a bit. There's auto-refresh now. http://codereview.chromium.org/8554003/diff/8003/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2321: self.rescanDirectory_(null, 300); Rescan directory second parameter is error callback
http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/compon... File chrome/browser/resources/component_extension_resources.grd (right): http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/compon... chrome/browser/resources/component_extension_resources.grd:25: <include name="IDR_FILE_MANAGER_MAIN" file="file_manager/main.html" flattenhtml="false" type="BINDATA" /> On 2011/11/22 14:42:47, Dmitry Zvorygin wrote: > flattenhtml should be true in source repository Done. http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:474: FileManager.prototype.requestFileSystem_ = function() { On 2011/11/22 14:42:47, Dmitry Zvorygin wrote: > It might be better to move requestFileSystem_ to init, or at least rename to > initFileSystem. Because this function should be only called once. Renamed. http://codereview.chromium.org/8554003/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:542: metrics.startInterval('EnumerateRoots'); On 2011/11/22 14:42:47, Dmitry Zvorygin wrote: > Should be corresponding metrics.recordTime or something ilke this Done. http://codereview.chromium.org/8554003/diff/8003/chrome/browser/resources/fil... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/8554003/diff/8003/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2313: var rescanDirectoryNeeded = false; On 2011/11/22 14:42:47, Dmitry Zvorygin wrote: > rescanDirecoty API has changed a bit. There's auto-refresh now. Deleted this. Hooray! http://codereview.chromium.org/8554003/diff/8003/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2321: self.rescanDirectory_(null, 300); On 2011/11/22 14:42:47, Dmitry Zvorygin wrote: > Rescan directory second parameter is error callback And this too. Hooray!
http://codereview.chromium.org/8554003/diff/10002/chrome/browser/extensions/e... File chrome/browser/extensions/extension_file_browser_private_api.cc (right): http://codereview.chromium.org/8554003/diff/10002/chrome/browser/extensions/e... chrome/browser/extensions/extension_file_browser_private_api.cc:1524: SET_STRING(IDS_FILE_BROWSER, CHROMEBOOK_DIRECTORY_LABEL); The "File shelf" string is probably not needed anymore. http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/co... File chrome/browser/resources/component_extension_resources.grd (right): http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/co... chrome/browser/resources/component_extension_resources.grd:27: <include name="IDR_FILE_MANAGER_MAIN" file="file_manager/main.html" flattenhtml="false" type="BINDATA" /> Must be "true" http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:658: command.label = command.textContent; Right way to int-ze the labes is using 'i18-values="label:STRING_ID;"' http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:1436: removableDirectoryEntry.fullPath || DOWNLOADS_DIRECTORY, removableDirectoryEntry may be undefined. Access to .fullPath would cause an exception in this case. http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:1754: FileManager.prototype.getLabelForRootPath_ = function(path) { Looks like meaning of the parameter has changed. Now the path is assumed to be absolute (starting with '/'). Please, write a comment to reflect that. http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:1769: return path || str('ROOT_DIRECTORY_LABEL'); path == '/', right? http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:2457: div.textContent = this.getLabelForRootPath_(path); Remove braces. http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:2698: /* var location = '#' + encodeURI(dirEntry.fullPath); Uncomment. http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:3558: if (this.dialogContainer_.hasAttribute('sidebar')) { Now we have 3 attributes that does essentially the same things: hidden, visibility and now 'sidebar'. Moreover behavior is different. "Low disk space" and the bottom panel resize the list with no delay then showing. We must have consistency with the animation and avoid code duplication.
http://codereview.chromium.org/8554003/diff/10002/chrome/browser/extensions/e... File chrome/browser/extensions/extension_file_browser_private_api.cc (right): http://codereview.chromium.org/8554003/diff/10002/chrome/browser/extensions/e... chrome/browser/extensions/extension_file_browser_private_api.cc:1524: SET_STRING(IDS_FILE_BROWSER, CHROMEBOOK_DIRECTORY_LABEL); On 2011/11/28 10:30:55, SeRya wrote: > The "File shelf" string is probably not needed anymore. Done. http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/co... File chrome/browser/resources/component_extension_resources.grd (right): http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/co... chrome/browser/resources/component_extension_resources.grd:27: <include name="IDR_FILE_MANAGER_MAIN" file="file_manager/main.html" flattenhtml="false" type="BINDATA" /> On 2011/11/28 10:30:55, SeRya wrote: > Must be "true" Done. http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:658: command.label = command.textContent; On 2011/11/28 10:30:55, SeRya wrote: > Right way to int-ze the labes is using 'i18-values="label:STRING_ID;"' Done. http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:1436: removableDirectoryEntry.fullPath || DOWNLOADS_DIRECTORY, On 2011/11/28 10:30:55, SeRya wrote: > removableDirectoryEntry may be undefined. Access to .fullPath would cause an > exception in this case. Done. http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:1754: FileManager.prototype.getLabelForRootPath_ = function(path) { On 2011/11/28 10:30:55, SeRya wrote: > Looks like meaning of the parameter has changed. Now the path is assumed to be > absolute (starting with '/'). Please, write a comment to reflect that. Done. http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:1769: return path || str('ROOT_DIRECTORY_LABEL'); On 2011/11/28 10:30:55, SeRya wrote: > path == '/', right? Path may be arbitrary here. Changed to remove |path| and removed ROOT_DIRECTORY_LABEL string entirely. http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:2457: div.textContent = this.getLabelForRootPath_(path); On 2011/11/28 10:30:55, SeRya wrote: > Remove braces. Done. http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:2698: /* var location = '#' + encodeURI(dirEntry.fullPath); On 2011/11/28 10:30:55, SeRya wrote: > Uncomment. Done. http://codereview.chromium.org/8554003/diff/10002/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:3558: if (this.dialogContainer_.hasAttribute('sidebar')) { On 2011/11/28 10:30:55, SeRya wrote: > Now we have 3 attributes that does essentially the same things: hidden, > visibility and now 'sidebar'. > > Moreover behavior is different. "Low disk space" and the bottom panel resize the > list with no delay then showing. We must have consistency with the animation and > avoid code duplication. I've filed a bug: crosbug.com/23455.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/8554003/22001
Can't process patch for file chrome/browser/resources/file_manager/images/chromebook_28x28.png. Failed to parse svn properties.
Erik, Please, approve the change to component_extension_resources.grd. Thanks, Dmitry
Erik, Evan, James, Could you please approve the change to component_extension_resources.grd? Thanks, Dmitry
It sounds like that file is not in the right place, since I don't think any one of the three of is the right person to review changes to it.
open_sidebar.png seems to have a border. Can that be done in CSS instead? http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... File chrome/browser/resources/file_manager/css/file_manager.css (right): http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:91: visibility: hidden; commands should be display none http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:131: background-color: rgba(240,240,240,1); ws after comma http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:137: -webkit-transition: margin-left 180ms ease; I thought we used 150ms across the board http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:194: opacity: 1.0; opacity: 1; http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:296: margin-left: 10px; RTL http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:301: margin-right: 5px; RTL http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:474: * Request local file system, resolve roots and init_ after that. missing @private? http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:1438: var path = (removableDirectoryEntry && removableDirectoryEntry.fullPath) || too many parentheses http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:1792: if (isParentPath(ARCHIVE_DIRECTORY, path)) else if? http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:1802: icon.setAttribute('src', this.getRootIconUrl_(entry.fullPath, false)); icon.src = ... http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... File chrome/browser/resources/file_manager/main.html (right): http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/main.html:170: onclick='fileManager.deleteEntries( It would be better if you didn't use inline event handlers but I see that these are just moved.
There will be some UI updates after this CL is landed, and I will ask UX designer about border in open_sidebar.png. Maybe, this file is in the wrong place, but I have to change it now and commit. Please, approve. I will try to move it after discussing with other developers. http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... File chrome/browser/resources/file_manager/css/file_manager.css (right): http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:91: visibility: hidden; On 2011/11/30 18:21:12, arv wrote: > commands should be display none Done. http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:131: background-color: rgba(240,240,240,1); On 2011/11/30 18:21:12, arv wrote: > ws after comma Done. http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:137: -webkit-transition: margin-left 180ms ease; On 2011/11/30 18:21:12, arv wrote: > I thought we used 150ms across the board I do not know about other places :-) This is taken directly from UX prototype. http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:194: opacity: 1.0; On 2011/11/30 18:21:12, arv wrote: > opacity: 1; Done. http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:296: margin-left: 10px; On 2011/11/30 18:21:12, arv wrote: > RTL Done. http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:301: margin-right: 5px; On 2011/11/30 18:21:12, arv wrote: > RTL Done. http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:474: * Request local file system, resolve roots and init_ after that. On 2011/11/30 18:21:12, arv wrote: > missing @private? Done. http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:1438: var path = (removableDirectoryEntry && removableDirectoryEntry.fullPath) || On 2011/11/30 18:21:12, arv wrote: > too many parentheses Done. http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:1792: if (isParentPath(ARCHIVE_DIRECTORY, path)) On 2011/11/30 18:21:12, arv wrote: > else if? Done. http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/js/file_manager.js:1802: icon.setAttribute('src', this.getRootIconUrl_(entry.fullPath, false)); On 2011/11/30 18:21:12, arv wrote: > icon.src = ... Done. http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... File chrome/browser/resources/file_manager/main.html (right): http://codereview.chromium.org/8554003/diff/27001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/main.html:170: onclick='fileManager.deleteEntries( On 2011/11/30 18:21:12, arv wrote: > It would be better if you didn't use inline event handlers but I see that these > are just moved. I will cleanup such things in next CL.
http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/fi... File chrome/browser/resources/file_manager/css/file_manager.css (right): http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:125: margin-left: -200px; -webkit-margin-start http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:137: margin-left: 15px; -webkit-margin-start http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:151: background-color: rgba(240,240,240,1); whitespace after commas http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:197: margin-top: 15px; Use margin: 15px 15px 4px 15px; http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:210: div.close-sidebar { skip div here. In general skip the tag name in front of a class name selector, especially if it is a common element like div or span. http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:213: right: 0; RTL? Any time you have left or right you also need to handle RTL. http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:230: margin-right: 10px; -webkit-margin-end http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/fi... chrome/browser/resources/file_manager/css/file_manager.css:265: margin-left: 0; -webkti-margin-start
Hi Erik, Filebrowser does not work in RTL yet. We will be happy to change this, but not in this CL :-) Also, I use the 'div.class' and rgb(3,2,1,0) with this style for consistency with other usages around. If you find this to be really bad, tell me, and I will file a cleanup-bug for this. If you don't see any serious issues (especially, in component_extension_resources.grd), please approve. Oh, and we need to land this before branch :-) Thank you for your understanding, Dmitry On Thu, Dec 1, 2011 at 11:08 PM, <arv@chromium.org> wrote: > > http://codereview.chromium.**org/8554003/diff/36001/chrome/** > browser/resources/file_**manager/css/file_manager.css<http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/file_manager/css/file_manager.css> > File chrome/browser/resources/file_**manager/css/file_manager.css (right): > > http://codereview.chromium.**org/8554003/diff/36001/chrome/** > browser/resources/file_**manager/css/file_manager.css#**newcode125<http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/file_manager/css/file_manager.css#newcode125> > chrome/browser/resources/file_**manager/css/file_manager.css:**125: > margin-left: -200px; > -webkit-margin-start > > http://codereview.chromium.**org/8554003/diff/36001/chrome/** > browser/resources/file_**manager/css/file_manager.css#**newcode137<http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/file_manager/css/file_manager.css#newcode137> > chrome/browser/resources/file_**manager/css/file_manager.css:**137: > margin-left: 15px; > -webkit-margin-start > > http://codereview.chromium.**org/8554003/diff/36001/chrome/** > browser/resources/file_**manager/css/file_manager.css#**newcode151<http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/file_manager/css/file_manager.css#newcode151> > chrome/browser/resources/file_**manager/css/file_manager.css:**151: > background-color: rgba(240,240,240,1); > whitespace after commas > > http://codereview.chromium.**org/8554003/diff/36001/chrome/** > browser/resources/file_**manager/css/file_manager.css#**newcode197<http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/file_manager/css/file_manager.css#newcode197> > chrome/browser/resources/file_**manager/css/file_manager.css:**197: > margin-top: 15px; > Use > > margin: 15px 15px 4px 15px; > > http://codereview.chromium.**org/8554003/diff/36001/chrome/** > browser/resources/file_**manager/css/file_manager.css#**newcode210<http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/file_manager/css/file_manager.css#newcode210> > chrome/browser/resources/file_**manager/css/file_manager.css:**210: > div.close-sidebar { > skip div here. In general skip the tag name in front of a class name > selector, especially if it is a common element like div or span. > > http://codereview.chromium.**org/8554003/diff/36001/chrome/** > browser/resources/file_**manager/css/file_manager.css#**newcode213<http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/file_manager/css/file_manager.css#newcode213> > chrome/browser/resources/file_**manager/css/file_manager.css:**213: right: > 0; > RTL? > > Any time you have left or right you also need to handle RTL. > > http://codereview.chromium.**org/8554003/diff/36001/chrome/** > browser/resources/file_**manager/css/file_manager.css#**newcode230<http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/file_manager/css/file_manager.css#newcode230> > chrome/browser/resources/file_**manager/css/file_manager.css:**230: > margin-right: 10px; > -webkit-margin-end > > http://codereview.chromium.**org/8554003/diff/36001/chrome/** > browser/resources/file_**manager/css/file_manager.css#**newcode265<http://codereview.chromium.org/8554003/diff/36001/chrome/browser/resources/file_manager/css/file_manager.css#newcode265> > chrome/browser/resources/file_**manager/css/file_manager.css:**265: > margin-left: 0; > -webkti-margin-start > > http://codereview.chromium.**org/8554003/<http://codereview.chromium.org/8554... >
LGTM Please follow up on the RTL issues and style violations.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/8554003/43001
Can't process patch for file chrome/browser/resources/file_manager/js/mock_chrome.js. File's status is None, patchset upload is incomplete.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/8554003/45001
Try job failure for 8554003-45001 (retry) on win_rel for steps "net_unittests, safe_browsing_tests" (clobber build). It's a second try, previously, step "compile" 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/dgozman@chromium.org/8554003/45001
Change committed as 113152
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/8554003/50001
Can't apply patch for file chrome/browser/resources/file_manager/js/file_manager.js. While running patch -p0 --forward --force; patching file chrome/browser/resources/file_manager/js/file_manager.js Hunk #1 succeeded at 61 (offset -2 lines). Hunk #2 succeeded at 473 (offset -2 lines). Hunk #3 FAILED at 496. Hunk #4 succeeded at 536 (offset -2 lines). Hunk #5 succeeded at 594 (offset -2 lines). Hunk #6 succeeded at 607 (offset -2 lines). Hunk #7 succeeded at 665 (offset -2 lines). Hunk #8 succeeded at 695 (offset -2 lines). Hunk #9 succeeded at 755 (offset -2 lines). Hunk #10 succeeded at 764 (offset -2 lines). Hunk #11 succeeded at 824 (offset -2 lines). Hunk #12 succeeded at 1127 (offset 1 line). Hunk #13 succeeded at 1337 (offset 1 line). Hunk #14 succeeded at 1364 (offset 1 line). Hunk #15 succeeded at 1376 (offset 1 line). Hunk #16 succeeded at 1413 (offset 1 line). Hunk #17 succeeded at 1456 (offset 3 lines). Hunk #18 succeeded at 1472 (offset 3 lines). Hunk #19 succeeded at 1713 (offset -2 lines). Hunk #20 succeeded at 1741 (offset -2 lines). Hunk #21 succeeded at 1830 (offset -2 lines). Hunk #22 succeeded at 2204 (offset -11 lines). Hunk #23 succeeded at 2225 (offset -11 lines). Hunk #24 succeeded at 2305 (offset -11 lines). Hunk #25 succeeded at 2363 (offset -14 lines). Hunk #26 succeeded at 2440 (offset -13 lines). Hunk #27 succeeded at 2456 (offset -13 lines). Hunk #28 succeeded at 2594 (offset -13 lines). Hunk #29 succeeded at 2732 with fuzz 2 (offset -9 lines). Hunk #30 succeeded at 2972 (offset -18 lines). Hunk #31 succeeded at 3007 (offset -18 lines). Hunk #32 succeeded at 3165 (offset -18 lines). Hunk #33 FAILED at 3281. Hunk #34 succeeded at 3619 (offset -22 lines). Hunk #35 succeeded at 3758 (offset -22 lines). 2 out of 35 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/dgozman@chromium.org/8554003/57002
Try job failure for 8554003-57002 (retry) on mac_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/8554003/57002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/8554003/61001
Change committed as 113804
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/8554003/64001
Change committed as 114227
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/8554003/70001
Try job failure for 8554003-70001 (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/dgozman@chromium.org/8554003/70001
Can't apply patch for file chrome/browser/resources/file_manager/css/file_manager.css. While running patch -p0 --forward --force; patching file chrome/browser/resources/file_manager/css/file_manager.css Hunk #4 FAILED at 376. Hunk #5 succeeded at 435 (offset 1 line). 1 out of 5 hunks FAILED -- saving rejects to file chrome/browser/resources/file_manager/css/file_manager.css.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/8554003/77001
Change committed as 115122 |
