Bug 118331

Created attachment 205952 [details] Patch

Created attachment 206015 [details] Patch

Created attachment 206016 [details] Patch

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

Created attachment 211064 [details] Patch

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.

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

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

Created attachment 211220 [details] Patch Using std::vector on public API.

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?

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.

Created attachment 215193 [details] Patch

Created attachment 215422 [details] Patch Removed a FIXME and some useless copy ctors.

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.

Created attachment 215435 [details] Patch

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.

Comment on attachment 215435 [details] Patch Clearing flags on attachment: 215435 Committed r158235: <http://trac.webkit.org/changeset/158235>

All reviewed patches have been landed. Closing bug.