Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23537002:
Adds new private API manifest for Files.app copy. (Closed)
|
Created:
7 years, 3 months ago by hidehiko Modified:
7 years, 3 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, rginda+watch_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdds new private API manifest for Files.app copy.
This CL introduces three new private APIs, which will be used to replace
Files.app's current copy implementation.
Currently, the copy operation is implemented in JavaScript layer, because
FileSystem API's copyTo method doesn't support progress update and cancelling.
To let Files.app focus on UI, the copy implementation is being moved to
C++ layer, and these private APIs will be used for it.
This CL just adds manifest entries, so no behavior change is expected.
BUG=279287
TEST=Tested manually.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220603
Patch Set 1 #Patch Set 2 : #
Total comments: 12
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 7
Patch Set 5 : #
Total comments: 14
Patch Set 6 : Rebase #Patch Set 7 : #
Total comments: 2
Patch Set 8 : Rebase #Patch Set 9 : #
Total comments: 4
Patch Set 10 : Rebase #Patch Set 11 : #
Total comments: 2
Patch Set 12 : Rebase #Patch Set 13 : #Messages
Total messages: 23 (0 generated)
Thank you for your review in advance, - hidehiko
https://codereview.chromium.org/23537002/diff/2001/chrome/common/extensions/a... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/2001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:358: "description": "URL for the entry. Available only for BEGIN_ENTRY_COPY, END_ENTRY_COPY, PROGRESS and SUCCESS." If a directory copy is initiated with startCopy(), this field tells what file in that directory is now being copied, right? Then maybe: "URL for the entry currently being copied. This field is particularly useful when a directory copy is initiated with startCopy(). The field tells what file in that directory is now being copied" https://codereview.chromium.org/23537002/diff/2001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:360: "size": { size -> num_bytes_copied ? see below https://codereview.chromium.org/23537002/diff/2001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:363: "description": "Number of processed bytes for each entry copy. Available only for PROGRESS event." What does this mean for a directory copy? I'm guessing this is the total number of bytes copied so far for multiple files (sizes of copied files + copied bytes of the file currently being copied), rather than just for the file currently being copied. To show a progress bar using the data, the client needs to pre-compute the total number of bytes of all files. Would be nice to document it here. https://codereview.chromium.org/23537002/diff/2001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:368: "description": "Error code of the error. Available only for ERROR event." What error code? Shouldn't we define enum codes for this? https://codereview.chromium.org/23537002/diff/2001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:1280: "description": "Periodically called during a copy task to notify its progress update.", events are not something "called". Maybe: "The event is periodically raised during a copy task to report progress update of the copy task.
Thank you for your review. PTAL? https://codereview.chromium.org/23537002/diff/2001/chrome/common/extensions/a... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/2001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:358: "description": "URL for the entry. Available only for BEGIN_ENTRY_COPY, END_ENTRY_COPY, PROGRESS and SUCCESS." On 2013/08/27 06:09:41, satorux1 wrote: > If a directory copy is initiated with startCopy(), this field tells what file in > that directory is now being copied, right? Then maybe: > > "URL for the entry currently being copied. This field is particularly useful > when a directory copy is initiated with startCopy(). The field tells what file > in that directory is now being copied" Done. https://codereview.chromium.org/23537002/diff/2001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:360: "size": { On 2013/08/27 06:09:41, satorux1 wrote: > size -> num_bytes_copied ? see below Will address in the next cycle. Please see also my reply below... https://codereview.chromium.org/23537002/diff/2001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:363: "description": "Number of processed bytes for each entry copy. Available only for PROGRESS event." On 2013/08/27 06:09:41, satorux1 wrote: > What does this mean for a directory copy? I'm guessing this is the total number > of bytes copied so far for multiple files (sizes of copied files + copied bytes > of the file currently being copied), rather than just for the file currently > being copied. > > To show a progress bar using the data, the client needs to pre-compute the total > number of bytes of all files. Would be nice to document it here. No, this won't be cumulative, but just num of copied bytes for each file. I'm thinking that; suppose a/b/{c.txt (100bytes), d.txt (200bytes)} and trying copy a/ to x/ recursively, the event sequence will be: BEGIN_ENTRY_COPY a/ <create x/a/> END_ENTRY_COPY a/ BEGIN_ENTRY_COPY a/b/ <create x/a/b/> END_ENTRY_COPY a/b/ BEGIN_ENTRY_COPY a/b/c.txt PROGRESS a/b/c.txt 0 PROGRESS a/b/c.txt 10 : PROGRESS a/b/c.txt 100 END_ENTRY_COPY a/b/c.txt BEGIN_ENTRY_COPY a/b/d.txt PROGRESS a/b/d.txt 0 PROGRESS a/b/d.txt 10 : PROGRESS a/b/d.txt 200 END_ENTRY_COPY a/b/d.txt SUCCESS x/a/ Added a comment that pre-compute the total bytes will be needed to show progress bar. https://codereview.chromium.org/23537002/diff/2001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:368: "description": "Error code of the error. Available only for ERROR event." On 2013/08/27 06:09:41, satorux1 wrote: > What error code? Shouldn't we define enum codes for this? Clarified it is FileError's code. (FileError is being deprecated, but Files.app still uses it). https://codereview.chromium.org/23537002/diff/2001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:1280: "description": "Periodically called during a copy task to notify its progress update.", On 2013/08/27 06:09:41, satorux1 wrote: > events are not something "called". Maybe: > > "The event is periodically raised during a copy task to report progress update > of the copy task. I see. Then how about "fired" as other event's descriptions use it?
https://codereview.chromium.org/23537002/diff/2001/chrome/common/extensions/a... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/2001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:363: "description": "Number of processed bytes for each entry copy. Available only for PROGRESS event." On 2013/08/27 06:32:18, hidehiko wrote: > On 2013/08/27 06:09:41, satorux1 wrote: > > What does this mean for a directory copy? I'm guessing this is the total > number > > of bytes copied so far for multiple files (sizes of copied files + copied > bytes > > of the file currently being copied), rather than just for the file currently > > being copied. > > > > To show a progress bar using the data, the client needs to pre-compute the > total > > number of bytes of all files. Would be nice to document it here. > > No, this won't be cumulative, but just num of copied bytes for each file. I'm > thinking that; > suppose a/b/{c.txt (100bytes), d.txt (200bytes)} and trying copy a/ to x/ > recursively, the event sequence will be: > > BEGIN_ENTRY_COPY a/ > <create x/a/> > END_ENTRY_COPY a/ > BEGIN_ENTRY_COPY a/b/ > <create x/a/b/> > END_ENTRY_COPY a/b/ > BEGIN_ENTRY_COPY a/b/c.txt > PROGRESS a/b/c.txt 0 > PROGRESS a/b/c.txt 10 > : > PROGRESS a/b/c.txt 100 > END_ENTRY_COPY a/b/c.txt > BEGIN_ENTRY_COPY a/b/d.txt > PROGRESS a/b/d.txt 0 > PROGRESS a/b/d.txt 10 > : > PROGRESS a/b/d.txt 200 > END_ENTRY_COPY a/b/d.txt > SUCCESS x/a/ > > Added a comment that pre-compute the total bytes will be needed to show progress > bar. Ah this explains a lot. https://codereview.chromium.org/23537002/diff/2001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:1280: "description": "Periodically called during a copy task to notify its progress update.", On 2013/08/27 06:32:18, hidehiko wrote: > On 2013/08/27 06:09:41, satorux1 wrote: > > events are not something "called". Maybe: > > > > "The event is periodically raised during a copy task to report progress update > > of the copy task. > > I see. Then how about "fired" as other event's descriptions use it? fired sgtm https://codereview.chromium.org/23537002/diff/7001/chrome/common/extensions/a... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/7001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:352: "enum": ["BEGIN_ENTRY_COPY", "END_ENTRY_COPY", "PROGRESS", "SUCCESS", "ERROR"], Why BEGIN_ENTRY_COPY instead of BEGIN_FILE_COPY? I'm guessing that "ENTRY" means either of a file or a directory. How to report a directory copy? How will the "size" field look like? If we are to copy a directory with thousands of sub directories + one very big file, how to show a progress bar nicely? https://codereview.chromium.org/23537002/diff/7001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:353: "description": "The type of the progress event. BEGIN_ENTRY_COPY is called before for each entry copy, END_ENTRY_COPY is called after for each entry copy. PROGRESS is called periodically during copy. SUCCESS is called after all copy is completed. ERROR is called when an error is found." nits: s/called/fired/ as this is an event. "BEGIN_ENTRY_COPY is called before for each entry copy" -> BEFORE_ENTRY_COPY is fired for each entry before starting the copy operation" "an error is found." -> "an error occurred" "SUCCESS is called after all copy is completed" -> "SUCCESS is fired after all entries are copied" https://codereview.chromium.org/23537002/diff/7001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:363: "description": "Number of processed bytes for each entry copy. Available only for PROGRESS event. To show the progress bar, a caller needs to pre-compute the size of files being copied." Maybe: Number of processed bytes for each entry copy. -> Number of processed bytes for the entry currently being copied.
Added comments to show an example event sequence. PTAL? https://codereview.chromium.org/23537002/diff/7001/chrome/common/extensions/a... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/7001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:352: "enum": ["BEGIN_ENTRY_COPY", "END_ENTRY_COPY", "PROGRESS", "SUCCESS", "ERROR"], On 2013/08/27 06:57:26, satorux1 wrote: > Why BEGIN_ENTRY_COPY instead of BEGIN_FILE_COPY? I'm guessing that "ENTRY" means > either of a file or a directory. How to report a directory copy? How will the > "size" field look like? > > If we are to copy a directory with thousands of sub directories + one very big > file, how to show a progress bar nicely? Yes, {BEGIN_END}_ENTRY_COPY are fired not only for files but also for directories. Files.app can assign some size to the directory virtually, so that it can show a smoother progress bar, as directory creation is notified to it by END_ENTRY_COPY. However, assigning the virtual size sounds a kind of hack to show the UI nicer, so IMHO we should avoid to introduce such stuff in backend/private API layer. https://codereview.chromium.org/23537002/diff/7001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:353: "description": "The type of the progress event. BEGIN_ENTRY_COPY is called before for each entry copy, END_ENTRY_COPY is called after for each entry copy. PROGRESS is called periodically during copy. SUCCESS is called after all copy is completed. ERROR is called when an error is found." On 2013/08/27 06:57:26, satorux1 wrote: > nits: > > s/called/fired/ as this is an event. > > "BEGIN_ENTRY_COPY is called before for each entry copy" -> BEFORE_ENTRY_COPY is > fired for each entry before starting the copy operation" > > > "an error is found." -> "an error occurred" > > "SUCCESS is called after all copy is completed" -> "SUCCESS is fired after all > entries are copied" Done. https://codereview.chromium.org/23537002/diff/7001/chrome/common/extensions/a... chrome/common/extensions/api/file_browser_private.json:363: "description": "Number of processed bytes for each entry copy. Available only for PROGRESS event. To show the progress bar, a caller needs to pre-compute the size of files being copied." On 2013/08/27 06:57:26, satorux1 wrote: > Maybe: > Number of processed bytes for each entry copy. > -> > Number of processed bytes for the entry currently being copied. Done.
https://codereview.chromium.org/23537002/diff/13001/chrome/common/extensions/... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/13001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:358: "description": "URL for the entry currently being copied. This field is particularly useful when a directory copy is initiated with startCopy(). The field tells what file in that directory is now being copied." what file -> what file/directory because a directory is also handled here. https://codereview.chromium.org/23537002/diff/13001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:363: "description": "Number of processed bytes for the entry currently being copied. Available only for PROGRESS event. To show the progress bar, a caller needs to pre-compute the size of files being copied." Please document that this field is always zero for if the entry currently being copied is a directory. BTW, do we want a field like "isDirectory" in the struct?
PTAL? https://codereview.chromium.org/23537002/diff/13001/chrome/common/extensions/... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/13001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:358: "description": "URL for the entry currently being copied. This field is particularly useful when a directory copy is initiated with startCopy(). The field tells what file in that directory is now being copied." On 2013/08/27 08:24:34, satorux1 wrote: > what file -> what file/directory because a directory is also handled here. Done. https://codereview.chromium.org/23537002/diff/13001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:363: "description": "Number of processed bytes for the entry currently being copied. Available only for PROGRESS event. To show the progress bar, a caller needs to pre-compute the size of files being copied." On 2013/08/27 08:24:34, satorux1 wrote: > Please document that this field is always zero for if the entry currently being > copied is a directory. > > BTW, do we want a field like "isDirectory" in the struct? Changed "the entry" -> "the file". PROGRESS should be fired only for files. (See also example below). I don't think we need isDirectory for now, because 1) currently there is no requirement for it, 2) if client needs to know the source (more), it is necessary to resolve the url to entry, and 3) we expect the entry is traversed before the copy task and known (to pre-compute the size of files being copied for progress bar).
https://codereview.chromium.org/23537002/diff/13001/chrome/common/extensions/... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/13001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:363: "description": "Number of processed bytes for the entry currently being copied. Available only for PROGRESS event. To show the progress bar, a caller needs to pre-compute the size of files being copied." On 2013/08/27 09:21:35, hidehiko wrote: > On 2013/08/27 08:24:34, satorux1 wrote: > > Please document that this field is always zero for if the entry currently > being > > copied is a directory. > > > > BTW, do we want a field like "isDirectory" in the struct? > > Changed "the entry" -> "the file". > PROGRESS should be fired only for files. (See also example below). Hmm, i thought we wanted PROGRESS for directories. How do you monitor the progress if you are copying a directory with thousands of empty directories? > > I don't think we need isDirectory for now, because > 1) currently there is no requirement for it, > 2) if client needs to know the source (more), it is necessary to resolve the url > to entry, and > 3) we expect the entry is traversed before the copy task and known (to > pre-compute the size of files being copied for progress bar). >
PTAL? https://codereview.chromium.org/23537002/diff/13001/chrome/common/extensions/... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/13001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:363: "description": "Number of processed bytes for the entry currently being copied. Available only for PROGRESS event. To show the progress bar, a caller needs to pre-compute the size of files being copied." On 2013/08/27 09:50:04, satorux1 wrote: > On 2013/08/27 09:21:35, hidehiko wrote: > > On 2013/08/27 08:24:34, satorux1 wrote: > > > Please document that this field is always zero for if the entry currently > > being > > > copied is a directory. > > > > > > BTW, do we want a field like "isDirectory" in the struct? > > > > Changed "the entry" -> "the file". > > PROGRESS should be fired only for files. (See also example below). > > Hmm, i thought we wanted PROGRESS for directories. How do you monitor the > progress if you are copying a directory with thousands of empty directories? > > > > > I don't think we need isDirectory for now, because > > 1) currently there is no requirement for it, > > 2) if client needs to know the source (more), it is necessary to resolve the > url > > to entry, and > > 3) we expect the entry is traversed before the copy task and known (to > > pre-compute the size of files being copied for progress bar). > > > BEGIN_ENTRY_COPY and END_ENTRY_COPY are fired for both files and directories. PROGRESS is fired only for files. Files.app can know each directory creation by listening {BEGIN,END}_ENTRY_COPY, and then can set appropriate progress.
https://codereview.chromium.org/23537002/diff/13001/chrome/common/extensions/... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/13001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:363: "description": "Number of processed bytes for the entry currently being copied. Available only for PROGRESS event. To show the progress bar, a caller needs to pre-compute the size of files being copied." On 2013/08/27 09:56:43, hidehiko wrote: > On 2013/08/27 09:50:04, satorux1 wrote: > > On 2013/08/27 09:21:35, hidehiko wrote: > > > On 2013/08/27 08:24:34, satorux1 wrote: > > > > Please document that this field is always zero for if the entry currently > > > being > > > > copied is a directory. > > > > > > > > BTW, do we want a field like "isDirectory" in the struct? > > > > > > Changed "the entry" -> "the file". > > > PROGRESS should be fired only for files. (See also example below). > > > > Hmm, i thought we wanted PROGRESS for directories. How do you monitor the > > progress if you are copying a directory with thousands of empty directories? > > > > > > > > I don't think we need isDirectory for now, because > > > 1) currently there is no requirement for it, > > > 2) if client needs to know the source (more), it is necessary to resolve the > > url > > > to entry, and > > > 3) we expect the entry is traversed before the copy task and known (to > > > pre-compute the size of files being copied for progress bar). > > > > > > > BEGIN_ENTRY_COPY and END_ENTRY_COPY are fired for both files and directories. > PROGRESS is fired only for files. That makes perfect sense. I forgot about the BEGIN/ENTRY stuff. > Files.app can know each directory creation by listening {BEGIN,END}_ENTRY_COPY, > and then can set appropriate progress.
The API design LGTM. minor nits: https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:353: "description": "The type of the progress event. BEGIN_ENTRY_COPY is fired for each entry before starting the copy operation, END_ENTRY_COPY is fired for each entry after ending the copy operation. PROGRESS is fired periodically during copy. SUCCESS is fired after all entries are copied. ERROR is fired when an error occurs." Some clarification: BEGIN_ENTRY_COPY is fired for each entry (file or directory) PROGRESS is fired periodically to report progress of a file copy (not directory) https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:363: "description": "Number of processed bytes for the file currently being copied. Available only for PROGRESS event. To show the progress bar, a caller needs to pre-compute the size of files being copied." for the file (not directory) it's redundant but a bit of redundancy would help https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:868: "description": "Starts to copy an entry. If the source is directory, the copy is done recursively.", Starts to copy an entry to another directory https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:878: "description": "URL of the destination location." URL of the destination directory. (to make it clear the destination should be a directory) https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:901: "description": "ID of the copy task to be cancelled." I thought it's canceled but cancelled seems to be slightly more popular inside chrome tree: % git grep -w cancelled |wc -l 782 % git grep -w canceled |wc -l 634 https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:1279: // copy a/ to x/ recursively. The events will be: remoe the trailing /? to be precise, the trailing / shouldn't be part of the canonical path. a/ -> a x/ -> x https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:1282: // END_ENTRY_COPY a/ add a blank line between each entry?
Thank you for your review. PTAL? https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:353: "description": "The type of the progress event. BEGIN_ENTRY_COPY is fired for each entry before starting the copy operation, END_ENTRY_COPY is fired for each entry after ending the copy operation. PROGRESS is fired periodically during copy. SUCCESS is fired after all entries are copied. ERROR is fired when an error occurs." On 2013/08/29 08:32:32, satorux1 wrote: > Some clarification: > > BEGIN_ENTRY_COPY is fired for each entry (file or directory) > > PROGRESS is fired periodically to report progress of a file copy (not directory) > Done. https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:363: "description": "Number of processed bytes for the file currently being copied. Available only for PROGRESS event. To show the progress bar, a caller needs to pre-compute the size of files being copied." On 2013/08/29 08:32:32, satorux1 wrote: > for the file (not directory) > > it's redundant but a bit of redundancy would help Done. https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:868: "description": "Starts to copy an entry. If the source is directory, the copy is done recursively.", On 2013/08/29 08:32:32, satorux1 wrote: > Starts to copy an entry to another directory The destination directory can be same. E.g. copy to a/b.txt to a/c.txt. https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:878: "description": "URL of the destination location." On 2013/08/29 08:32:32, satorux1 wrote: > URL of the destination directory. > > (to make it clear the destination should be a directory) Per offline discussion, I split the url into two; parent: the url of the destination directory. newName: the name of the new entry. The purpose is: 1) to avoid creating a URL which doesn't yet exist. 2) to follow the FileSystem API copyTo's manner. https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:901: "description": "ID of the copy task to be cancelled." On 2013/08/29 08:32:32, satorux1 wrote: > I thought it's canceled but cancelled seems to be slightly more popular inside > chrome tree: > > % git grep -w cancelled |wc -l > 782 > % git grep -w canceled |wc -l > 634 IIRC, cancelled for UK and canceled for US. I actually grep'ed to choose more popular one. https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:1279: // copy a/ to x/ recursively. The events will be: On 2013/08/29 08:32:32, satorux1 wrote: > remoe the trailing /? to be precise, the trailing / shouldn't be part of the > canonical path. > > a/ -> a > x/ -> x > Done. https://codereview.chromium.org/23537002/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:1282: // END_ENTRY_COPY a/ On 2013/08/29 08:32:32, satorux1 wrote: > add a blank line between each entry? Done.
Thank you for addressing comments! LGTM https://codereview.chromium.org/23537002/diff/26001/chrome/common/extensions/... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/26001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:828: "description": "Name of the new entry." This should not contain '/', right? Would be nice to mention about it.
Thank you for your review. https://codereview.chromium.org/23537002/diff/26001/chrome/common/extensions/... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/26001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:828: "description": "Name of the new entry." On 2013/08/29 10:03:01, satorux1 wrote: > This should not contain '/', right? Would be nice to mention about it. Done.
Ben, could you kindly review this as an OWNER? Thanks, - hidehiko
lgtm https://codereview.chromium.org/23537002/diff/30001/chrome/common/extensions/... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/30001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:823: "description": "Starts to copy an entry. If the source is directory, the copy is done recursively.", nit: "source is directory" -> "source is a directory" https://codereview.chromium.org/23537002/diff/30001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:842: "type": "function", Consider making this optional
lgtm
Thank you for your review. https://codereview.chromium.org/23537002/diff/30001/chrome/common/extensions/... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/30001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:823: "description": "Starts to copy an entry. If the source is directory, the copy is done recursively.", On 2013/08/30 05:22:10, Antony Sargent wrote: > nit: "source is directory" -> "source is a directory" Done. https://codereview.chromium.org/23537002/diff/30001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:842: "type": "function", On 2013/08/30 05:22:10, Antony Sargent wrote: > Consider making this optional Thank you for your feedback. Currently our client needs this callback always, so please let me keep this as mandatory.
lgtm + one minor question. https://codereview.chromium.org/23537002/diff/44001/chrome/common/extensions/... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/44001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:331: "enum": ["BEGIN_ENTRY_COPY", "END_ENTRY_COPY", "PROGRESS", "SUCCESS", "ERROR"], nit: afaik most APIs use lower-letters for enum. Is there a particular reason you use capital-letters here? (If it's for consistency within the module I think it's fine as this one's private API)
On 2013/08/30 06:59:23, kinuko wrote: > lgtm + one minor question. > > https://codereview.chromium.org/23537002/diff/44001/chrome/common/extensions/... > File chrome/common/extensions/api/file_browser_private.json (right): > > https://codereview.chromium.org/23537002/diff/44001/chrome/common/extensions/... > chrome/common/extensions/api/file_browser_private.json:331: "enum": > ["BEGIN_ENTRY_COPY", "END_ENTRY_COPY", "PROGRESS", "SUCCESS", "ERROR"], > nit: afaik most APIs use lower-letters for enum. Is there a particular reason > you use capital-letters here? (If it's for consistency within the module I think > it's fine as this one's private API) looks like asargent beat me to it :D
Thank you all for your review! https://codereview.chromium.org/23537002/diff/44001/chrome/common/extensions/... File chrome/common/extensions/api/file_browser_private.json (right): https://codereview.chromium.org/23537002/diff/44001/chrome/common/extensions/... chrome/common/extensions/api/file_browser_private.json:331: "enum": ["BEGIN_ENTRY_COPY", "END_ENTRY_COPY", "PROGRESS", "SUCCESS", "ERROR"], On 2013/08/30 06:59:23, kinuko wrote: > nit: afaik most APIs use lower-letters for enum. Is there a particular reason > you use capital-letters here? (If it's for consistency within the module I think > it's fine as this one's private API) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hidehiko@chromium.org/23537002/34001
Message was sent while issue was closed.
Change committed as 220603 |
