Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10692105:
Updates file type selector for fileSystem API (Closed)
|
Created:
8 years, 5 months ago by thorogood Modified:
8 years, 4 months ago CC:
chromium-reviews, Aaron Boodman, kinuko+watch, mihaip-chromium-reviews_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionUpdates file type selector for fileSystem API
Allows developers to explicitly specify what accept types they would like to allow in their file selectors, as well as whether they allow all files as a last option.
BUG=133066
TEST=Test cases, manual testing
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150515
Patch Set 1 #Patch Set 2 : Experiment with IDL file; longer comments so it's clearer what the goal is #
Total comments: 6
Patch Set 3 : All code now inside file_system_api.cc #
Total comments: 13
Patch Set 4 : after benwells@' inline suggestions #Patch Set 5 : Now with a basic test #
Total comments: 19
Patch Set 6 : Some method cleanups, test is now a unit test #
Total comments: 2
Patch Set 7 : Broken out suggested_name stuff, added test #
Total comments: 14
Patch Set 8 : More logic in GetDescriptionIdAndExtensions #Patch Set 9 : forgot to upload suggested_extension fix #Patch Set 10 : oops, over 80 chars #
Total comments: 13
Patch Set 11 : fixed args, need_suggestion bool #
Total comments: 5
Patch Set 12 : use find() instead of iter, remove own description generation code, cleanup tests #
Total comments: 20
Patch Set 13 : more unittests, nit fixes #
Total comments: 8
Patch Set 14 : three nits, one fix #Patch Set 15 : added another test case #
Total comments: 1
Patch Set 16 : Mihai's suggestions #
Total comments: 2
Patch Set 17 : Merge to head #
Total comments: 5
Patch Set 18 : typedef, description fix, doc regen #Patch Set 19 : sync to head #Patch Set 20 : Fix wstring on Windows #Patch Set 21 : wide characters in unittest #Patch Set 22 : Fixed Windows test again #Patch Set 23 : Remove another Windows test, fix later #Patch Set 24 : Patch #23 broken #Messages
Total messages: 46 (0 generated)
Hey Ben, can you look at the IDL file here? I've expanded the comments a little bit to be a bit more helpful. If you're happy with the goal then I can get coding.
On 2012/07/12 05:48:28, thorogood wrote: > Hey Ben, can you look at the IDL file here? I've expanded the comments a little > bit to be a bit more helpful. If you're happy with the goal then I can get > coding. And by this I mean: I've an earlier version of this patch (mostly already done, in patch set 1) which I want to change a bit, so I've updated the IDL and I'm keen for you to review just that for now. Thanks!
https://chromiumcodereview.appspot.com/10692105/diff/2001/chrome/common/exten... File chrome/common/extensions/api/file_system.idl (right): https://chromiumcodereview.appspot.com/10692105/diff/2001/chrome/common/exten... chrome/common/extensions/api/file_system.idl:24: DOMString? suggestedPath; Cool :) Feel free to do in a separate patch, or in this one. Should be suggestedFilename, not Path. We've avoided making paths a part of the API. https://chromiumcodereview.appspot.com/10692105/diff/2001/chrome/common/exten... chrome/common/extensions/api/file_system.idl:29: // For example, ['image/*', 'application/json,.jso'] would display two type This is not nitting on the text, just me trying to understand. My understanding is that the syntax is - an array of filters - each filter is a string, being made up of a comma separated list of types - each type is either - an accepted MIME type filter - an accepted extension Is that right? https://chromiumcodereview.appspot.com/10692105/diff/2001/chrome/common/exten... chrome/common/extensions/api/file_system.idl:38: // specified in the accepts argument? Nit: don't phrase as a question.
forgot to publish these comments. https://chromiumcodereview.appspot.com/10692105/diff/2001/chrome/common/exten... File chrome/common/extensions/api/file_system.idl (right): https://chromiumcodereview.appspot.com/10692105/diff/2001/chrome/common/exten... chrome/common/extensions/api/file_system.idl:24: DOMString? suggestedPath; On 2012/07/12 06:10:57, benwells wrote: > Cool :) Feel free to do in a separate patch, or in this one. > > Should be suggestedFilename, not Path. We've avoided making paths a part of the > API. Done in other patch. https://chromiumcodereview.appspot.com/10692105/diff/2001/chrome/common/exten... chrome/common/extensions/api/file_system.idl:29: // For example, ['image/*', 'application/json,.jso'] would display two type On 2012/07/12 06:10:57, benwells wrote: > This is not nitting on the text, just me trying to understand. > > My understanding is that the syntax is > - an array of filters > - each filter is a string, being made up of a comma separated list of types > - each type is either > - an accepted MIME type filter > - an accepted extension > > Is that right? Yes, that's right. The aim here is to allow for all sorts of different configurations. Ideally, I'd allow the API to also accept a single string argument, which would be treated as an array of length one, for a "simple case". I'm not sure if this is really practical or worth it though. https://chromiumcodereview.appspot.com/10692105/diff/2001/chrome/common/exten... chrome/common/extensions/api/file_system.idl:38: // specified in the accepts argument? On 2012/07/12 06:10:57, benwells wrote: > Nit: don't phrase as a question. Done.
Ben, I've implemented this now. PTAL. Some of this code is stolen/duplicated from other places in the codebase, and I think potentially the GetFileTypesFromAcceptTypes could be put somewhere else. I'm not really sure if I can seriously test this either - I've got a test application you can come past and look at if you're curious.
I like the direction. Overall I would like to see: - break up the longer functions - unit test the splitting of the accept string into the SelectFileDialog type https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/browser/exte... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/browser/exte... chrome/browser/extensions/api/file_system/file_system_api.cc:157: normalized_type_list.push_back(type); Could this be made simpler by 1) converting accept_types to lower case and 2) removing spaces in accept_types before we split the string? https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/browser/exte... chrome/browser/extensions/api/file_system/file_system_api.cc:175: // Assume this is a file extension, and add it to its own place insie insie->inside https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/browser/exte... chrome/browser/extensions/api/file_system/file_system_api.cc:186: description_id = IDS_VIDEO_FILES; This will overwrite description if it is already set. https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/browser/exte... chrome/browser/extensions/api/file_system/file_system_api.cc:191: // TODO(thorogood): It's possible to add the same extension multiple times. Are you planning on fixing this? Depending on what we do with description it may not be a problem. https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/browser/exte... chrome/browser/extensions/api/file_system/file_system_api.cc:208: // ".ext1, .ext2, .other-ext". Would be nicer to have "*.ext1, *.ext2..." https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/common/exten... File chrome/common/extensions/api/file_system.idl (right): https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/common/exten... chrome/common/extensions/api/file_system.idl:30: // options - one accepting images - and the other accepting json files and Collapse the above two paragraphs into one. https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/common/exten... chrome/common/extensions/api/file_system.idl:40: // The default is true. Ditto
I've addressed your inline comments, and I'll take a stab at breaking up the functions/unittesting them now. https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/browser/exte... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/browser/exte... chrome/browser/extensions/api/file_system/file_system_api.cc:175: // Assume this is a file extension, and add it to its own place insie On 2012/07/24 06:01:27, benwells wrote: > insie->inside Done. https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/browser/exte... chrome/browser/extensions/api/file_system/file_system_api.cc:186: description_id = IDS_VIDEO_FILES; On 2012/07/24 06:01:27, benwells wrote: > This will overwrite description if it is already set. Yes. This isn't actually what we want, but the value of description_id is totally irrelevant if valid_type_count is greater than one, which will be the case if description_id has been set multiple times. (phew). This is more or less duplicated from existing code in file_select_helper.cc. https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/browser/exte... chrome/browser/extensions/api/file_system/file_system_api.cc:191: // TODO(thorogood): It's possible to add the same extension multiple times. On 2012/07/24 06:01:27, benwells wrote: > Are you planning on fixing this? Depending on what we do with description it may > not be a problem. I'm not sure. It either means modifying the net::Get*Extensions calls so they don't overwrite things, or just putting everything into a set before we generate the description ID. https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/browser/exte... chrome/browser/extensions/api/file_system/file_system_api.cc:208: // ".ext1, .ext2, .other-ext". On 2012/07/24 06:01:27, benwells wrote: > Would be nicer to have "*.ext1, *.ext2..." Done. https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/common/exten... File chrome/common/extensions/api/file_system.idl (right): https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/common/exten... chrome/common/extensions/api/file_system.idl:30: // options - one accepting images - and the other accepting json files and On 2012/07/24 06:01:27, benwells wrote: > Collapse the above two paragraphs into one. Done. https://chromiumcodereview.appspot.com/10692105/diff/6001/chrome/common/exten... chrome/common/extensions/api/file_system.idl:40: // The default is true. On 2012/07/24 06:01:27, benwells wrote: > Ditto Done.
I've now added a basic test.
https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:161: } I think the ParseAcceptValue function could be made simpler by removing spaces and making lower case before splitting. Then you just need to split. https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:193: I think you should split this whole loop out into a new function std::vector<std::string> GetDescriptionIdAndExtensions(string, int*) and use a set when constructing the return value. This will encapsulate this ugly list of if's and handle duplicate extensions. https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:198: if (valid_type_count == 0) Can valid_type_count be replaced with extensions->size(), and old_extension_size removed? https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:507: scoped_ptr<SelectFileDialog::FileTypeInfo> file_type_info( This declaration should go just before where it is first used. https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:523: } Consider splitting this chunk out into a new function https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:540: suggested_extension.erase(suggested_extension.begin()); // drop the . And this chunk https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_apitest.cc:206: // TODO(thorogood): Not really a browser test, but this is the easiest way to This should be a unit test, not a browser test
PTAL, I've tried to address most of your comments. https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:161: } On 2012/07/26 05:40:45, benwells wrote: > I think the ParseAcceptValue function could be made simpler by removing spaces > and making lower case before splitting. Then you just need to split. Done, although now I feel like inlining the method just inside GetFileTypesFromAcceptType. Thoughts, now that GetFileTypesFromAcceptType is much shorter? https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:193: On 2012/07/26 05:40:45, benwells wrote: > I think you should split this whole loop out into a new function > std::vector<std::string> GetDescriptionIdAndExtensions(string, int*) and use a > set when constructing the return value. This will encapsulate this ugly list of > if's and handle duplicate extensions. I'm not sure if I completely understand you, but what I've done is: - written GetDescriptionIdAndExtensions, and made it return a std::vector - insert everything from that vector into a std::set held over the lifetime of GetFileTypesFromAcceptType - at the end of GetFileTypesFromAcceptType, copy everything from the std::set into the output variable "extensions" I've also updated the comment now on line 208 to be clearer as to why we only sometimes generate a description based on explicit definitions. https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:198: if (valid_type_count == 0) On 2012/07/26 05:40:45, benwells wrote: > Can valid_type_count be replaced with extensions->size(), and old_extension_size > removed? Probably not. The idea here is that we have a few magical types: image/*, audio/* and video/*, which actually expand to a much larger list each (but this also applies to say, text/html, which expands to "htm" and "html"). So, checking for extensions->size() == 1 doesn't really do what we want here. The alternative is to change the logic which sets description_id; to only set if it its currently unset, and set it to a dummy "generate our own description" value if we see anything that isn't "image/*", "audio/*" or "video/*". Phew. https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:507: scoped_ptr<SelectFileDialog::FileTypeInfo> file_type_info( On 2012/07/26 05:40:45, benwells wrote: > This declaration should go just before where it is first used. It is already, more or less, since it is updated inside the "if (options) {" block. https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_apitest.cc:206: // TODO(thorogood): Not really a browser test, but this is the easiest way to On 2012/07/26 05:40:45, benwells wrote: > This should be a unit test, not a browser test Done.
https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:161: } On 2012/07/26 07:39:05, thorogood wrote: > On 2012/07/26 05:40:45, benwells wrote: > > I think the ParseAcceptValue function could be made simpler by removing spaces > > and making lower case before splitting. Then you just need to split. > > Done, although now I feel like inlining the method just inside > GetFileTypesFromAcceptType. Thoughts, now that GetFileTypesFromAcceptType is > much shorter? I prefer it like this. https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:193: On 2012/07/26 07:39:05, thorogood wrote: > On 2012/07/26 05:40:45, benwells wrote: > > I think you should split this whole loop out into a new function > > std::vector<std::string> GetDescriptionIdAndExtensions(string, int*) and use a > > set when constructing the return value. This will encapsulate this ugly list > of > > if's and handle duplicate extensions. > > I'm not sure if I completely understand you, but what I've done is: > > - written GetDescriptionIdAndExtensions, and made it return a std::vector > - insert everything from that vector into a std::set held over the lifetime of > GetFileTypesFromAcceptType > - at the end of GetFileTypesFromAcceptType, copy everything from the std::set > into the output variable "extensions" > > I've also updated the comment now on line 208 to be clearer as to why we only > sometimes generate a description based on explicit definitions. Hmm, I meant somethign slightly different, but this works. With this you may as well return a set, not vector. It would be nice to clean up the description logic somehow, it's pretty confusing. One approach would be to only update the desktop in the GetDescriptionIdAndExtensions function if there it is currently blank. Otherwise set it to -1 or something. But -1 magic value is not that nice either. https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:198: if (valid_type_count == 0) On 2012/07/26 07:39:05, thorogood wrote: > On 2012/07/26 05:40:45, benwells wrote: > > Can valid_type_count be replaced with extensions->size(), and > old_extension_size > > removed? > > Probably not. The idea here is that we have a few magical types: image/*, > audio/* and video/*, which actually expand to a much larger list each (but this > also applies to say, text/html, which expands to "htm" and "html"). So, checking > for extensions->size() == 1 doesn't really do what we want here. > > The alternative is to change the logic which sets description_id; to only set if > it its currently unset, and set it to a dummy "generate our own description" > value if we see anything that isn't "image/*", "audio/*" or "video/*". > > Phew. Oh, ok, i see it is used in two places, not just the ==0. https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:507: scoped_ptr<SelectFileDialog::FileTypeInfo> file_type_info( On 2012/07/26 07:39:05, thorogood wrote: > On 2012/07/26 05:40:45, benwells wrote: > > This declaration should go just before where it is first used. > > It is already, more or less, since it is updated inside the "if (options) {" > block. Ah, ok. https://chromiumcodereview.appspot.com/10692105/diff/15002/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/15002/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_apitest.cc:207: class FileSystemApiUnitTest : public testing::Test { This should go in file file_system_unittest.cc, and be compiled into a different executable than browser_tests (probably unit_tests_.
https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:193: On 2012/07/26 08:10:23, benwells wrote: > On 2012/07/26 07:39:05, thorogood wrote: > > On 2012/07/26 05:40:45, benwells wrote: > > > I think you should split this whole loop out into a new function > > > std::vector<std::string> GetDescriptionIdAndExtensions(string, int*) and use > a > > > set when constructing the return value. This will encapsulate this ugly list > > of > > > if's and handle duplicate extensions. > > > > I'm not sure if I completely understand you, but what I've done is: > > > > - written GetDescriptionIdAndExtensions, and made it return a std::vector > > - insert everything from that vector into a std::set held over the lifetime of > > GetFileTypesFromAcceptType > > - at the end of GetFileTypesFromAcceptType, copy everything from the std::set > > into the output variable "extensions" > > > > I've also updated the comment now on line 208 to be clearer as to why we only > > sometimes generate a description based on explicit definitions. > > Hmm, I meant somethign slightly different, but this works. With this you may as > well return a set, not vector. > > It would be nice to clean up the description logic somehow, it's pretty > confusing. One approach would be to only update the desktop in the > GetDescriptionIdAndExtensions function if there it is currently blank. Otherwise > set it to -1 or something. But -1 magic value is not that nice either. I've gone over this a few times in my head and I don't think there's any better way to have the logic. I'm also not fussed about returning a set. I think by returning a vector we make the caller's life easier (since the SelectFileDialog::FileTypeInfo struct requires a vector). So I've left this unless you have strong feelings otherwise. https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:523: } On 2012/07/26 05:40:45, benwells wrote: > Consider splitting this chunk out into a new function I've considered it, but I think the interaction with error_ and multiple variables is too much for now, it's a bit contrived. This section isn't that long anyway. https://chromiumcodereview.appspot.com/10692105/diff/15001/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:540: suggested_extension.erase(suggested_extension.begin()); // drop the . On 2012/07/26 05:40:45, benwells wrote: > And this chunk Done, I've even added a test. https://chromiumcodereview.appspot.com/10692105/diff/15002/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/15002/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_apitest.cc:207: class FileSystemApiUnitTest : public testing::Test { On 2012/07/26 08:10:23, benwells wrote: > This should go in file file_system_unittest.cc, and be compiled into a different > executable than browser_tests (probably unit_tests_. Done.
https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:139: std::vector<std::string> ParseAcceptValue(const std::string& accept_types_) { Put a comment explaining how it's being parsed. https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:152: std::vector<FilePath::StringType> GetDescriptionIdAndExtensions( This is all convoluted, can be simpler by stopping the vector -> set -> vector transformation and simplifying the 'has valid description' logic. To do this change GetDescriptionIdAndExtension to have this signature: std::vector<FilePath:StringType> Get...ions(const std::vector<std::string> type_list, int* description_id, bool* has_description). This gets the extensions and description id for all the types, i.e. it has the loop, and a bit of logic to track if it gets multiple valid description ids. Internally it uses a set and at return converts to a vector. https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:154: int *description_id) { int *des -> int* des https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:183: string16 *description) { string16 *des -> string16* des https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:202: if (valid_type_count == 0) This can change to if (extension_set.size() > 0) https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:205: if (!description_id || valid_type_count > 1) { This can change to if (has_valid_description) { .... } else { .... } https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:494: // TODO(thorogood): If suggested_extension is non-empty, ensure that Implement this now or remove the suggested_extension calculation
https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:139: std::vector<std::string> ParseAcceptValue(const std::string& accept_types_) { On 2012/07/27 03:43:56, benwells wrote: > Put a comment explaining how it's being parsed. Done. https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:152: std::vector<FilePath::StringType> GetDescriptionIdAndExtensions( On 2012/07/27 03:43:56, benwells wrote: > This is all convoluted, can be simpler by stopping the vector -> set -> vector > transformation and simplifying the 'has valid description' logic. I certainly agree that it's all convoluted. :) > > To do this change GetDescriptionIdAndExtension to have this signature: > std::vector<FilePath:StringType> Get...ions(const std::vector<std::string> > type_list, int* description_id, bool* has_description). > > This gets the extensions and description id for all the types, i.e. it has the > loop, and a bit of logic to track if it gets multiple valid description ids. > Internally it uses a set and at return converts to a vector. So, I didn't implement your suggestion verbatim. Issues: - net::Get*Extensions* methods operate on a std::vector, so I've had to use an "inner" vector and then add to the set (so we still have the vector -> set -> vector, but at least it's within one method) - has_description felt superfluous, since I ended up only using it for internal state inside GetDescriptionIdAndExtensions (I can clear description_id if I've already seen a previous description) What I've done is generally simplified the logic. Basically now the only time you'll see a fixed description is if you call this method with a single "magic" type, i.e., "image/*", "audio/*", or "video/*". If you specify more than one valid type, we'll always fall out to expanding the whole list (*.jpeg, *.jpg, ...), and I think this method is easier to grok because of it. https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:154: int *description_id) { On 2012/07/27 03:43:56, benwells wrote: > int *des -> int* des Done. https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:183: string16 *description) { On 2012/07/27 03:43:56, benwells wrote: > string16 *des -> string16* des Done. https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:202: if (valid_type_count == 0) On 2012/07/27 03:43:56, benwells wrote: > This can change to if (extension_set.size() > 0) Done, well, to if (extension_set.empty())... https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:205: if (!description_id || valid_type_count > 1) { On 2012/07/27 03:43:56, benwells wrote: > This can change to if (has_valid_description) { .... } else { .... } I've changed it just to "!description_id", since that's all we return now from GetDsecriptionIdAndExtensions. https://chromiumcodereview.appspot.com/10692105/diff/22003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:494: // TODO(thorogood): If suggested_extension is non-empty, ensure that On 2012/07/27 03:43:56, benwells wrote: > Implement this now or remove the suggested_extension calculation Done, taken the implemented approach.
https://chromiumcodereview.appspot.com/10692105/diff/26002/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/26002/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:213: // We don't have a description ID; build a string like What happens if we don't supply a description? The file type info field being set is called description_overrides, what is the non-overidden description? https://chromiumcodereview.appspot.com/10692105/diff/26002/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:488: FilePath::StringType suggested_extension, use const& https://chromiumcodereview.appspot.com/10692105/diff/26002/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:489: const std::vector<std::string>* accepts, const& https://chromiumcodereview.appspot.com/10692105/diff/26002/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:490: const bool* acceptsAllTypes) { bool https://chromiumcodereview.appspot.com/10692105/diff/26002/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:509: suggested_extension = FilePath::StringType(); Introduce a variable instead of using suggested_extension.empty() to indicate something. You could call it need_all_files or something. https://chromiumcodereview.appspot.com/10692105/diff/26002/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:517: if (acceptsAllTypes && !file_type_info->extensions.empty() What is the default behaviour if you have include_all_files false but file_type_info->extensions empty or not containing the suggested extension? Does the select file dialog do something sensible? https://chromiumcodereview.appspot.com/10692105/diff/26002/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:577: options->accepts_all_types.get()); Doesn't this need another argument?
https://chromiumcodereview.appspot.com/10692105/diff/26002/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/26002/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:213: // We don't have a description ID; build a string like On 2012/07/27 05:59:24, benwells wrote: > What happens if we don't supply a description? The file type info field being > set is called description_overrides, what is the non-overidden description? select_file_helper.cc:292 tells us that at least on Windows, the select file dialog "uses the first extension in the list to form the description, like "EHTML Files". This is not what we want.". I've also just tried it (by commenting out the description append) on Linux, and it generates almost exactly what we do here, but without the spaces (i.e., "*.ext1,*.ext2"). I can find out for completeness what Mac does, but I feel like defining it is better. https://chromiumcodereview.appspot.com/10692105/diff/26002/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:489: const std::vector<std::string>* accepts, On 2012/07/27 05:59:24, benwells wrote: > const& This is sourced from calling .get() on a scoped_ptr<std::vector<std::string>>. If I dereference it and it is NULL, that would be bad. https://chromiumcodereview.appspot.com/10692105/diff/26002/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:490: const bool* acceptsAllTypes) { On 2012/07/27 05:59:24, benwells wrote: > bool as above. https://chromiumcodereview.appspot.com/10692105/diff/26002/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:509: suggested_extension = FilePath::StringType(); On 2012/07/27 05:59:24, benwells wrote: > Introduce a variable instead of using suggested_extension.empty() to indicate > something. You could call it need_all_files or something. I've called it need_suggestion, since we mark it as false when we find it - and no longer need to keep searching. need_all_files is a harder state to maintain knowledge of :) https://chromiumcodereview.appspot.com/10692105/diff/26002/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:517: if (acceptsAllTypes && !file_type_info->extensions.empty() On 2012/07/27 05:59:24, benwells wrote: > What is the default behaviour if you have include_all_files false but > file_type_info->extensions empty or not containing the suggested extension? Does > the select file dialog do something sensible? At least on Linux, it doesn't do anything sensible - it prevents you from selecting any files. https://chromiumcodereview.appspot.com/10692105/diff/26002/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:577: options->accepts_all_types.get()); On 2012/07/27 05:59:24, benwells wrote: > Doesn't this need another argument? Yep, I probably should have tested this. D'oh.
https://chromiumcodereview.appspot.com/10692105/diff/24003/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/24003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:513: if (need_suggestion) { you can use find here instead of iterating. https://chromiumcodereview.appspot.com/10692105/diff/24003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:557: scoped_ptr<SelectFileDialog::FileTypeInfo> file_type_info( Does this need to be a pointer?
https://chromiumcodereview.appspot.com/10692105/diff/24003/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/24003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:213: // We don't have a description ID; build a string like As discussed, I've reverted this bit - I'm going to assume that select_file_dialog_*.cc is canonical, and perhaps treat the _win.cc approach as a bug. This method is thus rather short, I could probably bring back the contents of GetDescriptionIdAndExtensions but this is probably fine too. https://chromiumcodereview.appspot.com/10692105/diff/24003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:513: if (need_suggestion) { On 2012/07/27 06:36:21, benwells wrote: > you can use find here instead of iterating. Done. https://chromiumcodereview.appspot.com/10692105/diff/24003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:557: scoped_ptr<SelectFileDialog::FileTypeInfo> file_type_info( On 2012/07/27 06:36:21, benwells wrote: > Does this need to be a pointer? I guess not. Fixed.
Feels close https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:139: // Parses a comma-seperated list of accept types into a std::vector, e.g., /Parses/Parses and normalizes/ https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:141: std::vector<std::string> ParseAcceptValue(const std::string& accept_types_) { Trailing underscore represents a field on a class. Don't use for parameter names. Instead, have your local variable communicate how it is different e.g. normalized_accept_types. https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:154: std::vector<FilePath::StringType> GetDescriptionIdAndExtensions( Now the description processing is so much simpler this can go back to the body of GetFileTypesFromAcceptType. https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:189: if (has_valid_types) Does this mean something like "video/*,.xyz" will not use the video description override? https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:478: file_type_info->include_all_files = false; Default should be true. https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:495: // Optionally hunt for our suggested_extension, ensuring it is one of the Nit: what does optionally mean here? https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:522: if (suggested_name->IsAbsolute()) { Nit: don't need braces. https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:526: if (!suggested_extension->empty()) { Nit: don't need braces. https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api_unittest.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api_unittest.cc:33: EXPECT_EQ(file_type_info.extensions.size(), (size_t) 1); Should test that the extensions are as expected. https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api_unittest.cc:35: // Test that not satisfying the extension will force all types. Add more tests: * test with multiple accept types in the list * test with special mime type that causes description override
https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:139: // Parses a comma-seperated list of accept types into a std::vector, e.g., On 2012/07/30 01:07:16, benwells wrote: > /Parses/Parses and normalizes/ Done. https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:141: std::vector<std::string> ParseAcceptValue(const std::string& accept_types_) { On 2012/07/30 01:07:16, benwells wrote: > Trailing underscore represents a field on a class. Don't use for parameter > names. Instead, have your local variable communicate how it is different e.g. > normalized_accept_types. Done. https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:154: std::vector<FilePath::StringType> GetDescriptionIdAndExtensions( On 2012/07/30 01:07:16, benwells wrote: > Now the description processing is so much simpler this can go back to the body > of GetFileTypesFromAcceptType. Done, PTAL. https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:189: if (has_valid_types) On 2012/07/30 01:07:16, benwells wrote: > Does this mean something like "video/*,.xyz" will not use the video description > override? Yes. As discussed, I'm changing this code so that: if we see a major "foo/*" with a matching description, we'll use that irrespective of what other extensions we add to the list. However on the corner case that we see more than one, e.g., "image/*" and "video/*", I've change the logic to fall back to no description at all (urgh). https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:478: file_type_info->include_all_files = false; On 2012/07/30 01:07:16, benwells wrote: > Default should be true. Nice catch, forgot about that from the IDL. https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:495: // Optionally hunt for our suggested_extension, ensuring it is one of the On 2012/07/30 01:07:16, benwells wrote: > Nit: what does optionally mean here? clarified this comment, it's now "If we still need to find suggested_extension, hunt for it inside the extensions returned from GetFileTypesFromAcceptType.". https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:522: if (suggested_name->IsAbsolute()) { On 2012/07/30 01:07:16, benwells wrote: > Nit: don't need braces. Done. https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:526: if (!suggested_extension->empty()) { On 2012/07/30 01:07:16, benwells wrote: > Nit: don't need braces. Done. https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api_unittest.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api_unittest.cc:33: EXPECT_EQ(file_type_info.extensions.size(), (size_t) 1); On 2012/07/30 01:07:16, benwells wrote: > Should test that the extensions are as expected. Done. https://chromiumcodereview.appspot.com/10692105/diff/17027/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api_unittest.cc:35: // Test that not satisfying the extension will force all types. On 2012/07/30 01:07:16, benwells wrote: > Add more tests: > * test with multiple accept types in the list > * test with special mime type that causes description override Done.
https://chromiumcodereview.appspot.com/10692105/diff/21003/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/21003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:156: bool GetFileTypesFromAcceptType(const std::string& accept_type, Nit: parameter list alignment. https://chromiumcodereview.appspot.com/10692105/diff/21003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:190: description_id = 0; // We already have an accept type with a description; What if we have three mime types? I believe this logic will make us use the last one. https://chromiumcodereview.appspot.com/10692105/diff/21003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:463: file_type_info->include_all_files = true; Nit: you can lose the else if you first set to true. https://chromiumcodereview.appspot.com/10692105/diff/21003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:484: suggested_extension) != extensions.end()) { Two nits: extra 4 space indent for these two lines as we're in the find parameter list; can maybe put the last two lines into one.
https://chromiumcodereview.appspot.com/10692105/diff/21003/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/21003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:156: bool GetFileTypesFromAcceptType(const std::string& accept_type, On 2012/07/30 07:19:32, benwells wrote: > Nit: parameter list alignment. Done. https://chromiumcodereview.appspot.com/10692105/diff/21003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:190: description_id = 0; // We already have an accept type with a description; On 2012/07/30 07:19:32, benwells wrote: > What if we have three mime types? I believe this logic will make us use the last > one. Nice catch. Fixed now. Also added a test. https://chromiumcodereview.appspot.com/10692105/diff/21003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:463: file_type_info->include_all_files = true; On 2012/07/30 07:19:32, benwells wrote: > Nit: you can lose the else if you first set to true. Done. https://chromiumcodereview.appspot.com/10692105/diff/21003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:484: suggested_extension) != extensions.end()) { On 2012/07/30 07:19:32, benwells wrote: > Two nits: extra 4 space indent for these two lines as we're in the find > parameter list; can maybe put the last two lines into one. Done.
https://chromiumcodereview.appspot.com/10692105/diff/28001/chrome/common/exte... File chrome/common/extensions/api/file_system.idl (right): https://chromiumcodereview.appspot.com/10692105/diff/28001/chrome/common/exte... chrome/common/extensions/api/file_system.idl:27: // by type. For example, <code>['image/*', 'text/html,.jso']</code> Apologies if this was discussed already, but the embedding of arrays inside of strings seems a bit hacky, and error prone for the developer. How about something a bit more structured. Make the param into: AcceptOption[]? accepts; With AcceptOption being: dictionary AcceptOption { // Optional, if not present, a description will be auto-generated DOMString? description; // One of mimeTypes or extensions must contain at least one element. DOMString[]? mimeTypes; DOMString[]? extensions; }
On 2012/07/30 23:33:03, Mihai Parparita wrote: > https://chromiumcodereview.appspot.com/10692105/diff/28001/chrome/common/exte... > File chrome/common/extensions/api/file_system.idl (right): > > https://chromiumcodereview.appspot.com/10692105/diff/28001/chrome/common/exte... > chrome/common/extensions/api/file_system.idl:27: // by type. For example, > <code>['image/*', 'text/html,.jso']</code> > Apologies if this was discussed already, but the embedding of arrays inside of > strings seems a bit hacky, and error prone for the developer. How about > something a bit more structured. Make the param into: > > AcceptOption[]? accepts; > > With AcceptOption being: > > dictionary AcceptOption { > // Optional, if not present, a description will be auto-generated > DOMString? description; > // One of mimeTypes or extensions must contain at least one element. > DOMString[]? mimeTypes; > DOMString[]? extensions; > } We went with this approach to be consistent with the file type inputs accept attribute; however just yesterday we were talking about being able to set the description somehow. In the current approach this would be even uglier. The format propised here by Mihai is much more extendable.
On 2012/07/31 00:40:29, benwells wrote: > On 2012/07/30 23:33:03, Mihai Parparita wrote: > > > https://chromiumcodereview.appspot.com/10692105/diff/28001/chrome/common/exte... > > File chrome/common/extensions/api/file_system.idl (right): > > > > > https://chromiumcodereview.appspot.com/10692105/diff/28001/chrome/common/exte... > > chrome/common/extensions/api/file_system.idl:27: // by type. For example, > > <code>['image/*', 'text/html,.jso']</code> > > Apologies if this was discussed already, but the embedding of arrays inside of > > strings seems a bit hacky, and error prone for the developer. How about > > something a bit more structured. Make the param into: > > > > AcceptOption[]? accepts; > > > > With AcceptOption being: > > > > dictionary AcceptOption { > > // Optional, if not present, a description will be auto-generated > > DOMString? description; > > // One of mimeTypes or extensions must contain at least one element. > > DOMString[]? mimeTypes; > > DOMString[]? extensions; > > } > > We went with this approach to be consistent with the file type inputs accept > attribute; however just yesterday we were talking about being able to set the > description somehow. In the current approach this would be even uglier. The > format propised here by Mihai is much more extendable. I guess because we have an actual API, we can be more explicit :) Great, I'll implement Mihai's proposal today. It doesn't drastically change the internal behaviour, just the interface. Thanks for the thoughts!
On 2012/07/31 00:42:41, thorogood wrote: > On 2012/07/31 00:40:29, benwells wrote: > > On 2012/07/30 23:33:03, Mihai Parparita wrote: > > > > > > https://chromiumcodereview.appspot.com/10692105/diff/28001/chrome/common/exte... > > > File chrome/common/extensions/api/file_system.idl (right): > > > > > > > > > https://chromiumcodereview.appspot.com/10692105/diff/28001/chrome/common/exte... > > > chrome/common/extensions/api/file_system.idl:27: // by type. For example, > > > <code>['image/*', 'text/html,.jso']</code> > > > Apologies if this was discussed already, but the embedding of arrays inside > of > > > strings seems a bit hacky, and error prone for the developer. How about > > > something a bit more structured. Make the param into: > > > > > > AcceptOption[]? accepts; > > > > > > With AcceptOption being: > > > > > > dictionary AcceptOption { > > > // Optional, if not present, a description will be auto-generated > > > DOMString? description; > > > // One of mimeTypes or extensions must contain at least one element. > > > DOMString[]? mimeTypes; > > > DOMString[]? extensions; > > > } > > > > We went with this approach to be consistent with the file type inputs accept > > attribute; however just yesterday we were talking about being able to set the > > description somehow. In the current approach this would be even uglier. The > > format propised here by Mihai is much more extendable. > > I guess because we have an actual API, we can be more explicit :) > > Great, I'll implement Mihai's proposal today. It doesn't drastically change the > internal behaviour, just the interface. Thanks for the thoughts! Mihai/Ben, PTAL. I've updated the code to use the new AcceptOption arg.
LGTM https://chromiumcodereview.appspot.com/10692105/diff/23003/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.h (right): https://chromiumcodereview.appspot.com/10692105/diff/23003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.h:12: namespace file_system = extensions::api::file_system; Namespace aliases aren't allowed in the top level of header files (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...). You can move it inside of the "extension" namespace, though by that point you're not really gaining much, shorthand-wise. Maybe don't use the alias in the header file, and add a "namespace AcceptOption = extensions::api::file_system::AcceptOption" in the .cc file, to make things even shorter. https://chromiumcodereview.appspot.com/10692105/diff/23003/chrome/common/exte... File chrome/common/extensions/docs/apps/fileSystem.html (right): https://chromiumcodereview.appspot.com/10692105/diff/23003/chrome/common/exte... chrome/common/extensions/docs/apps/fileSystem.html:697: <dd>The optional list of accepted mime-types for this file opener, grouped by type. For example, <code>['image/*', 'text/html,.jso']</code> would display two type options - one accepting images - and the other accepting HTML files and those with the extension 'jso'. This field is optional. However, it is an error to not specify this field (or pass an empty array) but pass acceptsAllTypes as false.</dd> Docs need to be regenerated.
https://chromiumcodereview.appspot.com/10692105/diff/27009/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/27009/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:171: if (description_id) { This logic is confusing, and probably not correct. Two problems: - it is order dependent. Having application/* then image/* will leave description at image, but the other way will clear the description - if there is image/a then image/b then image/c it will clear the description when it doesn't have to. https://chromiumcodereview.appspot.com/10692105/diff/23003/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.h (right): https://chromiumcodereview.appspot.com/10692105/diff/23003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.h:12: namespace file_system = extensions::api::file_system; On 2012/07/31 22:26:10, Mihai Parparita wrote: > Namespace aliases aren't allowed in the top level of header files (see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...). > You can move it inside of the "extension" namespace, though by that point you're > not really gaining much, shorthand-wise. Maybe don't use the alias in the header > file, and add a "namespace AcceptOption = > extensions::api::file_system::AcceptOption" in the .cc file, to make things even > shorter. You could also have a typedef in FileSystemChooseFileFunction for the vector of linked ptrs of extension::api::file_system::AcceptOptions.
https://chromiumcodereview.appspot.com/10692105/diff/27009/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://chromiumcodereview.appspot.com/10692105/diff/27009/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.cc:171: if (description_id) { On 2012/07/31 23:01:19, benwells wrote: > This logic is confusing, and probably not correct. Two problems: > - it is order dependent. Having application/* then image/* will leave > description at image, but the other way will clear the description This was a bug. However, I've made it clear the description in both cases, since we don't really know what the user intended. > - if there is image/a then image/b then image/c it will clear the description > when it doesn't have to. This is ugly, since it occurs in two possible cases: 1. image/*, image/jpeg 2. image/jpeg, image/* The first case is easy to catch, since the set doesn't get any bigger on hitting the jpeg. The second case is difficult since it doesn't look different than say, "audio/mp3, image/*". I've changed the code such that if we see more than one of anything, we revert to the default description - I think making the code more complicated here isn't worth the payoff. This isn't amazing, but since the user is now allowed to specify the text description explicitly, I don't think it's as important. https://chromiumcodereview.appspot.com/10692105/diff/23003/chrome/browser/ext... File chrome/browser/extensions/api/file_system/file_system_api.h (right): https://chromiumcodereview.appspot.com/10692105/diff/23003/chrome/browser/ext... chrome/browser/extensions/api/file_system/file_system_api.h:12: namespace file_system = extensions::api::file_system; On 2012/07/31 23:01:19, benwells wrote: > On 2012/07/31 22:26:10, Mihai Parparita wrote: > > Namespace aliases aren't allowed in the top level of header files (see > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...). > > You can move it inside of the "extension" namespace, though by that point > you're > > not really gaining much, shorthand-wise. Maybe don't use the alias in the > header > > file, and add a "namespace AcceptOption = > > extensions::api::file_system::AcceptOption" in the .cc file, to make things > even > > shorter. > > You could also have a typedef in FileSystemChooseFileFunction for the vector of > linked ptrs of extension::api::file_system::AcceptOptions. I've taken the typedef option inside the class definition - it seems neat. https://chromiumcodereview.appspot.com/10692105/diff/23003/chrome/common/exte... File chrome/common/extensions/docs/apps/fileSystem.html (right): https://chromiumcodereview.appspot.com/10692105/diff/23003/chrome/common/exte... chrome/common/extensions/docs/apps/fileSystem.html:697: <dd>The optional list of accepted mime-types for this file opener, grouped by type. For example, <code>['image/*', 'text/html,.jso']</code> would display two type options - one accepting images - and the other accepting HTML files and those with the extension 'jso'. This field is optional. However, it is an error to not specify this field (or pass an empty array) but pass acceptsAllTypes as false.</dd> On 2012/07/31 22:26:10, Mihai Parparita wrote: > Docs need to be regenerated. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thorogood@chromium.org/10692105/14006
Presubmit check for 10692105-14006 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
Presubmit checks took 1.2s to calculate.
+thakis, for top-level chrome/ folder: I've just added a test.
chrome_tests.gypi change lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thorogood@chromium.org/10692105/14006
Try job failure for 10692105-14006 (retry) on win_rel for step "compile" (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/thorogood@chromium.org/10692105/14006
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thorogood@chromium.org/10692105/33002
Try job failure for 10692105-33002 (retry) on win_rel for step "compile" (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/thorogood@chromium.org/10692105/13014
Try job failure for 10692105-13014 (retry) on win_rel for step "compile" (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/thorogood@chromium.org/10692105/14009
Try job failure for 10692105-14009 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thorogood@chromium.org/10692105/13022
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thorogood@chromium.org/10692105/24029
Change committed as 150515 |
