aw: Split hardware rendering into HardwareRenderer

aw: Split hardware rendering into HardwareRenderer

The split is currently not perfect, but idea is all compositor
interactions dealing with hardware mode is moved to HardwareRenderer.
Should be a no-op change in general.

Also removed the BrowserViewRenderer <- InProcessViewRenderer
inheritance since it is not needed anymore.

Removed other random cruft too.

BUG=344087
NOTRY=true

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

[boliu, 2014-02-24 03:03:06]

Hmm, need to get someone in mtv to review these changes...

[boliu, 2014-02-24 16:39:01]

Selim, want to review this (and all upcoming changes in this area)?

[sgurun-gerrit only, 2014-02-25 06:56:36]

did a pass. Since I am not familiar with most concepts here, I am likely missing
important details, so leaving them to Ben :)

https://codereview.chromium.org/176543004/diff/220001/android_webview/android...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/176543004/diff/220001/android_webview/android...
android_webview/android_webview.gyp:157: 'browser/hardware_renderer.h',
move hardware_renderer below g (g<h)

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:66:
client_->UpdateGlobalVisibleRect();
seems like we now call updateglobalvisiblerect always. before we would call it
if the level is smaller than TRIM_MEMORY_BACKGROUND. intentional?

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:81: 
nit:stale line?

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:84: // comositor yet or if
RequestDrawGL fails, it means we failed this draw and
nit: compositor

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:28: class SkCanvas;
remove. already forward declared in line 17 above

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:107: // return value indicates
nothing was or will be drawn.
nit: combine with line above.

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:119: // AwDrawGLInfo
documentation for more details of the contract.
nit: continue from line above

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

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/hardware_renderer.cc:73: void
HardwareRenderer::ReleaseHardwareDraw() {
consider moving this code to destructor - not much benefit of having separate
function.

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/hardware_renderer.cc:84: 
Original code does a RunTasks() here. not needed anymore?

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/hardware_renderer.cc:278: }
slightly confusing. It is probably better to have a RequestProcess function
which would call RequestProcessGLOnUiThread rather then recurse.

[benm (inactive), 2014-02-25 12:48:01]

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:70: ForceFakeCompositeSW();
out of interest, why do we do this? maybe worth a comment

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:269:
DCHECK(!hardware_renderer_.get());
does this replace the LOG_IF?

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:36: // Request DrawGL be called.
Passing null canvas implies the request
nit: |canvas|

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:45: // Called to trigger view
invalidations.
nit: "trigger a view invalidation"?

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:60: // Is a WebView-managed
fling in progress?
s/WebView/Android View system/ ?

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:85: virtual bool
RenderViaAuxilaryBitmapIfNeeded(
docs?

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

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/hardware_renderer.cc:27: size_t g_memory_multiplier =
10;
can be overridden with switches::kTileMemoryMultiplier between 2 and 50

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/hardware_renderer.cc:232: int_value <= 500) {
nit:consider making the magic numbers consts? here and above

https://codereview.chromium.org/176543004/diff/220001/android_webview/native/...
File android_webview/native/java_browser_view_renderer_helper.h (right):

https://codereview.chromium.org/176543004/diff/220001/android_webview/native/...
android_webview/native/java_browser_view_renderer_helper.h:17: class
JavaBrowserViewRendererHelper : public BrowserViewRendererJavaHelper {
naming is a little crazy here imo. is now the time to fix the todo on the line
above?

[boliu, 2014-02-25 19:02:22]

Finally someone else looking at this code :)

All un-replied comments are done.

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:66:
client_->UpdateGlobalVisibleRect();
On 2014/02/25 06:56:36, sgurun wrote:
> seems like we now call updateglobalvisiblerect always. before we would call it
> if the level is smaller than TRIM_MEMORY_BACKGROUND. intentional?

Yes for now. I'm told by frameworks folks that getGlobalVisibleRect is cheap.
Kind of unfortunate the current split between BVR and HR right now. HR has no
business directly calling view methods, but then memory management is in HR.

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:70: ForceFakeCompositeSW();
On 2014/02/25 12:48:01, benm wrote:
> out of interest, why do we do this? maybe worth a comment

Setting memory policy itself does nothing, compositor needs to call ManageTiles.
And if a webview is in the background or whatever, it will never happen, so we
force it to drop here.

This is a hack...

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:269:
DCHECK(!hardware_renderer_.get());
On 2014/02/25 12:48:01, benm wrote:
> does this replace the LOG_IF?

Yes. We already delay destroy until after onDetachedFromWindow (logic in
AwContents.java). The LOG_IF was originally added before that logic.

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

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/hardware_renderer.cc:84: 
On 2014/02/25 06:56:36, sgurun wrote:
> Original code does a RunTasks() here. not needed anymore?

Moved RunTasks to constructor of ScopedAllowGL.

https://codereview.chromium.org/176543004/diff/220001/android_webview/browser...
android_webview/browser/hardware_renderer.cc:232: int_value <= 500) {
On 2014/02/25 12:48:01, benm wrote:
> nit:consider making the magic numbers consts? here and above

Screw this. Even I'm not using these anymore. Removed these two switches.

https://codereview.chromium.org/176543004/diff/220001/android_webview/native/...
File android_webview/native/java_browser_view_renderer_helper.h (right):

https://codereview.chromium.org/176543004/diff/220001/android_webview/native/...
android_webview/native/java_browser_view_renderer_helper.h:17: class
JavaBrowserViewRendererHelper : public BrowserViewRendererJavaHelper {
On 2014/02/25 12:48:01, benm wrote:
> naming is a little crazy here imo. is now the time to fix the todo on the line
> above?

Didn't do this yet. Kind of big-ish change, so better to split it out this
already big-ish change.

[sgurun-gerrit only, 2014-02-25 20:08:45]

lgtm

[boliu, 2014-02-25 20:13:55]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-02-25 20:16:06]

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

[commit-bot: I haz the power, 2014-02-25 20:31:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-25 20:31:24]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[boliu, 2014-02-25 20:33:02]

+ted for content/public stamp

[Ted C, 2014-02-25 21:22:07]

On 2014/02/25 20:33:02, boliu wrote:
> +ted for content/public stamp

lgtm - content/public/browser/android/synchronous_compositor_client.h

[boliu, 2014-02-25 21:25:17]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-02-25 21:28:10]

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

[commit-bot: I haz the power, 2014-02-25 22:01:07]

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

[commit-bot: I haz the power, 2014-02-25 22:28:53]

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

[commit-bot: I haz the power, 2014-02-25 23:35:53]

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

[commit-bot: I haz the power, 2014-02-26 00:20:24]

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

[boliu, 2014-02-26 00:25:46]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2014-02-26 00:36:40]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-02-26 00:46:52]

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

[commit-bot: I haz the power, 2014-02-26 01:01:59]

Change committed as 253315