FileSystemWin.cpp needs listDirectory() implementation

[Brian Weinstein]

FileSystemWin.cpp needs listDirectory() implementation.

<rdar://problem/9126635>

[Brian Weinstein]

Created attachment 85716 [details]
[PATCH] Fix

[WebKit Review Bot]

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 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 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]

Created attachment 85724 [details]
[PATCH] Fix v2

[Adam Roben (:aroben)]

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]

Created attachment 85726 [details]
[PATCH] Fix v3

[Gavin Barraclough]

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]

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]

Created attachment 85727 [details]
[PATCH] Fix v4

[Brian Weinstein]

Landed in r81065.