[webkit-reviews] review denied: [Bug 118331] Adding Nix files in Source/Platform to trunk : [Attachment 211220] Patch

Fri Sep 27 14:30:44 PDT 2013

Benjamin Poulain <benjamin at webkit.org> has denied Hugo Parente Lima
<hugo.lima at openbossa.org>'s request for review:
Bug 118331: Adding Nix files in Source/Platform to trunk
https://bugs.webkit.org/show_bug.cgi?id=118331

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=211220&action=review

> Source/Platform/nix/public/AudioBus.h:2
> + * Copyright (C) 2010, Google Inc. All rights reserved.

Is this correct?

> Source/Platform/nix/public/AudioBus.h:57
> +    // initialize() allocates memory of the given length for the given
number of channels.
> +    NIX_EXPORT void initialize(unsigned numberOfChannels, size_t length,
double sampleRate);
> +
> +    // resizeSmaller() can only be called after initialize() with a new
length <= the initialization length.
> +    // The data stored in the bus will remain undisturbed.
> +    NIX_EXPORT void resizeSmaller(size_t newLength);
> +
> +    // reset() releases the memory allocated from initialize().
> +    NIX_EXPORT void reset();

The names are not great. The fast that you need comments indicates that.
    resizeSmaller -> shrink

Maybe you should design your API in a way that does not impose calling certain
methods in a certain order.

> Source/Platform/nix/public/AudioBus.h:74
> +    AudioBusPrivate* m_d;

This is an awful name.

> Source/Platform/nix/public/AudioDevice.h:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

Correct?

> Source/Platform/nix/public/AudioDevice.h:44
> +	   virtual void render(const std::vector<float*>& /*sourceData*/, const
std::vector<float*>& /*destinationData*/, size_t /*numberOfFrames*/) { }
> +
> +    protected:
> +	   virtual ~RenderCallback() { }

Why not make those pure virtual?

> Source/Platform/nix/public/Color.h:43
> +    RGBA32 rgba;

probably best encapsulate this? If only for debugging?

You could also make Color immutable, which would make design easier.

> Source/Platform/nix/public/Color.h:56
> +    Color(int r, int g, int b, int a = 255)
> +	   : rgba(std::max(0, std::min(a, 255)) << 24 | std::max(0, std::min(r,
255)) << 16 | std::max(0, std::min(g, 255)) << 8 | std::max(0, std::min(b,
255)))

int -> unsigned char?

"std::max(0, std::min(a, 255)" should be in a utility function.

> Source/Platform/nix/public/Data.h:2
> + * Copyright (C) 2009 Google Inc. All rights reserved.

Uh?

> Source/Platform/nix/public/Data.h:45
> +// WARNING: It is not safe to pass a Nix::Data across threads!!!

Typically, ADT are defined the other way around. Nothing is thread safe by
default, and you document when something is safe.

> Source/Platform/nix/public/Data.h:63
> +    template <int N>
> +    Data(const char (&data)[N]) : m_private(0)
> +    {
> +	   assign(data, N - 1);
> +    }
> +

This is odd. It means you expect data buffers to be zero terminated.
The type is also char and not unsigned char.

Is Data a bastardized StringImpl or a raw buffer?

> Source/Platform/nix/public/Data.h:72
> +    NIX_EXPORT void reset();

reset() on a buffer? You mean clear()?

> Source/Platform/nix/public/Data.h:74
> +    NIX_EXPORT void assign(const Data&);
> +    NIX_EXPORT void assign(const char* data, size_t);

Those two are odd. How do they differ from C++'s operator=?

> Source/Platform/nix/public/FFTFrame.h:48
> +    virtual void doFFT(const float*) { }
> +    virtual void doInverseFFT(float*) { }
> +    virtual void multiply(const FFTFrame&) { }
> +
> +    virtual unsigned frequencyDomainSampleCount() const { return 0; }
> +    // After multiplication and transform operations, the data is scaled
> +    // to take in account the scale used internally in WebKit, originally
> +    // from Mac's vecLib.
> +    // After multiplication: Planar data is scaled by 0.5.
> +    // After direct transform: Planar data is scaled by 2.0.
> +    // After inverse transform: Time domain data is scaled by 1.0/(2* FFT
size).
> +    virtual float* realData() const { return 0; }
> +    virtual float* imagData() const { return 0; }

Wouldn't it make more sense to have those pure virtual?

> Source/Platform/nix/public/Gamepad.h:1
> +// Copyright (C) 2011, Google Inc. All rights reserved.

Uh?

The whole copyright is also not following the usual style.

> Source/Platform/nix/public/Gamepad.h:54
> +    unsigned long long timestamp;

long sucks :)

uint64_t or double? :)

> Source/Platform/nix/public/Gamepads.h:1
> +// Copyright (C) 2011, Google Inc. All rights reserved.

Style + your own copyright.

> Source/Platform/nix/public/Gamepads.h:34
> +	   : length(0) { }

mixed style.

> Source/Platform/nix/public/Platform.h:2
> + * Copyright (C) 2012 Google Inc. All rights reserved.

uh

> Source/Platform/nix/public/Platform.h:65
> +    // FFTFrame

no "-------------------------------------------------------------"?

> Source/Platform/nix/public/Rect.h:2
> +#ifndef Nix_Rect_h
> +#define Nix_Rect_h

No copyright?

> Source/Platform/nix/public/Size.h:2
> + * Copyright (C) 2009 Google Inc. All rights reserved.

Uh

> Source/Platform/nix/public/ThemeEngine.h:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

Uh?

> Source/Platform/nix/src/DefaultWebThemeEngine.cpp:43
> +const double BGColor1 = 0xF6 / 256.0;
> +const double BGColor2 = 0xDE / 256.0;
> +const double BorderColor = 0xA4 / 256.0;
> +const double BorderOnHoverColor = 0x7A / 256.0;
> +const double CheckColor = 0x66 / 256.0;
> +const double TextFieldDarkBorderColor = 0x9A / 256.0;
> +const double TextFieldLightBorderColor = 0xEE / 256.0;

Dividing colors by 256??? Not 255?

> Source/Platform/nix/src/DefaultWebThemeEngine.h:1
> +#ifndef DefaultWebThemeEngine_h

No copyright for this file?

> Source/Platform/nix/src/DefaultWebThemeEngine.h:8
> +class DefaultWebThemeEngine : public ThemeEngine {

Missing override everywhere.
Shouldn't this class be final?

> Source/Platform/nix/src/Platform.cpp:48
> +static WTF::OwnPtr<EmptyPlatform> s_emptyPlatform = WTF::adoptPtr(new
EmptyPlatform());
> +
> +static Platform* s_platform = s_emptyPlatform.get();

Not NeverDestroyed?

> Source/WebCore/platform/audio/nix/AudioBusNix.cpp:2
> + * Copyright (C) 2010, Google Inc. All rights reserved.

Uh?

> Source/WebCore/platform/audio/nix/AudioBusNix.cpp:56
> +    // FIXME: the sampleRate parameter is ignored. It should be removed from
the API.

What about doing that now?

> Source/WebCore/platform/audio/nix/AudioBusNix.cpp:71
> +    // FIXME: the sampleRate parameter is ignored. It should be removed from
the API.

ditto.

> Source/WebCore/platform/audio/nix/AudioDestinationNix.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

Uh?

> Source/WebCore/platform/audio/nix/AudioDestinationNix.h:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

uh?

> Source/WebCore/platform/audio/nix/AudioDestinationNix.h:44
> +// An AudioDestination using Chromium's audio system

?

> Source/WebCore/platform/audio/nix/AudioDestinationNix.h:46
> +class AudioDestinationNix : public AudioDestination, public
Nix::AudioDevice::RenderCallback, public AudioSourceProvider {

Missing override in the implementation.

> Source/WebCore/platform/audio/nix/FFTFrameNix.cpp:2
> + * Copyright (C) 2013-2013 Nokia Corporation and/or its subsidiary(-ies).

2013-2013? :)

> Source/WebCore/platform/nix/support/AudioBusNix.cpp:2
> + * Copyright (C) 2010, Google Inc. All rights reserved.

Uh?

> Source/WebCore/platform/nix/support/AudioBusNix.cpp:58
> +    if (m_d)
> +	   delete m_d;
> +    m_d = new AudioBusPrivate(WebCore::AudioBus::create(numberOfChannels,
length));

This looks awful. Why not used STL's smart pointers?

> Tools/Scripts/webkitpy/style/checker.py:197
> +	 # name clash with other files, e.g. Canvas_h, Vector_h, etc.

Vector_h?