Hooking up setBackgroundColor from AwContents to render process

Hooking up setBackgroundColor from AwContents to renderer

Final call into WebKit API ommitted as it depends on blink change https://codereview.chromium.org/19883002/

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

[joth, 2013-07-20 06:22:50]

The blink side of this still needs some style fixup, but this is mostly there
now. Could use some guidance on how to trigger invalidation after background
color has changed.

https://codereview.chromium.org/19883002/

[joth, 2013-07-22 04:55:32]

+mkosiba for Aw java / test bits

[benm (inactive), 2013-07-22 12:42:06]

Martin's out today (back tomorrow I think), but looks reasonable to me! :)

https://codereview.chromium.org/19693016/diff/11002/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/19693016/diff/11002/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:100:
mCachedRendererBackgroundColor = color == 0 ? 1 : color;
Maybe make this clearer by adding a static final int INVALID_COLOR = 0?

Then no need for the comments.

[joth, 2013-07-23 02:22:52]

https://codereview.chromium.org/19693016/diff/11002/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/19693016/diff/11002/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:100:
mCachedRendererBackgroundColor = color == 0 ? 1 : color;
On 2013/07/22 12:42:06, benm wrote:
> Maybe make this clearer by adding a static final int INVALID_COLOR = 0?
> 
> Then no need for the comments.

Done.

[mkosiba (inactive), 2013-07-23 11:14:40]

lgtm

https://codereview.chromium.org/19693016/diff/15001/android_webview/browser/r...
File android_webview/browser/renderer_host/aw_render_view_host_ext.cc (right):

https://codereview.chromium.org/19693016/diff/15001/android_webview/browser/r...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:89: void
AwRenderViewHostExt::SetBackgroundColor(SkColor c) {
why not put it in AwSettings? There's already logic in there to sync the setting
on RenderViewCreated and this code seems to be replicating that.

https://codereview.chromium.org/19693016/diff/15001/android_webview/javatests...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsRenderTest.java
(right):

https://codereview.chromium.org/19693016/diff/15001/android_webview/javatests...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsRenderTest.java:48:
void setBackgroundColorSync(final int c) {
uber-nit: this is more of a setBackgroundColorOnUiThread(). It's not really Sync
since you're not waiting for the change to take effect in the renderer thread.

[joth, 2013-07-23 17:38:58]

Thanks!

https://codereview.chromium.org/19693016/diff/15001/android_webview/browser/r...
File android_webview/browser/renderer_host/aw_render_view_host_ext.cc (right):

https://codereview.chromium.org/19693016/diff/15001/android_webview/browser/r...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:89: void
AwRenderViewHostExt::SetBackgroundColor(SkColor c) {
On 2013/07/23 11:14:40, mkosiba wrote:
> why not put it in AwSettings? There's already logic in there to sync the
setting
> on RenderViewCreated and this code seems to be replicating that.

I feel AwSettings is the upstream (native) implementation of WebSettings. It's a
peer of AwContents - both handle the JNI boundary.
Internally AwSettings delegates IPC to AwRenderViewHostExt anyway, so delegating
here to AwSettings is just putting another player in the path for no benefit
AFAICT.
Also, the 'sync the settings' code in AwSettings is heavily focused on 'pulling'
values from java to native in a thread safe way. background color doesn't need
any of that (it's pushed from java, and single threaded), so I think it would be
confusing matters to route it via that path.

It an interesting point that AwSettings uses RenderViewCreated rather than
RenderViewReady. RVR seems to work, but the docs say "This method is invoked
when the RenderView of the current RenderViewHost is ready, e.g. because we
recreated it after a crash." which is kind of ambiguous. I think I'll switch to
use RVC to be consistent with AwSettings.

https://codereview.chromium.org/19693016/diff/15001/android_webview/javatests...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsRenderTest.java
(right):

https://codereview.chromium.org/19693016/diff/15001/android_webview/javatests...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsRenderTest.java:48:
void setBackgroundColorSync(final int c) {
On 2013/07/23 11:14:40, mkosiba wrote:
> uber-nit: this is more of a setBackgroundColorOnUiThread(). It's not really
Sync
> since you're not waiting for the change to take effect in the renderer thread.

Done.

[joth, 2013-07-23 17:40:58]

+palmer for android_webview/common/render_view_messages.h rubberstamp

[joth, 2013-07-23 19:01:50]

+cdn for android_webview/common/render_view_messages.h

[Cris Neckar, 2013-07-23 19:03:42]

IPC changes lgtm

[commit-bot: I haz the power, 2013-07-23 20:03:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/19693016/30001

[commit-bot: I haz the power, 2013-07-23 21:02:20]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-23 21:20:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/19693016/30001

[commit-bot: I haz the power, 2013-07-23 23:03:45]

Change committed as 213255