WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
[PATCH] Fix v2
(11.23 KB, patch)
2011-03-14 14:45 PDT
,
Brian Weinstein
aroben
: review-
Details
Formatted Diff
Diff
[PATCH] Fix v3
(11.21 KB, patch)
2011-03-14 15:02 PDT
,
Brian Weinstein
barraclough
: review-
Details
Formatted Diff
Diff
[PATCH] Fix v4
(11.23 KB, patch)
2011-03-14 15:10 PDT
,
Brian Weinstein
barraclough
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug