Bug 79196 - [BlackBerry] Upstream GLES2Context.{h, cpp}
Summary: [BlackBerry] Upstream GLES2Context.{h, cpp}
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Leo Yang
URL:
Keywords:
Depends on:
Blocks: 73144
  Show dependency tree / graph
 
Reported: 2012-02-21 22:17 PST by Leo Yang
Modified: 2012-04-24 11:12 PDT (History)
6 users (show)

See Also:

Attachments
Patch (8.23 KB, patch)
2012-02-22 01:32 PST, Leo Yang 		rwlbuis: review-
Details | Formatted Diff | Diff
Patch v2 (8.33 KB, patch)
2012-02-23 01:50 PST, Leo Yang 		no flags Details | Formatted Diff | Diff
Patch v3 (8.32 KB, patch)
2012-02-23 01:54 PST, Leo Yang 		rwlbuis: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Leo Yang 2012-02-21 22:17:12 PST 
OpenGL ES 2.0 context for the blackberry porting.
Comment 1 Leo Yang 2012-02-22 01:32:12 PST 
Created attachment 128155 [details]
Patch
Comment 2 Rob Buis 2012-02-22 04:14:44 PST 
Comment on attachment 128155 [details]
Patch

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

Still some things to improve.

> Source/WebKit/blackberry/WebKitSupport/GLES2Context.cpp:42
> +#include "WebPage_p.h"

All needed?

> Source/WebKit/blackberry/WebKitSupport/GLES2Context.cpp:44
> +#include <BlackBerryPlatformWindow.h>

Already included in header.

> Source/WebKit/blackberry/WebKitSupport/GLES2Context.cpp:113
> +    // Sniff, this is so sad.

Seems a strange comment, maybe explain why it is sad

> Source/WebKit/blackberry/WebKitSupport/GLES2Context.h:39
> +#include <wtf/OwnPtr.h>

Is this one needed?
Comment 3 Leo Yang 2012-02-23 01:49:46 PST 
(In reply to comment #2)
> (From update of attachment 128155 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128155&action=review
> 
> Still some things to improve.
> 
> > Source/WebKit/blackberry/WebKitSupport/GLES2Context.cpp:42
> > +#include "WebPage_p.h"
> 
> All needed?
> 
Yes. WebPagePrivate is being used in the constructor.

> > Source/WebKit/blackberry/WebKitSupport/GLES2Context.cpp:44
> > +#include <BlackBerryPlatformWindow.h>
> 
> Already included in header.
I'll remove it.

> 
> > Source/WebKit/blackberry/WebKitSupport/GLES2Context.cpp:113
> > +    // Sniff, this is so sad.
> 
> Seems a strange comment, maybe explain why it is sad
> 
I'll make a clear comment.

> > Source/WebKit/blackberry/WebKitSupport/GLES2Context.h:39
> > +#include <wtf/OwnPtr.h>
> 
> Is this one needed?
I'll remove it.
Comment 4 Leo Yang 2012-02-23 01:50:35 PST 
Created attachment 128435 [details]
Patch v2
Comment 5 Leo Yang 2012-02-23 01:54:22 PST 
Created attachment 128436 [details]
Patch v3

Removing some unnecessary blank lines.
Comment 6 Rob Buis 2012-02-23 04:30:29 PST 
Comment on attachment 128436 [details]
Patch v3

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

LGTM.

> Source/WebKit/blackberry/ChangeLog:1
> +2012-02-22  Leo Yang  <leo.yang@torchmobile.com.cn>

It is 23 feb now.

> Source/WebKit/blackberry/WebKitSupport/GLES2Context.cpp:60
> +    if (surface)

Can just do if (BackingStoreCompositingSurface* surface = compositingSurface())

> Source/WebKit/blackberry/WebKitSupport/GLES2Context.cpp:91
> +    if (surface)

Ditto
Comment 7 Leo Yang 2012-02-23 18:15:51 PST 
Committed r108711: <http://trac.webkit.org/changeset/108711>
Comment 8 Yong Li 2012-04-24 11:12:35 PDT 
Do we know why the 2 files are put in WebKitSupport rather than WebCoreSupport?