[webkit-reviews] review denied: [Bug 73798] Upstream 6 files into WebCore/platform/blackberry : [Attachment 118519] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied  review:
Bug 73798: Upstream 6 files into WebCore/platform/blackberry
https://bugs.webkit.org/show_bug.cgi?id=73798

Attachment 118519: Patch
https://bugs.webkit.org/attachment.cgi?id=118519&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=118519&action=review


Oh, just saw Rob gave r+, I have a rather big concern, so I revert to r-:

> Source/WebCore/platform/blackberry/FileSystemBlackBerry.cpp:44
> +Vector<String> listDirectory(const String& path, const String& filter)
> +{
> +    Vector<String> entries;

Ouch, we now have three different versions of listDirectory, that all more ore
less use the same opendir() based functionality.
Once in platform/efl/FileSystemEfl.cpp (our code seems to be a copy of it), and
platform/posix/FileSystemPosix.cpp.

Could you break this patch up into another piece, and refactor the
listDirectory method? We can 1:1 share with Efl I think, and maybe also posix/.

No need for us to have the maintenance burden alone :-)

And note: this code in general looks scary, and I'd rather avoid having
possibly unsafe code in trunk.
If we want to add this as new code, it needs an intensive review...

The rest of this patch looks fine, so I suggest to leave out
FileSysytemBlackberry from this patch, until it is properly refactored.

> Source/WebCore/platform/blackberry/SharedBufferBlackBerry.cpp:32
> +    return 0;

return nullptr;

> Source/WebCore/platform/blackberry/SharedTimerBlackBerry.cpp:61
> +	   timer = new SharedTimerBlackBerry();

s/()//

> Source/WebCore/platform/blackberry/SharedTimerBlackBerry.cpp:73
> +    if (m_started) {

if (!m_started) early return.