[Webkit-unassigned] [Bug 62961] [EFL] Add GraphicsContext3DInternal implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=62961


Raphael Kubo da Costa <kubo at profusion.mobi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kubo at profusion.mobi




--- Comment #12 from Raphael Kubo da Costa <kubo at profusion.mobi>  2011-06-20 07:01:11 PST ---
Informal r- from my side:

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:2
> +    Copyright (C) 2009-2011 Samsung Electronics

This file is new, so the copyright should start in 2011.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:57
> +bool GraphicsContext3DInternal::initialize(GraphicsContext3D::Attributes attrs, HostWindow* hostWindow, bool bRenderDirectlyToEvasGLObject)

I'd rather if instead of having a public constructor and this method you could have a static create() method which took care of creating and initializing the object automatically.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:144
> +void GraphicsContext3DInternal::bindAttribLocation(Platform3DObject program, GC3Duint index, const char* name)

I think it makes more sense to make name a String, and call .utf8().data() here.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:394
> +    char* name = static_cast<char*>(malloc(maxNameLength * sizeof(char)));

sizeof(char) is always 1.

Manually managing the memory allocated here should not be necessary. You can use, for example, an OwnArrayPtr: OwnArrayPtr<char> name = adoptArrayPtr(new char[maxNameLength]);

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:431
> +    char* name = static_cast<char*>(malloc(maxNameLength * sizeof(char)));

Ditto.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:486
> +        ListHashSet<GC3Denum>::iterator iter = m_syntheticErrors.begin();

I think it's clearer to do

  GC3Denum err = m_syntheticErrors.first();
  m_syntheticErrors.remove(m_syntheticErrors.begin());
  return err;

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:545
> +    char* log = static_cast<char*>(malloc(logLength * sizeof(char)));

Same comment about manual memory management.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:579
> +    char* log = static_cast<char*>(malloc(logLength * sizeof(char)));

Ditto.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:601
> +    char* log = static_cast<char*>(malloc(logLength * sizeof(char)));

Ditto.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:1113
> +    return 0;

Missing notImplemented()?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.h:2
> +    Copyright (C) 2009-2011 Samsung Electronics

This file is new, so the copyright should start in 2011.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.