Bug 73798

Created attachment 117844 [details] Patch

Comment on attachment 117844 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117844&action=review > Source/WebCore/platform/blackberry/FileSystemBlackBerry.cpp:25 > +#include "Vector.h" This should be: #include <wtf/Vector.h> > Source/WebCore/platform/blackberry/FileSystemBlackBerry.cpp:102 > + return ""; Nit: return String() (call the default constructor instead of C-string variant. > Source/WebCore/platform/blackberry/PlatformTouchEventBlackBerry.cpp:2 > + * This file is part of the WebKit project. This remark is obvious given you're proposing to land this file in the WebKit.org tree. > Source/WebCore/platform/blackberry/PlatformTouchEventBlackBerry.cpp:60 > + for (unsigned int i = 0; i < event->m_points.size(); ++i) unsigned int => unsigned I take it the compiler is smart enough to either cache event->m_points or event->m_points.size(). Otherwise, I suggest caching this value in a local variable. > Source/WebCore/platform/blackberry/SharedBufferBlackBerry.cpp:24 > +#include "PassRefPtr.h" This should be: #include <wtf/PassRefPtr.h> > Source/WebCore/platform/blackberry/SharedBufferBlackBerry.cpp:28 > +WTF::PassRefPtr<SharedBuffer> SharedBuffer::createWithContentsOfFile(String const&) Do we need the WTF:: prefix here? > Source/WebCore/platform/blackberry/SystemTimeBlackBerry.cpp:30 > + return 0.0f; This should be: return 0; Per the style rules and by implicit conversion of the return value.

(In reply to comment #2) > (From update of attachment 117844 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117844&action=review > > > Source/WebCore/platform/blackberry/FileSystemBlackBerry.cpp:25 > > +#include "Vector.h" > > This should be: > > #include <wtf/Vector.h> > > > Source/WebCore/platform/blackberry/FileSystemBlackBerry.cpp:102 > > + return ""; > > Nit: return String() > > (call the default constructor instead of C-string variant. > > > Source/WebCore/platform/blackberry/PlatformTouchEventBlackBerry.cpp:2 > > + * This file is part of the WebKit project. > > This remark is obvious given you're proposing to land this file in the WebKit.org tree. > > > Source/WebCore/platform/blackberry/PlatformTouchEventBlackBerry.cpp:60 > > + for (unsigned int i = 0; i < event->m_points.size(); ++i) > > unsigned int => unsigned > > I take it the compiler is smart enough to either cache event->m_points or event->m_points.size(). Otherwise, I suggest caching this value in a local variable. > > > Source/WebCore/platform/blackberry/SharedBufferBlackBerry.cpp:24 > > +#include "PassRefPtr.h" > > This should be: > > #include <wtf/PassRefPtr.h> > > > Source/WebCore/platform/blackberry/SharedBufferBlackBerry.cpp:28 > > +WTF::PassRefPtr<SharedBuffer> SharedBuffer::createWithContentsOfFile(String const&) > > Do we need the WTF:: prefix here? > > > Source/WebCore/platform/blackberry/SystemTimeBlackBerry.cpp:30 > > + return 0.0f; > > This should be: > > return 0; > > Per the style rules and by implicit conversion of the return value. thanks for your comments, Daniel, will update...

Created attachment 117852 [details] Patch

Comment on attachment 117852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117852&action=review Looks good, but some stuff that can be improved, so r- for now. > Source/WebCore/platform/blackberry/PlatformTouchEventBlackBerry.cpp:38 > +#if PLATFORM(BLACKBERRY) && OS(QNX) I talked with Dan and this is not needed. OS(QNX) is not needed at all. PLATFORM(BLACKBERRY) is implied since we only compile this on for BlackBerry. So you can remove the test. > Source/WebCore/platform/blackberry/SharedTimerBlackBerry.cpp:32 > + static SharedTimerBlackBerry* inst(); I think instance is a better name.

Created attachment 118309 [details] Patch

Comment on attachment 118309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118309&action=review Looking good, but still some issues left. > Source/WebCore/platform/blackberry/FileSystemBlackBerry.cpp:54 > + // loop invariant: directory part + '/' Should be made a sentence. > Source/WebCore/platform/blackberry/PlatformTouchEventBlackBerry.cpp:38 > +#if PLATFORM(BLACKBERRY) This one should also not be needed, this is a BlackBerry file. > Source/WebCore/platform/blackberry/SharedTimerBlackBerry.cpp:29 > + friend void setSharedTimerFiredFunction(void (*f)()); Is this friend declaration needed? > Source/WebCore/platform/blackberry/SharedTimerBlackBerry.cpp:37 > + bool m_started; Could this be moved to private part?

I think we referenced SharedTimerBlackBerry.cpp with qt porting, so should move both m_started and (*m_timerFunction) to private part.

Created attachment 118354 [details] Patch

Comment on attachment 118354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118354&action=review I am pretty sure these are the last remaining problems, but for now r-. > Source/WebCore/platform/blackberry/FileSystemBlackBerry.cpp:50 > + DIR* dir; You could move this to when they are actually used. For instance dir is only used much later. This is inconsistent with how de variable is used, which is only declared just before it is used. > Source/WebCore/platform/blackberry/FileSystemBlackBerry.cpp:54 > + // if directory part don't have '/' in the end, add it Should be made into a proper sentence starting with Capital and ending with punctuation mark. Also probably should be placed before the if. > Source/WebCore/platform/blackberry/SharedTimerBlackBerry.cpp:45 > +static SharedTimerBlackBerry* s_timer = 0; I think it is nicer to move this into the instance() method. Also then you can remove the s_ prefix.

yes, thanks for your comments, Rob.

Created attachment 118511 [details] Patch

Comment on attachment 118511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118511&action=review Some nits left. Once you have fixed, I am fine with the patch, so you can ask Leo to do the final r+ and cq. > Source/WebCore/platform/blackberry/FileSystemBlackBerry.cpp:55 > + fileName = filePath + cpath.length(); How about: char *fileName = filePath + cpath.length(); > Source/WebCore/platform/blackberry/FileSystemBlackBerry.cpp:61 > + fileNameSpace = sizeof(filePath) - (fileName - filePath) - 1; Why not move it down to: size_t fileNameSpace = sizeof(filePath) - (fileName - filePath) - 1; > Source/WebCore/platform/blackberry/FileSystemBlackBerry.cpp:64 > + dir = opendir(cpath.data()); I would suggest : DIR* dir = opendir(path.data())

Created attachment 118519 [details] Patch

Comment on attachment 118519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118519&action=review The code is fine, one nit left. So r+ but cq- :) This means the patch can be fixed and committed by hand, or reuploaded and I'll r+ it again. > Source/WebCore/platform/blackberry/PlatformTouchPointBlackBerry.cpp:3 > + * Copyright (C) 2010, 2011 Research In Motion Limited 2010. All rights reserved. The final 2010 seems misplaced.

Comment on attachment 118519 [details] Patch 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.

Nikolas, thanks for your comments. For disagreement on the file FileSystemBlackBerry.cpp, I removed it from the patch and give it to separate pr.

Created attachment 118734 [details] Patch

> > Source/WebCore/platform/blackberry/SharedBufferBlackBerry.cpp:32 > > + return 0; > > return nullptr; > In WebKit coding-style, I saw this "In C++, the null pointer value should be written as 0." So should we use nullptr?

Comment on attachment 118734 [details] Patch Looks good.

Comment on attachment 118734 [details] Patch Clearing flags on attachment: 118734 Committed r102588: <http://trac.webkit.org/changeset/102588>

All reviewed patches have been landed. Closing bug.