Bug 73798 - Upstream 6 files into WebCore/platform/blackberry
: Upstream 6 files into WebCore/platform/blackberry
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 73144
  Show dependency treegraph
 
Reported: 2011-12-04 22:23 PST by
Modified: 2011-12-12 08:35 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-12-04 22:35:18 PST -------
Created an attachment (id=117844) [details]
Patch
------- Comment #2 From 2011-12-05 00:06:24 PST -------
(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.
------- Comment #3 From 2011-12-05 00:23:09 PST -------
(In reply to comment #2)
> (From update of attachment 117844 [details] [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 From 2011-12-05 00:31:50 PST -------
Created an attachment (id=117852) [details]
Patch
------- Comment #5 From 2011-12-07 14:36:42 PST -------
(From update of attachment 117852 [details])
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 From 2011-12-07 18:57:58 PST -------
Created an attachment (id=118309) [details]
Patch
------- Comment #7 From 2011-12-07 19:12:58 PST -------
(From update of attachment 118309 [details])
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 From 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 From 2011-12-08 02:57:07 PST -------
Created an attachment (id=118354) [details]
Patch
------- Comment #10 From 2011-12-08 11:52:02 PST -------
(From update of attachment 118354 [details])
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 From 2011-12-08 18:31:42 PST -------
yes, thanks for your comments, Rob.
------- Comment #12 From 2011-12-08 18:39:27 PST -------
Created an attachment (id=118511) [details]
Patch
------- Comment #13 From 2011-12-08 19:41:39 PST -------
(From update of attachment 118511 [details])
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 From 2011-12-08 20:05:02 PST -------
Created an attachment (id=118519) [details]
Patch
------- Comment #15 From 2011-12-09 07:45:29 PST -------
(From update of attachment 118519 [details])
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 From 2011-12-10 01:30:13 PST -------
(From update of attachment 118519 [details])
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 From 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 From 2011-12-11 22:43:03 PST -------
Created an attachment (id=118734) [details]
Patch
------- Comment #19 From 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 From 2011-12-12 07:20:37 PST -------
(From update of attachment 118734 [details])
Looks good.
------- Comment #21 From 2011-12-12 08:35:23 PST -------
(From update of attachment 118734 [details])
Clearing flags on attachment: 118734

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