[Webkit-unassigned] [Bug 31154] Simplify Icon interface
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 5 10:50:29 PST 2009
https://bugs.webkit.org/show_bug.cgi?id=31154
Darin Adler <darin at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #42547|review? |review-
Flag| |
--- Comment #2 from Darin Adler <darin at apple.com> 2009-11-05 10:50:29 PDT ---
(From update of attachment 42547)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list