ContentShell rendering support on Android.

ContentShell rendering support on Android.

This renders to a SurfaceView using texture sharing on the GPU
thread. There is a bit of a public interface added to let the
app create a cmdbuffer (we want run GL contexts on the same thread
when sharing resources on Android) and perform the handshaking
needed for sharing the two compositor buffers.

It has bugs, such as that it doesn't handle it yet when the UI
surface handle changes (e.g. when hiding and restoring the app).

BUG=136923
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149327

[no sievers, 2012-07-27 03:54:08]

This is not necessarily the way Android will keep rendering, but it's a more or
less intermediate solution to get some frames on the screen and unblock people.

The current plan is to eventually run more of the compositor stack in the
browser process, which means that the public interfaces I added are to be taken
with a grain of salt.

[Satish, 2012-07-27 09:49:24]

drive-by comments..

It would be useful to have tests for the new graphics context code and also for
the java code to ensure the created/destroyed callbacks are coming properly

https://chromiumcodereview.appspot.com/10823051/diff/1/content/browser/androi...
File content/browser/android/graphics_context.cc (right):

https://chromiumcodereview.appspot.com/10823051/diff/1/content/browser/androi...
content/browser/android/graphics_context.cc:23: class CmdBufferGraphicsContext :
public content::GraphicsContext {
code within namespace{} isn't indented to the right, so you can indent all this
by 2 spaces to the left. also add a newline above and a class level comment

https://chromiumcodereview.appspot.com/10823051/diff/1/content/browser/androi...
content/browser/android/graphics_context.cc:94: if (!context->Initialize(
suggest using braces for if statements that span multiple lines like this one
(and not just when the body is multiple lines

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
File content/shell/android/java/src/org/chromium/content_shell/Shell.java
(right):

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:77:
loadUrl("http://www.google.com");
is this required or was it test code to be removed?

[klobag.chromium, 2012-07-27 19:42:22]

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
File content/shell/android/java/src/org/chromium/content_shell/Shell.java
(left):

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:62:
initilizeUrlField();
I don't think we need to move this line as it just sets up the EditTextView. I
think you want to defer any call to loadUrl until the first onSurfaceCreated is
called to ensure the main context is created.

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
File content/shell/android/java/src/org/chromium/content_shell/Shell.java
(right):

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:77:
loadUrl("http://www.google.com");
On 2012/07/27 09:49:24, Satish wrote:
> is this required or was it test code to be removed?

You may not want to do it for every onSurfaceCreated(), only the first
onSurfaceCreated() to indicate the app is ready.

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
File content/shell/android/shell_manager.cc (right):

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
content/shell/android/shell_manager.cc:83: NOTIMPLEMENTED();
Should we delete the context here as we can't touch the surface after this
method is returned?

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/shell_br...
File content/shell/shell_browser_main_parts.cc (right):

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/shell_br...
content/shell/shell_browser_main_parts.cc:33: #if defined(OS_ANDROID)
Consider to modify the commandLine args to pass "".

[aelias_OOO_until_Jul13, 2012-07-27 19:58:24]

This code is almost all temporary scaffolding to get content shell up ASAP, so
tests aren't appropriate for it.

http://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/shell_and...
File content/shell/shell_android.cc (right):

http://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/shell_and...
content/shell/shell_android.cc:28: // TODO(sievers): This should just assert the
right flags are
These will be added by Yusuf's gesture patch
http://chromiumcodereview.appspot.com/10790066

[Ted C, 2012-07-27 20:31:14]

mainly style nits...

passing the startup url is something that needs to get cleaned up, but we can
fix it after this lands if it is too involved.  It is probably better to render
something then navigate to URL passed from the command line :-)

https://chromiumcodereview.appspot.com/10823051/diff/1/content/browser/androi...
File content/browser/android/draw_delegate.cc (right):

https://chromiumcodereview.appspot.com/10823051/diff/1/content/browser/androi...
content/browser/android/draw_delegate.cc:32: };  // namespace content
; not required

https://chromiumcodereview.appspot.com/10823051/diff/1/content/browser/androi...
File content/browser/android/graphics_context.cc (right):

https://chromiumcodereview.appspot.com/10823051/diff/1/content/browser/androi...
content/browser/android/graphics_context.cc:59: } // anonymous namespace
two spaces between the } and //

https://chromiumcodereview.appspot.com/10823051/diff/1/content/browser/androi...
content/browser/android/graphics_context.cc:73: LOG(INFO) << "Creating context
for window " << window;
remove?

https://chromiumcodereview.appspot.com/10823051/diff/1/content/browser/androi...
content/browser/android/graphics_context.cc:97:
content::CAUSE_FOR_GPU_LAUNCH_WEBGRAPHICSCONTEXT3DCOMMANDBUFFERIMPL_INITIALIZE))
this is in fact the longest constant known to man...that is all.

actually can you drop the content:: as you are in the content namespace to maybe
fit it on one line?

https://chromiumcodereview.appspot.com/10823051/diff/1/content/browser/androi...
content/browser/android/graphics_context.cc:119: } // namespace content
one more space here too

https://chromiumcodereview.appspot.com/10823051/diff/1/content/public/browser...
File content/public/browser/android/draw_delegate.h (right):

https://chromiumcodereview.appspot.com/10823051/diff/1/content/public/browser...
content/public/browser/android/draw_delegate.h:53: };  // namespace content
no semi-colon

https://chromiumcodereview.appspot.com/10823051/diff/1/content/public/browser...
File content/public/browser/android/graphics_context.h (right):

https://chromiumcodereview.appspot.com/10823051/diff/1/content/public/browser...
content/public/browser/android/graphics_context.h:33: } // namespace content
extra space

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
File content/shell/android/draw_context.cc (right):

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
content/shell/android/draw_context.cc:37: -1.0f, -1.0f,
increase indent by one or should this be 4 and not 2?

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
content/shell/android/draw_context.cc:49: GL_STATIC_DRAW);
align with above

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
content/shell/android/draw_context.cc:66: 
remove blank line

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
File content/shell/android/java/src/org/chromium/content_shell/Shell.java
(right):

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:58: public
void setSurfaceView(View surfaceView) {
javadoc for these public methods...even if it simply says they are temporary.

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:77:
loadUrl("http://www.google.com");
On 2012/07/27 19:42:22, klobag.chromium wrote:
> On 2012/07/27 09:49:24, Satish wrote:
> > is this required or was it test code to be removed?
> 
> You may not want to do it for every onSurfaceCreated(), only the first
> onSurfaceCreated() to indicate the app is ready.

loadUrl happens as part of shell creation during startup, is the extra required?
 Can we just call reload?

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
File content/shell/android/shell_manager.cc (right):

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
content/shell/android/shell_manager.cc:71: LOG(INFO) << "SurfaceCreated";
remove log?

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/android/...
content/shell/android/shell_manager.cc:92: if (view) {
no braces needed

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/shell_br...
File content/shell/shell_browser_main_parts.cc (right):

https://chromiumcodereview.appspot.com/10823051/diff/1/content/shell/shell_br...
content/shell/shell_browser_main_parts.cc:33: #if defined(OS_ANDROID)
On 2012/07/27 19:42:22, klobag.chromium wrote:
> Consider to modify the commandLine args to pass "".

Ah ha...I see the load URL need now.  We need to figure out a way to pass an URL
set at startup to the shells.  Hmm...

[jam, 2012-07-27 20:46:29]

http://codereview.chromium.org/10823051/diff/1/content/public/browser/android...
File content/public/browser/android/draw_delegate.h (right):

http://codereview.chromium.org/10823051/diff/1/content/public/browser/android...
content/public/browser/android/draw_delegate.h:18: class DrawDelegate {
this isn't really an interface. why is all this internal implementation detail
part of the interface? the interface should only have stuff that needs to be
called from embedders.

it looks like only OnSurfaceUpdated and SetDrawSurface are called from outside
of content. these should be pure virtual, and the other functions should be
taken out of the interface. see
http://www.chromium.org/developers/content-module/content-api for more info

http://codereview.chromium.org/10823051/diff/1/content/public/browser/android...
File content/public/browser/android/graphics_context.h (right):

http://codereview.chromium.org/10823051/diff/1/content/public/browser/android...
content/public/browser/android/graphics_context.h:20: GraphicsContext();
nit: get rid of this

http://codereview.chromium.org/10823051/diff/1/content/public/browser/android...
content/public/browser/android/graphics_context.h:21: virtual
~GraphicsContext();
nit: inline this

http://codereview.chromium.org/10823051/diff/1/content/public/browser/android...
content/public/browser/android/graphics_context.h:30:
DISALLOW_COPY_AND_ASSIGN(GraphicsContext);
nit: not needed on interfaces

http://codereview.chromium.org/10823051/diff/1/content/shell/android/draw_con...
File content/shell/android/draw_context.h (right):

http://codereview.chromium.org/10823051/diff/1/content/shell/android/draw_con...
content/shell/android/draw_context.h:19: class DrawContext {
nit: all new code in content should be in the content namespace

[Satish, 2012-07-30 09:51:25]

On 2012/07/27 19:58:24, aelias wrote:
> This code is almost all temporary scaffolding to get content shell up ASAP, so
> tests aren't appropriate for it.

How long is this code expected to remain before replaced with a permanent
solution? The first comment says 'eventually run more of the compositor stack in
the browser process' which seems like could take many weeks.. in which case
tests seem appropriate? We don't want rendering to break due to some other CL
and not notice it on the bots.

[no sievers, 2012-07-31 01:28:44]

http://codereview.chromium.org/10823051/diff/1/content/browser/android/draw_d...
File content/browser/android/draw_delegate.cc (right):

http://codereview.chromium.org/10823051/diff/1/content/browser/android/draw_d...
content/browser/android/draw_delegate.cc:32: };  // namespace content
On 2012/07/27 20:31:15, Ted C wrote:
> ; not required

Done.

http://codereview.chromium.org/10823051/diff/1/content/browser/android/graphi...
File content/browser/android/graphics_context.cc (right):

http://codereview.chromium.org/10823051/diff/1/content/browser/android/graphi...
content/browser/android/graphics_context.cc:23: class CmdBufferGraphicsContext :
public content::GraphicsContext {
On 2012/07/27 09:49:24, Satish wrote:
> code within namespace{} isn't indented to the right, so you can indent all
this
> by 2 spaces to the left. also add a newline above and a class level comment

Done.

http://codereview.chromium.org/10823051/diff/1/content/browser/android/graphi...
content/browser/android/graphics_context.cc:59: } // anonymous namespace
On 2012/07/27 20:31:15, Ted C wrote:
> two spaces between the } and //

Done.

http://codereview.chromium.org/10823051/diff/1/content/browser/android/graphi...
content/browser/android/graphics_context.cc:73: LOG(INFO) << "Creating context
for window " << window;
On 2012/07/27 20:31:15, Ted C wrote:
> remove?

Done.

http://codereview.chromium.org/10823051/diff/1/content/browser/android/graphi...
content/browser/android/graphics_context.cc:94: if (!context->Initialize(
On 2012/07/27 09:49:24, Satish wrote:
> suggest using braces for if statements that span multiple lines like this one
> (and not just when the body is multiple lines

Done.

http://codereview.chromium.org/10823051/diff/1/content/browser/android/graphi...
content/browser/android/graphics_context.cc:97:
content::CAUSE_FOR_GPU_LAUNCH_WEBGRAPHICSCONTEXT3DCOMMANDBUFFERIMPL_INITIALIZE))
On 2012/07/27 20:31:15, Ted C wrote:
> this is in fact the longest constant known to man...that is all.
> 
> actually can you drop the content:: as you are in the content namespace to
maybe
> fit it on one line?

Done.

http://codereview.chromium.org/10823051/diff/1/content/browser/android/graphi...
content/browser/android/graphics_context.cc:119: } // namespace content
On 2012/07/27 20:31:15, Ted C wrote:
> one more space here too

Done.

http://codereview.chromium.org/10823051/diff/1/content/public/browser/android...
File content/public/browser/android/draw_delegate.h (right):

http://codereview.chromium.org/10823051/diff/1/content/public/browser/android...
content/public/browser/android/draw_delegate.h:18: class DrawDelegate {
I split this into DrawDelegate and DrawDelegateImpl.

On 2012/07/27 20:46:29, John Abd-El-Malek wrote:
> this isn't really an interface. why is all this internal implementation detail
> part of the interface? the interface should only have stuff that needs to be
> called from embedders.
> 
> it looks like only OnSurfaceUpdated and SetDrawSurface are called from outside
> of content. these should be pure virtual, and the other functions should be
> taken out of the interface. see
> http://www.chromium.org/developers/content-module/content-api for more info

http://codereview.chromium.org/10823051/diff/1/content/public/browser/android...
content/public/browser/android/draw_delegate.h:53: };  // namespace content
On 2012/07/27 20:31:15, Ted C wrote:
> no semi-colon

Done.

http://codereview.chromium.org/10823051/diff/1/content/public/browser/android...
File content/public/browser/android/graphics_context.h (right):

http://codereview.chromium.org/10823051/diff/1/content/public/browser/android...
content/public/browser/android/graphics_context.h:20: GraphicsContext();
On 2012/07/27 20:46:29, John Abd-El-Malek wrote:
> nit: get rid of this

Done.

http://codereview.chromium.org/10823051/diff/1/content/public/browser/android...
content/public/browser/android/graphics_context.h:21: virtual
~GraphicsContext();
On 2012/07/27 20:46:29, John Abd-El-Malek wrote:
> nit: inline this

Done.

http://codereview.chromium.org/10823051/diff/1/content/public/browser/android...
content/public/browser/android/graphics_context.h:30:
DISALLOW_COPY_AND_ASSIGN(GraphicsContext);
On 2012/07/27 20:46:29, John Abd-El-Malek wrote:
> nit: not needed on interfaces

Done.

http://codereview.chromium.org/10823051/diff/1/content/public/browser/android...
content/public/browser/android/graphics_context.h:33: } // namespace content
On 2012/07/27 20:31:15, Ted C wrote:
> extra space

Done.

http://codereview.chromium.org/10823051/diff/1/content/shell/android/draw_con...
File content/shell/android/draw_context.cc (right):

http://codereview.chromium.org/10823051/diff/1/content/shell/android/draw_con...
content/shell/android/draw_context.cc:37: -1.0f, -1.0f,
On 2012/07/27 20:31:15, Ted C wrote:
> increase indent by one or should this be 4 and not 2?

Done.

http://codereview.chromium.org/10823051/diff/1/content/shell/android/draw_con...
content/shell/android/draw_context.cc:49: GL_STATIC_DRAW);
On 2012/07/27 20:31:15, Ted C wrote:
> align with above

Done.

http://codereview.chromium.org/10823051/diff/1/content/shell/android/draw_con...
content/shell/android/draw_context.cc:66: 
On 2012/07/27 20:31:15, Ted C wrote:
> remove blank line

Done.

http://codereview.chromium.org/10823051/diff/1/content/shell/android/draw_con...
File content/shell/android/draw_context.h (right):

http://codereview.chromium.org/10823051/diff/1/content/shell/android/draw_con...
content/shell/android/draw_context.h:19: class DrawContext {
On 2012/07/27 20:46:29, John Abd-El-Malek wrote:
> nit: all new code in content should be in the content namespace

Done.

http://codereview.chromium.org/10823051/diff/1/content/shell/android/java/src...
File content/shell/android/java/src/org/chromium/content_shell/Shell.java
(left):

http://codereview.chromium.org/10823051/diff/1/content/shell/android/java/src...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:62:
initilizeUrlField();
On 2012/07/27 19:42:22, klobag.chromium wrote:
> I don't think we need to move this line as it just sets up the EditTextView. I
> think you want to defer any call to loadUrl until the first onSurfaceCreated
is
> called to ensure the main context is created.

Done.

http://codereview.chromium.org/10823051/diff/1/content/shell/android/java/src...
File content/shell/android/java/src/org/chromium/content_shell/Shell.java
(right):

http://codereview.chromium.org/10823051/diff/1/content/shell/android/java/src...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:58: public
void setSurfaceView(View surfaceView) {
On 2012/07/27 20:31:15, Ted C wrote:
> javadoc for these public methods...even if it simply says they are temporary.

Done.

http://codereview.chromium.org/10823051/diff/1/content/shell/android/java/src...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:77:
loadUrl("http://www.google.com");
Required for now since we have to create the UI graphics context first (which
needs the surface) before we can create a renderer. (There currently is a
dependency between compositor and UI context for texture image transport).

On 2012/07/27 09:49:24, Satish wrote:
> is this required or was it test code to be removed?

http://codereview.chromium.org/10823051/diff/1/content/shell/android/shell_ma...
File content/shell/android/shell_manager.cc (right):

http://codereview.chromium.org/10823051/diff/1/content/shell/android/shell_ma...
content/shell/android/shell_manager.cc:71: LOG(INFO) << "SurfaceCreated";
On 2012/07/27 20:31:15, Ted C wrote:
> remove log?

Done.

http://codereview.chromium.org/10823051/diff/1/content/shell/android/shell_ma...
content/shell/android/shell_manager.cc:83: NOTIMPLEMENTED();
On 2012/07/27 19:42:22, klobag.chromium wrote:
> Should we delete the context here as we can't touch the surface after this
> method is returned?

Done.

http://codereview.chromium.org/10823051/diff/1/content/shell/android/shell_ma...
content/shell/android/shell_manager.cc:92: if (view) {
On 2012/07/27 20:31:15, Ted C wrote:
> no braces needed

Done.

http://codereview.chromium.org/10823051/diff/1/content/shell/shell_android.cc
File content/shell/shell_android.cc (right):

http://codereview.chromium.org/10823051/diff/1/content/shell/shell_android.cc...
content/shell/shell_android.cc:28: // TODO(sievers): This should just assert the
right flags are
Cool, I turned it into a dcheck() then.

On 2012/07/27 19:58:24, aelias wrote:
> These will be added by Yusuf's gesture patch
> http://chromiumcodereview.appspot.com/10790066

http://codereview.chromium.org/10823051/diff/1/content/shell/shell_browser_ma...
File content/shell/shell_browser_main_parts.cc (right):

http://codereview.chromium.org/10823051/diff/1/content/shell/shell_browser_ma...
content/shell/shell_browser_main_parts.cc:33: #if defined(OS_ANDROID)
Hrm, I had no immediate luck. gurl.cc would dcheck() about an invalid url spec.

On 2012/07/27 19:42:22, klobag.chromium wrote:
> Consider to modify the commandLine args to pass "".

[aelias_OOO_until_Jul13, 2012-07-31 03:49:28]

On 2012/07/30 09:51:25, Satish wrote:
> On 2012/07/27 19:58:24, aelias wrote:
> > This code is almost all temporary scaffolding to get content shell up ASAP,
so
> > tests aren't appropriate for it.
> 
> How long is this code expected to remain before replaced with a permanent
> solution? The first comment says 'eventually run more of the compositor stack
in
> the browser process' which seems like could take many weeks.. in which case
> tests seem appropriate? We don't want rendering to break due to some other CL
> and not notice it on the bots.

I still don't think it's worth the time.  We are in a hurry to get content shell
running, and after that to replace it with the final architecture.  If this
breaks we'll deal with it when that happens; we're not releasing anything off
this.

[Ted C, 2012-07-31 20:58:05]

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/java/...
File
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java
(right):

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/java/...
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java:28:
private static final String DEFAULT_SHELL_URL = "http://www.google.com";
drop the private here and use the same constant in ShellManager for the initial
value of the startup URL.

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/java/...
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java:47:
if (!TextUtils.isEmpty(startupUrl)) {
Move the declaration of startupUrl to right above this block as this isn't used
up there anymore.

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/java/...
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java:48:
startupUrl = Shell.sanitizeUrl(startupUrl);
no need to further modify startupUrl.

This can just be:
mShellManager.setStartupUrl(Shell.sanitizeUrl(startupUrl));

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/java/...
File content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
(right):

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/java/...
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:40:
public void surfaceChanged(SurfaceHolder holder, int format, int width, int
height) {
add @Override to these methods.

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/shell...
File content/shell/android/shell_manager.cc (right):

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/shell...
content/shell/android/shell_manager.cc:88: NOTIMPLEMENTED();
this seems like a bit of a fib...maybe make this a TODO to have it do something
more useful?

[no sievers, 2012-07-31 21:17:16]

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/java/...
File
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java
(right):

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/java/...
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java:28:
private static final String DEFAULT_SHELL_URL = "http://www.google.com";
On 2012/07/31 20:58:05, Ted C wrote:
> drop the private here and use the same constant in ShellManager for the
initial
> value of the startup URL.

Done.

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/java/...
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java:47:
if (!TextUtils.isEmpty(startupUrl)) {
On 2012/07/31 20:58:05, Ted C wrote:
> Move the declaration of startupUrl to right above this block as this isn't
used
> up there anymore.

Done.

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/java/...
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java:48:
startupUrl = Shell.sanitizeUrl(startupUrl);
On 2012/07/31 20:58:05, Ted C wrote:
> no need to further modify startupUrl.
> 
> This can just be:
> mShellManager.setStartupUrl(Shell.sanitizeUrl(startupUrl));

Done.

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/java/...
File content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
(right):

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/java/...
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:40:
public void surfaceChanged(SurfaceHolder holder, int format, int width, int
height) {
On 2012/07/31 20:58:05, Ted C wrote:
> add @Override to these methods.

Done.

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/shell...
File content/shell/android/shell_manager.cc (right):

http://codereview.chromium.org/10823051/diff/4003/content/shell/android/shell...
content/shell/android/shell_manager.cc:88: NOTIMPLEMENTED();
I'll remove the NOTIMPLEMENTED() here.
The problem is not really in this code, but somewhere else so we can let it
dcheck() or crash there. (The context already glFinishes() here before we return
and let the framework destroy the surface but it's the dependency from the other
compositor context mostly, I think.)

On 2012/07/31 20:58:05, Ted C wrote:
> this seems like a bit of a fib...maybe make this a TODO to have it do
something
> more useful?

[Ted C, 2012-07-31 21:25:08]

lgtm

[no sievers, 2012-07-31 21:40:52]

Need jam for OWNERS.

[jam, 2012-07-31 22:46:14]

lgtm

http://codereview.chromium.org/10823051/diff/9018/content/browser/android/dra...
File content/browser/android/draw_delegate_impl.h (right):

http://codereview.chromium.org/10823051/diff/9018/content/browser/android/dra...
content/browser/android/draw_delegate_impl.h:35: gfx::Size size_;
nit: disallow_copy_and_assign

http://codereview.chromium.org/10823051/diff/9018/content/browser/android/gra...
File content/browser/android/graphics_context.cc (right):

http://codereview.chromium.org/10823051/diff/9018/content/browser/android/gra...
content/browser/android/graphics_context.cc:54: private:
nit: blank line above

http://codereview.chromium.org/10823051/diff/9018/content/public/browser/andr...
File content/public/browser/android/draw_delegate.h (right):

http://codereview.chromium.org/10823051/diff/9018/content/public/browser/andr...
content/public/browser/android/draw_delegate.h:36: virtual void
SetBounds(gfx::Size& size) = 0;
nit: const ref

http://codereview.chromium.org/10823051/diff/9018/content/shell/android/draw_...
File content/shell/android/draw_context.h (right):

http://codereview.chromium.org/10823051/diff/9018/content/shell/android/draw_...
content/shell/android/draw_context.h:28: private:
nit: blank line above

http://codereview.chromium.org/10823051/diff/9018/content/shell/android/shell...
File content/shell/android/shell_manager.cc (right):

http://codereview.chromium.org/10823051/diff/9018/content/shell/android/shell...
content/shell/android/shell_manager.cc:17: #include
"content/public/browser/android/draw_delegate.h"
nit: order

[commit-bot: I haz the power, 2012-07-31 23:10:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/10823051/9020

[commit-bot: I haz the power, 2012-08-01 00:16:53]

Change committed as 149327