RESOLVED FIXED 56331
FileSystemWin.cpp needs listDirectory() implementation
https://bugs.webkit.org/show_bug.cgi?id=56331
Summary FileSystemWin.cpp needs listDirectory() implementation
Brian Weinstein
Reported 2011-03-14 14:03:43 PDT
FileSystemWin.cpp needs listDirectory() implementation. <rdar://problem/9126635>
Attachments
[PATCH] Fix (9.63 KB, patch)
2011-03-14 14:08 PDT, Brian Weinstein
aroben: review+
[PATCH] Fix v2 (11.23 KB, patch)
2011-03-14 14:45 PDT, Brian Weinstein
aroben: review-
[PATCH] Fix v3 (11.21 KB, patch)
2011-03-14 15:02 PDT, Brian Weinstein
barraclough: review-
[PATCH] Fix v4 (11.23 KB, patch)
2011-03-14 15:10 PDT, Brian Weinstein
barraclough: review+
Brian Weinstein
Comment 1 2011-03-14 14:08:37 PDT
Created attachment 85716 [details] [PATCH] Fix
WebKit Review Bot
Comment 2 2011-03-14 14:10:35 PDT
Attachment 85716 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit2/UIProcess/Plugins/win/PluginInfoStoreWin.cpp:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Adam Roben (:aroben)
Comment 3 2011-03-14 14:11:46 PDT
Comment on attachment 85716 [details] [PATCH] Fix View in context: https://bugs.webkit.org/attachment.cgi?id=85716&action=review > Source/WebCore/platform/win/FileSystemWin.cpp:307 > + String windowsSpecificFilter = "\\" + filter; > + PathWalker walker(directory, windowsSpecificFilter); Please just pass filter through here. > Source/WebCore/platform/win/FileSystemWin.cpp:316 > + String fileName = walker.data().cFileName; > + entries.append(makeString(directory, "\\", fileName)); This will (I think) be more efficient if you remove the local fileName variable. > Source/WebCore/platform/win/PathWalker.cpp:2 > + * Copyright (C) 2010 Apple Inc. All rights reserved. 2011! > Source/WebCore/platform/win/PathWalker.cpp:54 > +bool PathWalker::isValid() const > +{ > + return m_handle != INVALID_HANDLE_VALUE; > +} > + > +const WIN32_FIND_DATAW& PathWalker::data() const > +{ > + return m_data; > +} I think these could go in the header.
Brian Weinstein
Comment 4 2011-03-14 14:32:46 PDT
Comment on attachment 85716 [details] [PATCH] Fix View in context: https://bugs.webkit.org/attachment.cgi?id=85716&action=review >> Source/WebCore/platform/win/FileSystemWin.cpp:307 >> + PathWalker walker(directory, windowsSpecificFilter); > > Please just pass filter through here. Fixed. >> Source/WebCore/platform/win/FileSystemWin.cpp:316 >> + entries.append(makeString(directory, "\\", fileName)); > > This will (I think) be more efficient if you remove the local fileName variable. Fixed. Added some code to StringConcatenate to handle UChar* - I will send out a new patch shortly. >> Source/WebCore/platform/win/PathWalker.cpp:2 >> + * Copyright (C) 2010 Apple Inc. All rights reserved. > > 2011! Fixed! >> Source/WebCore/platform/win/PathWalker.cpp:54 >> +} > > I think these could go in the header. Moved.
Brian Weinstein
Comment 5 2011-03-14 14:45:19 PDT
Created attachment 85724 [details] [PATCH] Fix v2
Adam Roben (:aroben)
Comment 6 2011-03-14 14:48:20 PDT
Comment on attachment 85724 [details] [PATCH] Fix v2 View in context: https://bugs.webkit.org/attachment.cgi?id=85724&action=review I think the StringConcatenate changes need some work. > Source/JavaScriptCore/wtf/text/StringConcatenate.h:102 > + if (m_length > std::numeric_limits<unsigned>::max()) > + CRASH(); I don't think this will ever be true. > Source/JavaScriptCore/wtf/text/StringConcatenate.h:112 > + for (unsigned i = 0; i < m_length; ++i) { > + unsigned char c = m_buffer[i]; > + destination[i] = c; > + } You're stripping off the high 8 bits of each character. That doesn't seem right. I think memcpy would be better. > Source/WebCore/platform/win/PathWalker.h:27 > +#include <wtf/text/WTFString.h> Why is this neeed? Would a forward declaration suffice?
Brian Weinstein
Comment 7 2011-03-14 15:02:20 PDT
Created attachment 85726 [details] [PATCH] Fix v3
Gavin Barraclough
Comment 8 2011-03-14 15:04:16 PDT
Comment on attachment 85726 [details] [PATCH] Fix v3 View in context: https://bugs.webkit.org/attachment.cgi?id=85726&action=review > Source/JavaScriptCore/wtf/text/StringConcatenate.h:111 > + memcpy(destination, m_buffer, m_length * sizeof(UChar)); please cast m_length to size_t before multiplying, this might overflow.
WebKit Review Bot
Comment 9 2011-03-14 15:04:57 PDT
Attachment 85726 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/WebCore/platform/win/PathWalker.h:30: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Brian Weinstein
Comment 10 2011-03-14 15:10:49 PDT
Created attachment 85727 [details] [PATCH] Fix v4
Brian Weinstein
Comment 11 2011-03-14 15:16:21 PDT
Landed in r81065.
Note You need to log in before you can comment on or make changes to this bug.