Bug 73798 – Upstream 6 files into WebCore/platform/blackberry

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Mary Wu]

Upstream 6 files into WebCore/platform/blackberry:
FileSystemBlackBerry.cpp          PlatformTouchPointBlackBerry.cpp  SharedTimerBlackBerry.cpp
PlatformTouchEventBlackBerry.cpp  SharedBufferBlackBerry.cpp        SystemTimeBlackBerry.cpp

[Mary Wu]

Created attachment 117844 [details]
Patch

[Daniel Bates]

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.

[Mary Wu]

(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...

[Mary Wu]

Created attachment 117852 [details]
Patch

[Rob Buis]

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.

[Mary Wu]

Created attachment 118309 [details]
Patch

[Rob Buis]

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?

[Mary Wu]

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

[Mary Wu]

Created attachment 118354 [details]
Patch

[Rob Buis]

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.

[Mary Wu]

yes, thanks for your comments, Rob.

[Mary Wu]

Created attachment 118511 [details]
Patch

[Rob Buis]

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())

[Mary Wu]

Created attachment 118519 [details]
Patch

[Rob Buis]

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.

[Nikolas Zimmermann]

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.

[Mary Wu]

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

[Mary Wu]

Created attachment 118734 [details]
Patch

[Mary Wu]

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

[Rob Buis]

Comment on attachment 118734 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 118734 [details]
Patch

Clearing flags on attachment: 118734

Committed r102588: <http://trac.webkit.org/changeset/102588>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.