[Android WebView] Move the most of WebSettings into AwSettings

[Android WebView] Move the most of WebSettings into AwSettings

Historically, WebSettings management is used to be in the ContentSettings class,
while only WebView actually needs it. Chrome for Android is only interested in
the value of the "JavaScript enabled" setting.

This change leaves zoom-related settings intact, as moving them will require
decoupling of the ZoomManager class from the ContentView* family of classes,
which deserves a separate change.

BUG=b/8296421

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

[mnaganov (inactive), 2013-04-24 11:53:11]

Ben, PTAL!

The change is quite big but is almost trivial. I have currently left
zoom-related properties in ContentSettings, because they are used by ZoomManager
which is coupled with the ContentView* classes family, so moving them deserves a
patch on its own.

[joth, 2013-04-24 17:07:43]

could quick drive-bys on content/

https://codereview.chromium.org/14271024/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://codereview.chromium.org/14271024/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:237:
return mJavaScriptEnabled;
maybe for another patch, but, I don't think the "sync from native" machinery in
here is in anyway needed if this is the only setting it syncs. We could just
make a JNI call over on an as-needed basis.
(Small question of whether this needs to be callable on any thread though....
maybe that means we do need all this clutter?)

https://codereview.chromium.org/14271024/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(left):

https://codereview.chromium.org/14271024/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:566:
if (mPersonality == PERSONALITY_VIEW) {
can we get rid of the PERSONALITY thing from content view altogether now?

[mnaganov (inactive), 2013-04-24 17:41:37]

https://codereview.chromium.org/14271024/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://codereview.chromium.org/14271024/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:237:
return mJavaScriptEnabled;
On 2013/04/24 17:07:44, joth wrote:
> maybe for another patch, but, I don't think the "sync from native" machinery
in
> here is in anyway needed if this is the only setting it syncs. We could just
> make a JNI call over on an as-needed basis.
> (Small question of whether this needs to be callable on any thread though....
> maybe that means we do need all this clutter?)

Agreed, but definitely this patch isn't the last. Let's move in baby steps.

https://codereview.chromium.org/14271024/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(left):

https://codereview.chromium.org/14271024/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:566:
if (mPersonality == PERSONALITY_VIEW) {
On 2013/04/24 17:07:44, joth wrote:
> can we get rid of the PERSONALITY thing from content view altogether now?

I've found this fragment in initializeContainerView:

if (mPersonality == PERSONALITY_CHROME) {
            // Doing this in PERSONALITY_VIEW mode causes rendering problems in
our
            // current WebView test case (the HTMLViewer application).
            // TODO(benm): Figure out why this is the case.
            if (mContainerView.getScrollBarStyle() ==
View.SCROLLBARS_INSIDE_OVERLAY) {
                mContainerView.setHorizontalScrollBarEnabled(false);
                mContainerView.setVerticalScrollBarEnabled(false);
            }
        }

I guess, this is the only obstacle to getting rid of it.

[benm (inactive), 2013-04-24 18:02:15]

https://codereview.chromium.org/14271024/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(left):

https://codereview.chromium.org/14271024/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:566:
if (mPersonality == PERSONALITY_VIEW) {
Ah, I remember that one, never did get to the bottom of that weirdness. It was a
very long time ago though so would be worth just removing and seeing what
happens.

[joth, 2013-04-24 20:05:45]

On 24 April 2013 11:02, <benm@chromium.org> wrote:

>
> https://codereview.chromium.**org/14271024/diff/3001/**
> content/public/android/java/**src/org/chromium/content/**
>
browser/ContentViewCore.java<https://codereview.chromium.org/14271024/diff/3001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java>
> File
> content/public/android/java/**src/org/chromium/content/**
> browser/ContentViewCore.java
> (left):
>
> https://codereview.chromium.**org/14271024/diff/3001/**
> content/public/android/java/**src/org/chromium/content/**
>
browser/ContentViewCore.java#**oldcode566<https://codereview.chromium.org/14271024/diff/3001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#oldcode566>
> content/public/android/java/**src/org/chromium/content/**
> browser/ContentViewCore.java:**566:
> if (mPersonality == PERSONALITY_VIEW) {
> Ah, I remember that one, never did get to the bottom of that weirdness.
> It was a very long time ago though so would be worth just removing and
> seeing what happens.
>
>
Also, it should now be trivial to move those two calls (with the TODO) into
AwContents instead. Back when that code was written I don't think we had
the aw/ layer at all.

[joth, 2013-04-24 20:06:43]

PS I'm fine with removing PERSONALITY in a separate baby-step too.


On 24 April 2013 13:05, Jonathan Dixon <joth@chromium.org> wrote:

>
>
>
> On 24 April 2013 11:02, <benm@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/14271024/diff/3001/**
>> content/public/android/java/**src/org/chromium/content/**
>>
browser/ContentViewCore.java<https://codereview.chromium.org/14271024/diff/3001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java>
>> File
>> content/public/android/java/**src/org/chromium/content/**
>> browser/ContentViewCore.java
>> (left):
>>
>> https://codereview.chromium.**org/14271024/diff/3001/**
>> content/public/android/java/**src/org/chromium/content/**
>>
browser/ContentViewCore.java#**oldcode566<https://codereview.chromium.org/14271024/diff/3001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#oldcode566>
>> content/public/android/java/**src/org/chromium/content/**
>> browser/ContentViewCore.java:**566:
>> if (mPersonality == PERSONALITY_VIEW) {
>> Ah, I remember that one, never did get to the bottom of that weirdness.
>> It was a very long time ago though so would be worth just removing and
>> seeing what happens.
>>
>>
> Also, it should now be trivial to move those two calls (with the TODO)
> into AwContents instead. Back when that code was written I don't think we
> had the aw/ layer at all.
>
>
>

[benm (inactive), 2013-04-25 11:46:31]

lgtm

Maybe update the CL description, the sentence "Chrome for Android is only
interested in
the value of the "JavaScript" enabled setting" confused me a bit as
ContentSettings still manages zoom settings as well as JS, and AwSettings
delegates to ContentSettings for zoom. But I think you mean that the *native*
side of Chrome for Android is only interested in the JS flag.

[mnaganov (inactive), 2013-04-25 12:25:13]

On 2013/04/25 11:46:31, benm wrote:
> lgtm
> 
> Maybe update the CL description, the sentence "Chrome for Android is only
> interested in
> the value of the "JavaScript" enabled setting" confused me a bit as
> ContentSettings still manages zoom settings as well as JS, and AwSettings
> delegates to ContentSettings for zoom. But I think you mean that the *native*
> side of Chrome for Android is only interested in the JS flag.

Thanks! I updated the description.

In fact, Chrome is not really interested in zoom settings values, as they cannot
change (unlike the "JS enabled" flag which is controllable from Chrome's
settings.) Initially, I was planning to factor zoom-related settings out, but
encountered difficulties, which deserve a separate change.

[mnaganov (inactive), 2013-04-25 12:28:16]

Marcus, can you please make an OWNER's review for content/browser/android and
content/public/android? It is only code deletions, so the review should be easy
:)

[bulach, 2013-04-25 12:55:57]

lgtm, but I have a radically different suggestion :)

since you're just moving code around, it's perfectly fine to do as a follow up.

https://codereview.chromium.org/14271024/diff/3001/android_webview/native/aw_...
File android_webview/native/aw_settings.cc (right):

https://codereview.chromium.org/14271024/diff/3001/android_webview/native/aw_...
android_webview/native/aw_settings.cc:43: // FIXME: we should be using a new
GetFieldIDFromClassName() with caching.
given the "note on speed" above, would it be possible to just use regular,
auto-generated getters / setters? :)

this struct will be gone.. the usage will be much clearer and safer below, etc,
etc...

hey, here's a thought! we have that "template java generation"... it's mostly
used for enums, but I bet we could do that for something like:

AwSettingsStruct.java.template:

#define EXPAND_FIELD(Type, Name) \
private Type m##Name; \
@CalledByNative \
private Type get##Name() { return m##Name; }
@CalledByNative \
private void set##Name(Type val) { m##Name = value; }


EXPAND_FIELD(String, TextSizePercent)
EXPAND_FIELD(String, StandardFontFamily)
...
EXPAND_FIELD(int, InitialPageScalePercent)
...


if this pattern work out, it'd be a very nice addition for "struct like passing
between java-c++"!!
truly no duplication whatsoever... :)
callers in java can just use AwSettingsStruct directly.
callers in c++ can use Java_AwSettingsStruct_setFooBar / _getFooBar...

wdyt?

https://codereview.chromium.org/14271024/diff/3001/content/browser/android/co...
File content/browser/android/content_settings.cc (right):

https://codereview.chromium.org/14271024/diff/3001/content/browser/android/co...
content/browser/android/content_settings.cc:26: // FIXME: we should be using a
new GetFieldIDFromClassName() with caching.
ditto..

[mnaganov (inactive), 2013-04-25 13:15:16]

On 2013/04/25 12:55:57, bulach wrote:

Thanks, Marcus! I'll consider your proposal!

> lgtm, but I have a radically different suggestion :)
> 
> since you're just moving code around, it's perfectly fine to do as a follow
up.
> 
>
https://codereview.chromium.org/14271024/diff/3001/android_webview/native/aw_...
> File android_webview/native/aw_settings.cc (right):
> 
>
https://codereview.chromium.org/14271024/diff/3001/android_webview/native/aw_...
> android_webview/native/aw_settings.cc:43: // FIXME: we should be using a new
> GetFieldIDFromClassName() with caching.
> given the "note on speed" above, would it be possible to just use regular,
> auto-generated getters / setters? :)
> 
> this struct will be gone.. the usage will be much clearer and safer below,
etc,
> etc...
> 
> hey, here's a thought! we have that "template java generation"... it's mostly
> used for enums, but I bet we could do that for something like:
> 
> AwSettingsStruct.java.template:
> 
> #define EXPAND_FIELD(Type, Name) \
> private Type m##Name; \
> @CalledByNative \
> private Type get##Name() { return m##Name; }
> @CalledByNative \
> private void set##Name(Type val) { m##Name = value; }
> 
> 
> EXPAND_FIELD(String, TextSizePercent)
> EXPAND_FIELD(String, StandardFontFamily)
> ...
> EXPAND_FIELD(int, InitialPageScalePercent)
> ...
> 
> 
> if this pattern work out, it'd be a very nice addition for "struct like
passing
> between java-c++"!!
> truly no duplication whatsoever... :)
> callers in java can just use AwSettingsStruct directly.
> callers in c++ can use Java_AwSettingsStruct_setFooBar / _getFooBar...
> 
> wdyt?
> 
>
https://codereview.chromium.org/14271024/diff/3001/content/browser/android/co...
> File content/browser/android/content_settings.cc (right):
> 
>
https://codereview.chromium.org/14271024/diff/3001/content/browser/android/co...
> content/browser/android/content_settings.cc:26: // FIXME: we should be using a
> new GetFieldIDFromClassName() with caching.
> ditto..

[commit-bot: I haz the power, 2013-04-25 13:15:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/14271024/3001

[commit-bot: I haz the power, 2013-04-25 15:10:06]

Change committed as 196399