Bug 87234 - Cleanup: introduce toFile() to reduce static cast from Blob to File
Summary: Cleanup: introduce toFile() to reduce static cast from Blob to File
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kinuko Yasuda
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-23 01:56 PDT by Kinuko Yasuda
Modified: 2012-05-24 11:07 PDT (History)
5 users (show)

See Also:


Attachments
Patch (14.98 KB, patch)
2012-05-23 01:59 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (14.92 KB, patch)
2012-05-23 06:26 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (12.16 KB, patch)
2012-05-24 05:37 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2012-05-23 01:56:31 PDT
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.
Comment 1 Kinuko Yasuda 2012-05-23 01:59:06 PDT
Created attachment 143504 [details]
Patch
Comment 2 Caio Marcelo de Oliveira Filho 2012-05-23 05:26:34 PDT
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.
Comment 3 Kinuko Yasuda 2012-05-23 06:26:51 PDT
Created attachment 143556 [details]
Patch
Comment 4 Kinuko Yasuda 2012-05-23 06:28:17 PDT
(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.
Comment 5 Alexey Proskuryakov 2012-05-23 15:06:27 PDT
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);
}
Comment 6 Adam Barth 2012-05-23 16:04:30 PDT
> 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.)
Comment 7 Kinuko Yasuda 2012-05-24 05:36:59 PDT
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.
Comment 8 Kinuko Yasuda 2012-05-24 05:37:23 PDT
Created attachment 143797 [details]
Patch
Comment 9 Eric Seidel (no email) 2012-05-24 09:28:11 PDT
Comment on attachment 143797 [details]
Patch

LGTM.
Comment 10 WebKit Review Bot 2012-05-24 11:07:38 PDT
Comment on attachment 143797 [details]
Patch

Clearing flags on attachment: 143797

Committed r118394: <http://trac.webkit.org/changeset/118394>
Comment 11 WebKit Review Bot 2012-05-24 11:07:43 PDT
All reviewed patches have been landed.  Closing bug.