Bug 87234 – Cleanup: introduce toFile() to reduce static cast from Blob to File

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kinuko Yasuda]

Cleanup: introduce Blob::asFile() to reduce static casting from Blob to File for better readability.
This must be a purely mechanical change which has no side effect.

[Kinuko Yasuda]

Created attachment 143504 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 143504 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143504&action=review

I'm not a reviewer, but this patch looks good to me. I have a small suggestion.

> Source/WebKit/chromium/src/WebDragData.cpp:89
> +                if (blob->asFile()) {
> +                    const File* file = blob->asFile();

Minor suggestion: here you could use "if (const File* file = blob->asFile())" instead of writing blob->asFile() twice.

There are other places in the patch where you declare/initialize outside the if-statement but it could be done in the condition too.

[Kinuko Yasuda]

Created attachment 143556 [details]
Patch

[Kinuko Yasuda]

(In reply to comment #2)
> (From update of attachment 143504 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143504&action=review
> 
> I'm not a reviewer, but this patch looks good to me. I have a small suggestion.
> 
> > Source/WebKit/chromium/src/WebDragData.cpp:89
> > +                if (blob->asFile()) {
> > +                    const File* file = blob->asFile();
> 
> Minor suggestion: here you could use "if (const File* file = blob->asFile())" instead of writing blob->asFile() twice.
> 
> There are other places in the patch where you declare/initialize outside the if-statement but it could be done in the condition too.

Looks neat, I converted some conditions to look like that. Thanks.

[Alexey Proskuryakov]

Comment on attachment 143556 [details]
Patch

This is not quite the pattern used elsewhere in WebCore. Compare to:

inline Element* toElement(Node* node)
{
    ASSERT(!node || node->isElementNode());
    return static_cast<Element*>(node);
}

[Adam Barth]

> This is not quite the pattern used elsewhere in WebCore. Compare to:

We use both patterns.  It's a trade-off between performance and avoiding bugs.  (I'm not familiar enough with where all this function is/will be used to comment on which approach is better here.)

[Kinuko Yasuda]

Having the second look at the change, I felt that keeping an explicit isFile() boolean method and having a separate toFile() might be better, we need two more tiny methods than asFile() approach.  I'm happy if I can get rid of static-cast's to make it easier to modify this code, so I'd simply take toFile() approach.

[Kinuko Yasuda]

Created attachment 143797 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 143797 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 143797 [details]
Patch

Clearing flags on attachment: 143797

Committed r118394: <http://trac.webkit.org/changeset/118394>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.