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.
Created attachment 143504 [details] Patch
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.
Created attachment 143556 [details] Patch
(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 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); }
> 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.)
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.
Created attachment 143797 [details] Patch
Comment on attachment 143797 [details] Patch LGTM.
Comment on attachment 143797 [details] Patch Clearing flags on attachment: 143797 Committed r118394: <http://trac.webkit.org/changeset/118394>
All reviewed patches have been landed. Closing bug.