Bug 56331 - FileSystemWin.cpp needs listDirectory() implementation
Summary: FileSystemWin.cpp needs listDirectory() implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Brian Weinstein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-14 14:03 PDT by Brian Weinstein
Modified: 2018-11-27 17:24 PST (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2011-03-14 14:03:43 PDT
FileSystemWin.cpp needs listDirectory() implementation.

<rdar://problem/9126635>
Comment 1 Brian Weinstein 2011-03-14 14:08:37 PDT
Created attachment 85716 [details]
[PATCH] Fix
Comment 2 WebKit Review Bot 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.
Comment 3 Adam Roben (:aroben) 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.
Comment 4 Brian Weinstein 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.
Comment 5 Brian Weinstein 2011-03-14 14:45:19 PDT
Created attachment 85724 [details]
[PATCH] Fix v2
Comment 6 Adam Roben (:aroben) 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?
Comment 7 Brian Weinstein 2011-03-14 15:02:20 PDT
Created attachment 85726 [details]
[PATCH] Fix v3
Comment 8 Gavin Barraclough 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.
Comment 9 WebKit Review Bot 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.
Comment 10 Brian Weinstein 2011-03-14 15:10:49 PDT
Created attachment 85727 [details]
[PATCH] Fix v4
Comment 11 Brian Weinstein 2011-03-14 15:16:21 PDT
Landed in r81065.