Bug 31154 - Simplify Icon interface
Summary: Simplify Icon interface
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 31195
  Show dependency treegraph
Reported: 2009-11-04 21:53 PST by Kent Tamura
Modified: 2009-11-08 17:28 PST (History)
2 users (show)

See Also:

Proposed patch (33.80 KB, patch)
2009-11-04 22:09 PST, Kent Tamura
darin: review-
Details | Formatted Diff | Diff
Proposed patch (rev.2) (16.07 KB, patch)
2009-11-05 19:40 PST, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2009-11-04 21:53:53 PST
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.
Comment 1 Kent Tamura 2009-11-04 22:09:06 PST
Created attachment 42547 [details]
Proposed patch
Comment 2 Darin Adler 2009-11-05 10:50:29 PST
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.

> +    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;
    useIconFromFirstFile = true;
    useIconFromFirstFile = filenames.size() == 1;

    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.
Comment 3 Kent Tamura 2009-11-05 19:40:45 PST
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 4 WebKit Commit Bot 2009-11-08 17:28:27 PST
Comment on attachment 42621 [details]
Proposed patch (rev.2)

Clearing flags on attachment: 42621

Committed r50632: <http://trac.webkit.org/changeset/50632>
Comment 5 WebKit Commit Bot 2009-11-08 17:28:31 PST
All reviewed patches have been landed.  Closing bug.