[webkit-reviews] review denied: [Bug 31154] Simplify Icon interface : [Attachment 42547] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 31154: Simplify Icon interface
https://bugs.webkit.org/show_bug.cgi?id=31154

Attachment 42547: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=42547&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.