Add synchronous requestDrawGL

Add synchronous requestDrawGL

And stop using executeHardwareAction and use synchronous
requestDrawGL. The branching structure needs to happen in DrawGL.

Add a queue of closures to SharedRendererState. So the general pattern is
1. Add closure to queue
2. requestDrawGL

Need to be careful not to let the queue grow unboundedly if requestDrawGL
returns false.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267484

[boliu, 2014-04-28 22:37:59]

Still work in progress, but early preview for ya. I think I'm going to break
this apart into separate CLs.

[boliu, 2014-04-30 18:39:03]

TrimMemory retaining current behavior

[boliu, 2014-04-30 20:40:40]

Hmm, still broke something. Specifically this sequence in android browser:

Start browser, webview gets attached
Click browser tab switcher, webview gets detached
Click app switcher button
Select browser app again
Select the tab to attach webview again

At this point it's no longer drawing

[boliu, 2014-04-30 23:22:11]

On 2014/04/30 20:40:40, boliu wrote:
> Hmm, still broke something. Specifically this sequence in android browser:
> 
> Start browser, webview gets attached
> Click browser tab switcher, webview gets detached
> Click app switcher button
> Select browser app again
> Select the tab to attach webview again
> 
> At this point it's no longer drawing

Found the issue. Apparently in that sequence, webview is attached then detached
without drawing, and in detach, we leave the release closure in the queue, and
the next time draw comes it just clears the queue and not intialize.

sgurun for stamp after hush reviews

[hush (inactive), 2014-05-01 00:35:54]

https://codereview.chromium.org/255023004/diff/120001/android_webview/browser...
File android_webview/browser/browser_view_renderer_client.h (right):

https://codereview.chromium.org/255023004/diff/120001/android_webview/browser...
android_webview/browser/browser_view_renderer_client.h:20: // together with
|canvas|. The callback may never be made, and the mode may
do you mean |wait_for_completion| should not be true if the canvas is not null?

https://codereview.chromium.org/255023004/diff/120001/android_webview/browser...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/255023004/diff/120001/android_webview/browser...
android_webview/browser/hardware_renderer.cc:146: LOG(ERROR) <<
"------------------- HardwareRenderer::TrimMemory";
remove?

https://codereview.chromium.org/255023004/diff/120001/android_webview/buildbo...
File android_webview/buildbot/aosp_manifest.xml (right):

https://codereview.chromium.org/255023004/diff/120001/android_webview/buildbo...
android_webview/buildbot/aosp_manifest.xml:247: <project
path="frameworks/webview" name="platform/frameworks/webview"
revision="24c4a3d70e8a2f1001e100ac78c0c1673c5710c3" remote="aosp" />
what is this hash? How is it related to this CL?

https://codereview.chromium.org/255023004/diff/120001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/255023004/diff/120001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:139: *
Should avoid setting |waitForCompletion| when |canvas| is not null.
what happens if canvas is not null and waitForCompletion is true?

https://codereview.chromium.org/255023004/diff/120001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/255023004/diff/120001/android_webview/native/...
android_webview/native/aw_contents.cc:741:
shared_renderer_state_.SetHardwareInitialized(true);
why do we need this?
We are resetting hardware_render_ every time we call SetHardwareInitialized.
hardware_renderer_ != NULL is equivalent to
shared_renderer_state_.IsHardwareInitialized() returning true?

https://codereview.chromium.org/255023004/diff/120001/android_webview/native/...
android_webview/native/aw_contents.cc:750: bool draw_functor_succeeded =
RequestDrawGL(NULL, true);
these 3 lines seem like a good candidate for a member function on
shared_renderer_state.
Basically you are trying to post a "closure" to the render thread, from the UI
thread, and block the UI thread until the render thread is done running it.

Maybe put it into something like
bool PostTaskToRenderThreadForResult(Closure&)

[boliu, 2014-05-01 00:46:36]

https://codereview.chromium.org/255023004/diff/120001/android_webview/browser...
File android_webview/browser/browser_view_renderer_client.h (right):

https://codereview.chromium.org/255023004/diff/120001/android_webview/browser...
android_webview/browser/browser_view_renderer_client.h:20: // together with
|canvas|. The callback may never be made, and the mode may
On 2014/05/01 00:35:54, hush wrote:
> do you mean |wait_for_completion| should not be true if the canvas is not
null?

Yes. Fixed up comment.

https://codereview.chromium.org/255023004/diff/120001/android_webview/browser...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/255023004/diff/120001/android_webview/browser...
android_webview/browser/hardware_renderer.cc:146: LOG(ERROR) <<
"------------------- HardwareRenderer::TrimMemory";
On 2014/05/01 00:35:54, hush wrote:
> remove?

oops, done

https://codereview.chromium.org/255023004/diff/120001/android_webview/buildbo...
File android_webview/buildbot/aosp_manifest.xml (right):

https://codereview.chromium.org/255023004/diff/120001/android_webview/buildbo...
android_webview/buildbot/aosp_manifest.xml:247: <project
path="frameworks/webview" name="platform/frameworks/webview"
revision="24c4a3d70e8a2f1001e100ac78c0c1673c5710c3" remote="aosp" />
On 2014/05/01 00:35:54, hush wrote:
> what is this hash? How is it related to this CL?

To get the new RequestDrawGL that takes waitForCompletion param. Apparently I
can't open aosp gerrit on vpn, but I landed it yesterday and manually got the
sha1. Otherwise android_aosp bot wouldn't compile.

https://codereview.chromium.org/255023004/diff/120001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/255023004/diff/120001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:139: *
Should avoid setting |waitForCompletion| when |canvas| is not null.
On 2014/05/01 00:35:54, hush wrote:
> what happens if canvas is not null and waitForCompletion is true?

"Undefined" :p

I'm not sure. I think it will either deadlock, or waitForCompletion won't do
anything. Either way it should be avoided for now.

https://codereview.chromium.org/255023004/diff/120001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/255023004/diff/120001/android_webview/native/...
android_webview/native/aw_contents.cc:741:
shared_renderer_state_.SetHardwareInitialized(true);
On 2014/05/01 00:35:54, hush wrote:
> why do we need this?
> We are resetting hardware_render_ every time we call SetHardwareInitialized.
> hardware_renderer_ != NULL is equivalent to
> shared_renderer_state_.IsHardwareInitialized() returning true?

Can't query hardware_renderer_ on UI thread since it lives on RT.

https://codereview.chromium.org/255023004/diff/120001/android_webview/native/...
android_webview/native/aw_contents.cc:750: bool draw_functor_succeeded =
RequestDrawGL(NULL, true);
On 2014/05/01 00:35:54, hush wrote:
> these 3 lines seem like a good candidate for a member function on
> shared_renderer_state.
> Basically you are trying to post a "closure" to the render thread, from the UI
> thread, and block the UI thread until the render thread is done running it.
> 
> Maybe put it into something like
> bool PostTaskToRenderThreadForResult(Closure&)

I thought about it, but we have 3 tasks and 3 different uses:
For OnAttached/hardware init, we only append but don't schedule
For detach, we append, schedule, and then no matter what clear the queue.
For trim, we dont' clear the queue.

So I think there isn't enough common code to matter.

[hush (inactive), 2014-05-01 01:08:04]

lgtm

https://codereview.chromium.org/255023004/diff/120001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/255023004/diff/120001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:139: *
Should avoid setting |waitForCompletion| when |canvas| is not null.
Okay.
Can we at least write a log msg when waitForCompletion is true and canvas is not
null?
Just to see whether we hit it in real life.

On 2014/05/01 00:46:36, boliu wrote:
> On 2014/05/01 00:35:54, hush wrote:
> > what happens if canvas is not null and waitForCompletion is true?
> 
> "Undefined" :p
> 
> I'm not sure. I think it will either deadlock, or waitForCompletion won't do
> anything. Either way it should be avoided for now.

https://codereview.chromium.org/255023004/diff/120001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/255023004/diff/120001/android_webview/native/...
android_webview/native/aw_contents.cc:741:
shared_renderer_state_.SetHardwareInitialized(true);
okay
On 2014/05/01 00:46:36, boliu wrote:
> On 2014/05/01 00:35:54, hush wrote:
> > why do we need this?
> > We are resetting hardware_render_ every time we call SetHardwareInitialized.
> > hardware_renderer_ != NULL is equivalent to
> > shared_renderer_state_.IsHardwareInitialized() returning true?
> 
> Can't query hardware_renderer_ on UI thread since it lives on RT.

https://codereview.chromium.org/255023004/diff/120001/android_webview/native/...
android_webview/native/aw_contents.cc:750: bool draw_functor_succeeded =
RequestDrawGL(NULL, true);
alright
On 2014/05/01 00:46:36, boliu wrote:
> On 2014/05/01 00:35:54, hush wrote:
> > these 3 lines seem like a good candidate for a member function on
> > shared_renderer_state.
> > Basically you are trying to post a "closure" to the render thread, from the
UI
> > thread, and block the UI thread until the render thread is done running it.
> > 
> > Maybe put it into something like
> > bool PostTaskToRenderThreadForResult(Closure&)
> 
> I thought about it, but we have 3 tasks and 3 different uses:
> For OnAttached/hardware init, we only append but don't schedule
> For detach, we append, schedule, and then no matter what clear the queue.
> For trim, we dont' clear the queue.
> 
> So I think there isn't enough common code to matter.

[sgurun-gerrit only, 2014-05-01 01:12:08]

On 2014/05/01 01:08:04, hush wrote:
> lgtm
> 
>
https://codereview.chromium.org/255023004/diff/120001/android_webview/java/sr...
> File android_webview/java/src/org/chromium/android_webview/AwContents.java
> (right):
> 
>
https://codereview.chromium.org/255023004/diff/120001/android_webview/java/sr...
> android_webview/java/src/org/chromium/android_webview/AwContents.java:139: *
> Should avoid setting |waitForCompletion| when |canvas| is not null.
> Okay.
> Can we at least write a log msg when waitForCompletion is true and canvas is
not
> null?
> Just to see whether we hit it in real life.
> 
> On 2014/05/01 00:46:36, boliu wrote:
> > On 2014/05/01 00:35:54, hush wrote:
> > > what happens if canvas is not null and waitForCompletion is true?
> > 
> > "Undefined" :p
> > 
> > I'm not sure. I think it will either deadlock, or waitForCompletion won't do
> > anything. Either way it should be avoided for now.
> 
>
https://codereview.chromium.org/255023004/diff/120001/android_webview/native/...
> File android_webview/native/aw_contents.cc (right):
> 
>
https://codereview.chromium.org/255023004/diff/120001/android_webview/native/...
> android_webview/native/aw_contents.cc:741:
> shared_renderer_state_.SetHardwareInitialized(true);
> okay
> On 2014/05/01 00:46:36, boliu wrote:
> > On 2014/05/01 00:35:54, hush wrote:
> > > why do we need this?
> > > We are resetting hardware_render_ every time we call
SetHardwareInitialized.
> > > hardware_renderer_ != NULL is equivalent to
> > > shared_renderer_state_.IsHardwareInitialized() returning true?
> > 
> > Can't query hardware_renderer_ on UI thread since it lives on RT.
> 
>
https://codereview.chromium.org/255023004/diff/120001/android_webview/native/...
> android_webview/native/aw_contents.cc:750: bool draw_functor_succeeded =
> RequestDrawGL(NULL, true);
> alright
> On 2014/05/01 00:46:36, boliu wrote:
> > On 2014/05/01 00:35:54, hush wrote:
> > > these 3 lines seem like a good candidate for a member function on
> > > shared_renderer_state.
> > > Basically you are trying to post a "closure" to the render thread, from
the
> UI
> > > thread, and block the UI thread until the render thread is done running
it.
> > > 
> > > Maybe put it into something like
> > > bool PostTaskToRenderThreadForResult(Closure&)
> > 
> > I thought about it, but we have 3 tasks and 3 different uses:
> > For OnAttached/hardware init, we only append but don't schedule
> > For detach, we append, schedule, and then no matter what clear the queue.
> > For trim, we dont' clear the queue.
> > 
> > So I think there isn't enough common code to matter.

aw lgtm rubberstamp

[boliu, 2014-05-01 01:35:48]

https://codereview.chromium.org/255023004/diff/120001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/255023004/diff/120001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:139: *
Should avoid setting |waitForCompletion| when |canvas| is not null.
On 2014/05/01 01:08:04, hush wrote:
> Okay.
> Can we at least write a log msg when waitForCompletion is true and canvas is
not
> null?
> Just to see whether we hit it in real life.
> 
> On 2014/05/01 00:46:36, boliu wrote:
> > On 2014/05/01 00:35:54, hush wrote:
> > > what happens if canvas is not null and waitForCompletion is true?
> > 
> > "Undefined" :p
> > 
> > I'm not sure. I think it will either deadlock, or waitForCompletion won't do
> > anything. Either way it should be avoided for now.
> 

Added a DCHECK

[boliu, 2014-05-01 01:35:56]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-05-01 01:36:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/255023004/160001

[commit-bot: I haz the power, 2014-05-01 08:29:40]

Change committed as 267484