|
|
Description[fsp] Cleanup IDL.
This CL adds descriptions to dictionaries.
TEST=Tested manually with a test server.
BUG=248427
Committed: https://crrev.com/5e5d8cab910cf1f3e4b4101a4c8fe279b9db823a
Cr-Commit-Position: refs/heads/master@{#293121}
Patch Set 1 #
Total comments: 27
Patch Set 2 : Fixes. #Patch Set 3 : Rebased. #Patch Set 4 : Rebased. #Messages
Total messages: 17 (4 generated)
mtomasz@chromium.org changed reviewers: + benwells@chromium.org
@benwells: PTAL. Besides just review, could you verify in depth English correctness? Since it's going to appear as official manual, I'd like to keep it nice. Thanks!
https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/file_system_provider.idl (right): https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:57: // An identifier of the file system. s/An/The/ https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:60: // A human-readable name of the file system. s/of/for/ https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:63: // Whether the file system supports contents changing operations. This isn't a very typical way of describing this. How about what you have below (line 76)? https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:69: // A string indentifier of the file system. Must be unique per each s/A/The/ https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:73: // A human-readable name of the file system. s/of/for/ https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:83: // An identifier of the file system to be unmounted. s/An/The/ https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:89: // An identifier of the file system to be unmounted. s/An/The/ https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:96: // An identifier of the file system related to this operation. s/An/The/ (same for all options dictionaries). https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:99: // Unique identifier of this request. The unique... (same for all options dictionaries). https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:102: // A path of the entry to fetch metadata about. s/A/The/ (same for all options dictionaries). https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:178: // Whether the operation is recursive (for directories only). I'm unclear what this means for create directory. Does it mean to create all necessary parents? https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:265: // Position in the file (in bytes) to start writing the bytes at. s/at/from/
https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/file_system_provider.idl (right): https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:45: // File size in bytes. Should it be "The file size in bytes"? https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:57: // An identifier of the file system. On 2014/09/03 01:19:38, benwells wrote: > s/An/The/ Done. https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:60: // A human-readable name of the file system. On 2014/09/03 01:19:38, benwells wrote: > s/of/for/ Done. https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:60: // A human-readable name of the file system. Should it be "The human-readable name..."? https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:63: // Whether the file system supports contents changing operations. On 2014/09/03 01:19:37, benwells wrote: > This isn't a very typical way of describing this. How about what you have below > (line 76)? Done. https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:69: // A string indentifier of the file system. Must be unique per each On 2014/09/03 01:19:38, benwells wrote: > s/A/The/ Done. https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:73: // A human-readable name of the file system. On 2014/09/03 01:19:38, benwells wrote: > s/of/for/ Done. https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:83: // An identifier of the file system to be unmounted. On 2014/09/03 01:19:38, benwells wrote: > s/An/The/ Done. https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:89: // An identifier of the file system to be unmounted. On 2014/09/03 01:19:38, benwells wrote: > s/An/The/ Done. https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:96: // An identifier of the file system related to this operation. On 2014/09/03 01:19:38, benwells wrote: > s/An/The/ > > (same for all options dictionaries). Done. https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:99: // Unique identifier of this request. On 2014/09/03 01:19:38, benwells wrote: > The unique... > > (same for all options dictionaries). https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:123: // A request ID which will be used by consecutive read/write and close Shall it be "The request ID"? WDYT? https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:178: // Whether the operation is recursive (for directories only). On 2014/09/03 01:19:38, benwells wrote: > I'm unclear what this means for create directory. Does it mean to create all > necessary parents? Yes. More information is in the comment for onCreateDirectoryRequested. I wanted to avoid duplication. WDYT?
On 2014/09/03 04:01:19, mtomasz wrote: > https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... > File chrome/common/extensions/api/file_system_provider.idl (right): > > https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/file_system_provider.idl:45: // File size in bytes. > Should it be "The file size in bytes"? > > https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/file_system_provider.idl:57: // An identifier of > the file system. > On 2014/09/03 01:19:38, benwells wrote: > > s/An/The/ > > Done. > > https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/file_system_provider.idl:60: // A human-readable > name of the file system. > On 2014/09/03 01:19:38, benwells wrote: > > s/of/for/ > > Done. > > https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/file_system_provider.idl:60: // A human-readable > name of the file system. > Should it be "The human-readable name..."? > > https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/file_system_provider.idl:63: // Whether the file > system supports contents changing operations. > On 2014/09/03 01:19:37, benwells wrote: > > This isn't a very typical way of describing this. How about what you have > below > > (line 76)? > > Done. > > https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/file_system_provider.idl:69: // A string > indentifier of the file system. Must be unique per each > On 2014/09/03 01:19:38, benwells wrote: > > s/A/The/ > > Done. > > https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/file_system_provider.idl:73: // A human-readable > name of the file system. > On 2014/09/03 01:19:38, benwells wrote: > > s/of/for/ > > Done. > > https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/file_system_provider.idl:83: // An identifier of > the file system to be unmounted. > On 2014/09/03 01:19:38, benwells wrote: > > s/An/The/ > > Done. > > https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/file_system_provider.idl:89: // An identifier of > the file system to be unmounted. > On 2014/09/03 01:19:38, benwells wrote: > > s/An/The/ > > Done. > > https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/file_system_provider.idl:96: // An identifier of > the file system related to this operation. > On 2014/09/03 01:19:38, benwells wrote: > > s/An/The/ > > > > (same for all options dictionaries). > > Done. > > https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/file_system_provider.idl:99: // Unique identifier > of this request. > On 2014/09/03 01:19:38, benwells wrote: > > The unique... > > > > (same for all options dictionaries). > > https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/file_system_provider.idl:123: // A request ID which > will be used by consecutive read/write and close > Shall it be "The request ID"? WDYT? > > https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/file_system_provider.idl:178: // Whether the > operation is recursive (for directories only). > On 2014/09/03 01:19:38, benwells wrote: > > I'm unclear what this means for create directory. Does it mean to create all > > necessary parents? > > Yes. More information is in the comment for onCreateDirectoryRequested. I wanted > to avoid duplication. WDYT? @benwells: I added some questions as comments. PTAL. Thanks.
lgtm https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/file_system_provider.idl (right): https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:123: // A request ID which will be used by consecutive read/write and close On 2014/09/03 04:01:18, mtomasz wrote: > Shall it be "The request ID"? WDYT? I think this one is OK as is. https://codereview.chromium.org/527723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_system_provider.idl:178: // Whether the operation is recursive (for directories only). On 2014/09/03 04:01:18, mtomasz wrote: > On 2014/09/03 01:19:38, benwells wrote: > > I'm unclear what this means for create directory. Does it mean to create all > > necessary parents? > > Yes. More information is in the comment for onCreateDirectoryRequested. I wanted > to avoid duplication. WDYT? OK, no problem.
The CQ bit was checked by mtomasz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/527723002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mtomasz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/527723002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 753daa1637c3f965c7a8897fea1d1f29d1f113ee
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5e5d8cab910cf1f3e4b4101a4c8fe279b9db823a Cr-Commit-Position: refs/heads/master@{#293121} |