Bug 73798 - Upstream 6 files into WebCore/platform/blackberry
: Upstream 6 files into WebCore/platform/blackberry
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Platform
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 73144
  Show dependency treegraph
 
Reported: 2011-12-04 22:23 PST by Mary Wu
Modified: 2011-12-12 08:35 PST (History)
3 users (show)

See Also:


Attachments
Patch (16.01 KB, patch)
2011-12-04 22:35 PST, Mary Wu
no flags Details | Formatted Diff | Diff
Patch (15.84 KB, patch)
2011-12-05 00:31 PST, Mary Wu
no flags Details | Formatted Diff | Diff
Patch (15.83 KB, patch)
2011-12-07 18:57 PST, Mary Wu
no flags Details | Formatted Diff | Diff
Patch (15.80 KB, patch)
2011-12-08 02:57 PST, Mary Wu
no flags Details | Formatted Diff | Diff
Patch (15.80 KB, patch)
2011-12-08 18:39 PST, Mary Wu
no flags Details | Formatted Diff | Diff
Patch (15.80 KB, patch)
2011-12-08 20:05 PST, Mary Wu
no flags Details | Formatted Diff | Diff
Patch (12.30 KB, patch)
2011-12-11 22:43 PST, Mary Wu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mary Wu 2011-12-04 22:23:42 PST
Upstream 6 files into WebCore/platform/blackberry:
FileSystemBlackBerry.cpp          PlatformTouchPointBlackBerry.cpp  SharedTimerBlackBerry.cpp
PlatformTouchEventBlackBerry.cpp  SharedBufferBlackBerry.cpp        SystemTimeBlackBerry.cpp
Comment 1 Mary Wu 2011-12-04 22:35:18 PST
Created attachment 117844 [details]
Patch
Comment 2 Daniel Bates 2011-12-05 00:06:24 PST
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.
Comment 3 Mary Wu 2011-12-05 00:23:09 PST
(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...
Comment 4 Mary Wu 2011-12-05 00:31:50 PST
Created attachment 117852 [details]
Patch
Comment 5 Rob Buis 2011-12-07 14:36:42 PST
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.
Comment 6 Mary Wu 2011-12-07 18:57:58 PST
Created attachment 118309 [details]
Patch
Comment 7 Rob Buis 2011-12-07 19:12:58 PST
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?
Comment 8 Mary Wu 2011-12-08 02:46:46 PST
I think we referenced SharedTimerBlackBerry.cpp with qt porting, so should move both m_started and (*m_timerFunction) to private part.
Comment 9 Mary Wu 2011-12-08 02:57:07 PST
Created attachment 118354 [details]
Patch
Comment 10 Rob Buis 2011-12-08 11:52:02 PST
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.
Comment 11 Mary Wu 2011-12-08 18:31:42 PST
yes, thanks for your comments, Rob.
Comment 12 Mary Wu 2011-12-08 18:39:27 PST
Created attachment 118511 [details]
Patch
Comment 13 Rob Buis 2011-12-08 19:41:39 PST
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())
Comment 14 Mary Wu 2011-12-08 20:05:02 PST
Created attachment 118519 [details]
Patch
Comment 15 Rob Buis 2011-12-09 07:45:29 PST
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 16 Nikolas Zimmermann 2011-12-10 01:30:13 PST
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.
Comment 17 Mary Wu 2011-12-11 22:42:19 PST
Nikolas, thanks for your comments. For disagreement on the file FileSystemBlackBerry.cpp, I removed it from the patch and give it to separate pr.
Comment 18 Mary Wu 2011-12-11 22:43:03 PST
Created attachment 118734 [details]
Patch
Comment 19 Mary Wu 2011-12-11 23:41:26 PST
> > 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 20 Rob Buis 2011-12-12 07:20:37 PST
Comment on attachment 118734 [details]
Patch

Looks good.
Comment 21 WebKit Review Bot 2011-12-12 08:35:23 PST
Comment on attachment 118734 [details]
Patch

Clearing flags on attachment: 118734

Committed r102588: <http://trac.webkit.org/changeset/102588>
Comment 22 WebKit Review Bot 2011-12-12 08:35:28 PST
All reviewed patches have been landed.  Closing bug.