Bug 118331 – Adding Nix files in Source/Platform to trunk

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

[Thiago de Barros Lacerda]

All Nix related files that are located under Source/Platform are being added, without any cmake files

[Thiago de Barros Lacerda]

Created attachment 205952 [details]
Patch

[Marcelo Lira]

Created attachment 206015 [details]
Patch

[Marcelo Lira]

Created attachment 206016 [details]
Patch

[Hugo Parente Lima]

This patch is out dated, I'm going to rebase/update it.

[Hugo Parente Lima]

Created attachment 211064 [details]
Patch

[Anders Carlsson]

Comment on attachment 211064 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=211064&action=review

> Source/Platform/nix/public/Vector.h:187
> +/*
> + * Copyright (C) 2009 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef Nix_Vector_h
> +#define Nix_Vector_h
> +
> +#include "Common.h"
> +
> +#include <algorithm>
> +#include <cassert>
> +
> +namespace Nix {
> +
> +// A simple vector class.
> +//
> +// Sample usage:
> +//
> +//   void Foo(WebVector<int>& result)
> +//   {
> +//       WebVector<int> data(10);
> +//       for (size_t i = 0; i < data.size(); ++i)
> +//           data[i] = ...
> +//       result.swap(data);
> +//   }
> +//
> +// It is also possible to assign from other types of random access
> +// containers:
> +//
> +//   void Foo(const std::vector<std::string>& input)
> +//   {
> +//       WebVector<WebCString> cstrings = input;
> +//       ...
> +//   }
> +//
> +template <typename T>
> +class Vector {
> +public:
> +    typedef T ValueType;
> +
> +    ~Vector()
> +    {
> +        destroy();
> +    }
> +
> +    explicit Vector(size_t size = 0)
> +    {
> +        initialize(size);
> +    }
> +
> +    Vector(const Vector<T>& other)
> +    {
> +        initializeFrom(other.m_ptr, other.m_size);
> +    }
> +
> +    template <typename C>
> +    Vector(const C& other)
> +    {
> +        initializeFrom(other.size() ? &other[0] : 0, other.size());
> +    }
> +
> +    Vector& operator=(const Vector& other)
> +    {
> +        if (this != &other)
> +            assign(other);
> +        return *this;
> +    }
> +
> +    template <typename C>
> +    Vector<T>& operator=(const C& other)
> +    {
> +        if (this != reinterpret_cast<const Vector<T>*>(&other))
> +            assign(other);
> +        return *this;
> +    }
> +
> +    template <typename C>
> +    void assign(const C& other)
> +    {
> +        assign(other.size() ? &other[0] : 0, other.size());
> +    }
> +
> +    template <typename U>
> +    void assign(const U* values, size_t size)
> +    {
> +        destroy();
> +        initializeFrom(values, size);
> +    }
> +
> +    size_t size() const { return m_size; }
> +    bool isEmpty() const { return !m_size; }
> +
> +    T& operator[](size_t i)
> +    {
> +        assert(i < m_size);
> +        return m_ptr[i];
> +    }
> +    const T& operator[](size_t i) const
> +    {
> +        assert(i < m_size);
> +        return m_ptr[i];
> +    }
> +
> +    bool contains(const T& value) const
> +    {
> +        for (size_t i = 0; i < m_size; i++) {
> +            if (m_ptr[i] == value)
> +                return true;
> +        }
> +        return false;
> +    }
> +
> +    T* data() { return m_ptr; }
> +    const T* data() const { return m_ptr; }
> +
> +    void swap(Vector<T>& other)
> +    {
> +        std::swap(m_ptr, other.m_ptr);
> +        std::swap(m_size, other.m_size);
> +    }
> +
> +private:
> +    void initialize(size_t size)
> +    {
> +        m_size = size;
> +        if (!m_size)
> +            m_ptr = 0;
> +        else {
> +            m_ptr = static_cast<T*>(::operator new(sizeof(T) * m_size));
> +            for (size_t i = 0; i < m_size; ++i)
> +                new (&m_ptr[i]) T();
> +        }
> +    }
> +
> +    template <typename U>
> +    void initializeFrom(const U* values, size_t size)
> +    {
> +        m_size = size;
> +        if (!m_size)
> +            m_ptr = 0;
> +        else {
> +            m_ptr = static_cast<T*>(::operator new(sizeof(T) * m_size));
> +            for (size_t i = 0; i < m_size; ++i)
> +                new (&m_ptr[i]) T(values[i]);
> +        }
> +    }
> +
> +    void destroy()
> +    {
> +        for (size_t i = 0; i < m_size; ++i)
> +            m_ptr[i].~T();
> +        ::operator delete(m_ptr);
> +    }
> +
> +    T* m_ptr;
> +    size_t m_size;
> +};
> +
> +} // namespace Nix
> +
> +#endif

What's this? We don't need another Vector class with the same header file name as Vector.h from WTF.

[Hugo Parente Lima]

(In reply to comment #6)
> (From update of attachment 211064 [details])
> [...]
> What's this? We don't need another Vector class with the same header file name as Vector.h from WTF.

Hi,

This vector class is part of the Nix::Platform API, this API is used to let the applications implement the backends usually found inside WebCore on other ports, i.e. Nix doesn't have a WebAudio implementation, the application using Nix must provide one.

So, the Vector.h is used by the application using Nix when using some Nix::Platform API methods, it was there because we can't export WTF to the API. You could imagine it as our QVector if we were the Qt port.

Now things that can be improved:

The current implementation was written by Chromium guys on WebKit-Chromium era, we may do two things about it:

 - Write it as a thin wrapper around WTF::Vector to avoid having yet another implementation of a Vector.
 - Use std::vector, however the decision of using std types on our API wasn't concluded yet.

Has some time since I touched these files, I'll check the code using this Vector class to see how it's used and what can be done to simplify the things up, then and post here, thanks.

[Hugo Parente Lima]

(In reply to comment #7)
> (In reply to comment #6)
> Now things that can be improved:
> 
> The current implementation was written by Chromium guys on WebKit-Chromium era, we may do two things about it:
> 
>  - Write it as a thin wrapper around WTF::Vector to avoid having yet another implementation of a Vector.
>  - Use std::vector, however the decision of using std types on our API wasn't concluded yet.
> 
> Has some time since I touched these files, I'll check the code using this Vector class to see how it's used and what can be done to simplify the things up, then and post here, thanks.

I took a look at the code using Nix::Vector, there are no good reasons to create yet another Vector class, in other words, I'm going to update it to use std::vector instead.

[Hugo Parente Lima]

Created attachment 211220 [details]
Patch

Using std::vector on public API.

[Benjamin Poulain]

Comment on attachment 211220 [details]
Patch

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?

[Hugo Parente Lima]

Thanks for the review and sorry for the delay. Most of the (bizarre) things pointed by you were already fixed, but I'll wait a bit to send a new version of this patch, I'm going to do it when I get time to put some love on this WebAudio Platform API that nowadays is a blind copy of the one used by Chromium.

[Hugo Parente Lima]

Created attachment 215193 [details]
Patch

[Hugo Parente Lima]

Created attachment 215422 [details]
Patch

Removed a FIXME and some useless copy ctors.

[Benjamin Poulain]

Comment on attachment 215193 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215193&action=review

> Source/Platform/nix/public/Color.h:54
> +        : m_argb(fixValue(a) << 24 | fixValue(r) << 16 | fixValue(g) << 8 | fixValue(b))

I think the function name fixValue() is not good.

> Source/Platform/nix/public/Common.h:29
> +// Exported symbols need to be annotated with NIX_EXPORT

Missing period.
This comment does not really add much.

> Source/Platform/nix/public/Gamepads.h:46
> +    unsigned length;
> +
> +    // Gamepad data for N separate gamepad devices.
> +    Gamepad items[itemsLengthCap];

No need to enforce object consistency through encapsulation here?

> Source/Platform/nix/public/MediaPlayer.h:54
> +    MediaPlayer(MediaPlayerClient* client)
> +        : m_playerClient(client)
> +    {
> +    }
> +
> +    virtual ~MediaPlayer()
> +    {
> +        delete m_playerClient;

The object ownership is awful here. You should take advantage of smart pointers.

> Source/Platform/nix/public/MultiChannelPCMData.h:54
> +    void* m_data;

I think this class could benefit from a better design. The ownership of m_data is odd.

> Source/Platform/nix/src/DefaultWebThemeEngine.cpp:276
> +// These values have been copied from RenderThemeChromiumSkia.cpp

Period.

> Source/Platform/nix/src/DefaultWebThemeEngine.cpp:288
> +    return progressAnimationInterval * progressAnimationFrames * 2; // "2" for back and forth;

Remove the tail semicolon, add a period.

> Source/Platform/nix/src/DefaultWebThemeEngine.cpp:335
> +    // Background

Period.

> Source/Platform/nix/src/DefaultWebThemeEngine.cpp:339
> +    // Meter

Period.

> Source/WebCore/platform/audio/nix/AudioBusNix.cpp:55
> +    FILE* file = fopen(absoluteFilename.utf8().data(), "rb");

You should not necessarily encode to utf8, but to the filesystem encoding.

On Mac there are methods to do the conversion. I cannot think of an easy way on Linux.

> Source/WebCore/platform/audio/nix/AudioBusNix.cpp:62
> +    fseek(file, 0, SEEK_END);
> +    WTF::Vector<char> fileContents;
> +    fileContents.resize(ftell(file));
> +    rewind(file);

This seems like a bad idea. Shouldn't you get the file size from the inode instead?

> Source/WebCore/platform/audio/nix/AudioDestinationNix.h:54
> +    virtual bool isPlaying() OVERRIDE { return m_isPlaying; }
> +
> +    virtual float sampleRate() const OVERRIDE { return m_sampleRate; }

I am not a fan of putting virtual function's implementation in header files.

> Source/WebCore/platform/audio/nix/FFTFrameNix.cpp:58
> +FFTFrame::~FFTFrame()
> +{
> +}

You don't need this.

[Hugo Parente Lima]

Created attachment 215435 [details]
Patch

[Hugo Parente Lima]

Thanks very much for the review!

(In reply to comment #14)
> (From update of attachment 215193 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215193&action=review
> 
> > Source/Platform/nix/public/Color.h:54
> > +        : m_argb(fixValue(a) << 24 | fixValue(r) << 16 | fixValue(g) << 8 | fixValue(b))
> 
> I think the function name fixValue() is not good.

boundValue :-)
 
> > Source/Platform/nix/public/Common.h:29
> > +// Exported symbols need to be annotated with NIX_EXPORT
> 
> Missing period.
> This comment does not really add much.

Comment removed.
 
> > Source/Platform/nix/public/Gamepads.h:46
> > +    unsigned length;
> > +
> > +    // Gamepad data for N separate gamepad devices.
> > +    Gamepad items[itemsLengthCap];
> 
> No need to enforce object consistency through encapsulation here?

You are right, but we don't plan to touch much on this gamepad API/code, the gamepad code on WebKit itself has some odd stuff like the fixed value of gamepads, using long long instead of uint64, etc. i.e. it's not worth to spend time on it right now.
 
> > Source/Platform/nix/public/MediaPlayer.h:54
> > +    MediaPlayer(MediaPlayerClient* client)
> > +        : m_playerClient(client)
> > +    {
> > +    }
> > +
> > +    virtual ~MediaPlayer()
> > +    {
> > +        delete m_playerClient;
> 
> The object ownership is awful here. You should take advantage of smart pointers.

done.
 
> > Source/Platform/nix/public/MultiChannelPCMData.h:54
> > +    void* m_data;
> 
> I think this class could benefit from a better design. The ownership of m_data is odd.
> 
> > Source/Platform/nix/src/DefaultWebThemeEngine.cpp:276
> > +// These values have been copied from RenderThemeChromiumSkia.cpp
> 
> Period.

done.
 
> > Source/Platform/nix/src/DefaultWebThemeEngine.cpp:288
> > +    return progressAnimationInterval * progressAnimationFrames * 2; // "2" for back and forth;
> 
> Remove the tail semicolon, add a period.

done.
 
> > Source/Platform/nix/src/DefaultWebThemeEngine.cpp:335
> > +    // Background
> 
> Period.

done.
 
> > Source/Platform/nix/src/DefaultWebThemeEngine.cpp:339
> > +    // Meter
> 
> Period.

done.
 
> > Source/WebCore/platform/audio/nix/AudioBusNix.cpp:55
> > +    FILE* file = fopen(absoluteFilename.utf8().data(), "rb");
> 
> You should not necessarily encode to utf8, but to the filesystem encoding.
> 
> On Mac there are methods to do the conversion. I cannot think of an easy way on Linux.

I put a FIXME just as a reminder, but the file names handled here comes from WebKit itself and I don't believe WebKit has resource files in their codebase with encodings different from ascii.
 
> > Source/WebCore/platform/audio/nix/AudioBusNix.cpp:62
> > +    fseek(file, 0, SEEK_END);
> > +    WTF::Vector<char> fileContents;
> > +    fileContents.resize(ftell(file));
> > +    rewind(file);
> 
> This seems like a bad idea. Shouldn't you get the file size from the inode instead?

done.
 
> > Source/WebCore/platform/audio/nix/AudioDestinationNix.h:54
> > +    virtual bool isPlaying() OVERRIDE { return m_isPlaying; }
> > +
> > +    virtual float sampleRate() const OVERRIDE { return m_sampleRate; }
> 
> I am not a fan of putting virtual function's implementation in header files.
> 
> > Source/WebCore/platform/audio/nix/FFTFrameNix.cpp:58
> > +FFTFrame::~FFTFrame()
> > +{
> > +}
> 
> You don't need this.

The destructor is declared on FFTFrame.h, a file shared by all ports with a different implementation for each port, so if I don't define it I'll get a linker error.

[WebKit Commit Bot]

Comment on attachment 215435 [details]
Patch

Clearing flags on attachment: 215435

Committed r158235: <http://trac.webkit.org/changeset/158235>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.