In WebCore/platform/graphics/Icon.h The roles of Icon::createIconForFile() and createIconForFiles() are not well-defined. For example, it's not sure whether a caller can use createIconForFiles() for a single file or not. So I'd like to propose removing createIconForFile() and using createIconForFiles() for any cases.
Created attachment 42547 [details] Proposed patch
Comment on attachment 42547 [details] Proposed patch It's best not to combine refactoring with a bug fix in a single patch. A first patch that refactors and then a second that fixes the bug would be a superior approach. Or vice versa. > +#ifndef BUILDING_ON_TIGER > + if (filenames.size() == 1) > +#endif // FIXME: find a better image for multiple files to use on Tiger. > + { > + // Don't pass relative filenames -- we don't want a result that depends on the current directory. > + // Need 0U here to disambiguate String::operator[] from operator(NSString*, int)[] > + if (filenames[0].isEmpty() || filenames[0][0U] != '/') > + return 0; > + > + NSImage* image = [[NSWorkspace sharedWorkspace] iconForFile:filenames[0]]; > + if (!image) > + return 0; > + > + return adoptRef(new Icon(image)); > + } It's better to not have a conditional if like this -- too hard to read. Instead, a boolean could be used to keep it simpler although a few more lines of code. bool useIconFromFirstFile; #ifdef BUILDING_ON_TIGER useIconFromFirstFile = true; #else useIconFromFirstFile = filenames.size() == 1; #endif if (useIconFromFirstFile) { // body here } > + FileList* list = input->files(); > + Vector<String> filenames; > + for (unsigned i = 0; list && i < list->length(); ++i) > + filenames.append(list->item(i)->path()); > + m_fileChooser = FileChooser::create(this, filenames); It's inelegant to check list for 0 every time through the loop and also to call the length function every time. Both can be solved by having a local variable. unsigned length = list ? list->length() : 0; I'm going to say review- so you can make the style changes I suggest and also consider whether you can separate the refactoring from the behavior change. Perhaps I was mistaken and there is no behavior change. In that case, it would be best to land the test first separately, then land the code change.
Created attachment 42621 [details] Proposed patch (rev.2) Darin, thank you for the comments. I removed the bug fix part, and applied your comments.
Comment on attachment 42621 [details] Proposed patch (rev.2) Clearing flags on attachment: 42621 Committed r50632: <http://trac.webkit.org/changeset/50632>
All reviewed patches have been landed. Closing bug.