Bug 79196

Summary: [BlackBerry] Upstream GLES2Context.{h, cpp}
Product: WebKit Reporter: Leo Yang <leo.yang>
Component: New BugsAssignee: Leo Yang <leo.yang>
Status: RESOLVED FIXED    
Severity: Normal CC: charles.wei, rwlbuis, staikos, tonikitoo, yong.li.webkit, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 73144    
Attachments:
Description Flags
Patch
rwlbuis: review-
Patch v2
none
Patch v3 rwlbuis: review+

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?