[Android WebView] Set WebView font renderer preferences

[Android WebView] Set WebView font renderer preferences

This achieves backwards compat with "Classic" Android WebView which
would always set this level of font hinting and enable sub puxel positioning.

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

[benm (inactive), 2013-08-29 16:27:15]

ptal!

[boliu, 2013-08-29 16:29:59]

https://codereview.chromium.org/23461014/diff/1/android_webview/native/aw_con...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/23461014/diff/1/android_webview/native/aw_con...
android_webview/native/aw_contents.cc:214: void
AwContents::SetAndroidWebViewRendererPrefs() {
What calls this and when is it called? Can we guarantee we will have a RVH when
it's called?

[benm (inactive), 2013-08-29 16:31:50]

https://codereview.chromium.org/23461014/diff/1/android_webview/native/aw_con...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/23461014/diff/1/android_webview/native/aw_con...
android_webview/native/aw_contents.cc:214: void
AwContents::SetAndroidWebViewRendererPrefs() {
Oops, cherry pick fail! please see next PS!

On 2013/08/29 16:29:59, boliu wrote:
> What calls this and when is it called? Can we guarantee we will have a RVH
when
> it's called?

[benm (inactive), 2013-08-29 16:36:34]

please take another look :)

[boliu, 2013-08-29 16:48:31]

lgtm

https://codereview.chromium.org/23461014/diff/11001/android_webview/native/aw...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/23461014/diff/11001/android_webview/native/aw...
android_webview/native/aw_contents.cc:222:
web_contents_->GetRenderViewHost()->SyncRendererPrefs();
Hmm, I think we do have a RVH at this point, but not RV, and this just becomes a
no-op in that case? Guess that's ok.

[benm (inactive), 2013-08-29 16:53:36]

thanks bo!

https://codereview.chromium.org/23461014/diff/11001/android_webview/native/aw...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/23461014/diff/11001/android_webview/native/aw...
android_webview/native/aw_contents.cc:222:
web_contents_->GetRenderViewHost()->SyncRendererPrefs();
Yes, that's correct AFAIU. This doesn't crash, anyway :-)

But thinking about it, as this is now a separate function that could be called
from elsewhere, I will add the null check to be safe. Also gonna alphabetise the
prefs, it's bugging me.

[joth, 2013-08-29 18:19:40]

https://codereview.chromium.org/23461014/diff/11001/android_webview/native/aw...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/23461014/diff/11001/android_webview/native/aw...
android_webview/native/aw_contents.cc:222:
web_contents_->GetRenderViewHost()->SyncRendererPrefs();
On 2013/08/29 16:53:36, benm wrote:
> Yes, that's correct AFAIU. This doesn't crash, anyway :-)
> 
> But thinking about it, as this is now a separate function that could be called
> from elsewhere, I will add the null check to be safe. Also gonna alphabetise
the
> prefs, it's bugging me.

Sounds like AwRVHExt OnRVHCreated would be the place to do this then?

[boliu, 2013-08-29 18:26:07]

https://codereview.chromium.org/23461014/diff/11001/android_webview/native/aw...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/23461014/diff/11001/android_webview/native/aw...
android_webview/native/aw_contents.cc:222:
web_contents_->GetRenderViewHost()->SyncRendererPrefs();
On 2013/08/29 18:19:40, joth wrote:
> Sounds like AwRVHExt OnRVHCreated would be the place to do this then?

Not really. When render view is created, the existing preferences is passed rv
already, so no need to call sync there. It's only after render view is created
and we want to change the preference again that we *need* to call sync.

[benm (inactive), 2013-08-30 13:47:43]

https://codereview.chromium.org/23461014/diff/11001/android_webview/native/aw...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/23461014/diff/11001/android_webview/native/aw...
android_webview/native/aw_contents.cc:222:
web_contents_->GetRenderViewHost()->SyncRendererPrefs();
We could move to AWRHExt::OnRVCreated, - this is actually where I had this
originally, but then noticed Bo's change for tap_targets was done here in
AwContents so moved it over. Any preference?

[boliu, 2013-08-30 15:49:11]

On 2013/08/30 13:47:43, benm wrote:
>
https://codereview.chromium.org/23461014/diff/11001/android_webview/native/aw...
> File android_webview/native/aw_contents.cc (right):
> 
>
https://codereview.chromium.org/23461014/diff/11001/android_webview/native/aw...
> android_webview/native/aw_contents.cc:222:
> web_contents_->GetRenderViewHost()->SyncRendererPrefs();
> We could move to AWRHExt::OnRVCreated, - this is actually where I had this
> originally, but then noticed Bo's change for tap_targets was done here in
> AwContents so moved it over. Any preference?

I have no preference. Saving one ipc is not really worth it. And eventually for
the disambiguation popup, we would probably want to control it dymaically
anyway.

[joth, 2013-08-30 19:07:13]

lgtm

when we have our own //a_w layer state we need to be careful to handcode our own
code to do the sync for each new RVH creation, but for //content layer
properties that sync automatically makes sense to just lean on that to do the
work.

[commit-bot: I haz the power, 2013-08-30 19:53:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benm@chromium.org/23461014/22001

[commit-bot: I haz the power, 2013-08-30 21:50:23]

Change committed as 220668