[Android] Implement WebSettings.{get|set}Allow{Content|File}Access.

[Android] Implement WebSettings.{get|set}Allow{Content|File}Access.

This is as much as we can put upstream now.

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

[joth, 2012-08-16 18:52:45]

https://chromiumcodereview.appspot.com/10827380/diff/1/base/android/javatests...
File
base/android/javatests/src/org/chromium/base/test/TestContentProviderClient.java
(right):

https://chromiumcodereview.appspot.com/10827380/diff/1/base/android/javatests...
base/android/javatests/src/org/chromium/base/test/TestContentProviderClient.java:11:
import android.content.Context;
this test can't live in 'base' if it imports from all these other places. (And,
content provider is a webview concept, not part of base/ anyway)
Did you mean to include this file at all?

https://chromiumcodereview.appspot.com/10827380/diff/1/base/android/javatests...
File base/android/javatests/src/org/chromium/base/test/TestFileUtil.java
(right):

https://chromiumcodereview.appspot.com/10827380/diff/1/base/android/javatests...
base/android/javatests/src/org/chromium/base/test/TestFileUtil.java:24: return
false;
I'm surprised if test code really cares to handle both IOException and the
return == false for file not existing. I'd put at most one or other in the API.

https://chromiumcodereview.appspot.com/10827380/diff/1/base/android/javatests...
base/android/javatests/src/org/chromium/base/test/TestFileUtil.java:33:
"</body></html>").getBytes());
it's a bit random having base/ know how to make html. granted it's only a test
util, but I could easily see this ballooning with createHtmlWithOneImageLink..
etc etc.

https://chromiumcodereview.appspot.com/10827380/diff/1/base/android/javatests...
base/android/javatests/src/org/chromium/base/test/TestFileUtil.java:41: 
these functions are not used in this CL. Did you mean to add them.

https://chromiumcodereview.appspot.com/10827380/diff/1/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://chromiumcodereview.appspot.com/10827380/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:49:
// be modified through the ContentSettings instance.
completely off topic, but a thought: would it make sense to have this guy
implement 2 interfaces, ContentSettingsReader and ContentSettingWriter, and then
we can structure code to only pass around the interfaces that make sense for the
main part? e.g. chrome code could exclusively use ContentSettingsReader
interface.

[mnaganov (inactive), 2012-08-17 12:46:02]

Thanks, Jonathan!

Have you seen the full changelist (downstream) 23680? I added you as a reviewer
for it.

https://chromiumcodereview.appspot.com/10827380/diff/1/base/android/javatests...
File
base/android/javatests/src/org/chromium/base/test/TestContentProviderClient.java
(right):

https://chromiumcodereview.appspot.com/10827380/diff/1/base/android/javatests...
base/android/javatests/src/org/chromium/base/test/TestContentProviderClient.java:11:
import android.content.Context;
On 2012/08/16 18:52:45, joth wrote:
> this test can't live in 'base' if it imports from all these other places.
(And,
> content provider is a webview concept, not part of base/ anyway)
> Did you mean to include this file at all?

Content provider is an Android concept. We have tests both in content and in
chrome for testing interaction with Android content providers. I was following
Karen's guideline for putting "generic utilities" into org.chromium.base.test.
She didn't mention that utilities can't depend on Android libs. Thanks for
clarifying this!

I've moved the file into org.chromium.content.browser.test.utils, and as the
whole dir isn't upstreamed yet, the file will be excluded from this CL.

https://chromiumcodereview.appspot.com/10827380/diff/1/base/android/javatests...
File base/android/javatests/src/org/chromium/base/test/TestFileUtil.java
(right):

https://chromiumcodereview.appspot.com/10827380/diff/1/base/android/javatests...
base/android/javatests/src/org/chromium/base/test/TestFileUtil.java:24: return
false;
On 2012/08/16 18:52:45, joth wrote:
> I'm surprised if test code really cares to handle both IOException and the
> return == false for file not existing. I'd put at most one or other in the
API.

Yep, that was brain dead, fixed.

https://chromiumcodereview.appspot.com/10827380/diff/1/base/android/javatests...
base/android/javatests/src/org/chromium/base/test/TestFileUtil.java:33:
"</body></html>").getBytes());
On 2012/08/16 18:52:45, joth wrote:
> it's a bit random having base/ know how to make html. granted it's only a test
> util, but I could easily see this ballooning with createHtmlWithOneImageLink..
> etc etc.

These functions are used both in content and chrome tests (both downstream),
and, again, following Karin's guideline, they are quite "generic" (doesn't
depend on content or chrome) to live here. My intent is to avoid code
duplication. I would be glad to put these functions in a more appropriate place
if you suggest one.

https://chromiumcodereview.appspot.com/10827380/diff/1/base/android/javatests...
base/android/javatests/src/org/chromium/base/test/TestFileUtil.java:41: 
On 2012/08/16 18:52:45, joth wrote:
> these functions are not used in this CL. Did you mean to add them.

Please see the previous comment.

https://chromiumcodereview.appspot.com/10827380/diff/1/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://chromiumcodereview.appspot.com/10827380/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:49:
// be modified through the ContentSettings instance.
On 2012/08/16 18:52:45, joth wrote:
> completely off topic, but a thought: would it make sense to have this guy
> implement 2 interfaces, ContentSettingsReader and ContentSettingWriter, and
then
> we can structure code to only pass around the interfaces that make sense for
the
> main part? e.g. chrome code could exclusively use ContentSettingsReader
> interface.

A good idea. This way, we can get rid of assertions in setters that check for
the current mode.

[joth, 2012-08-17 16:57:34]

lgtm

https://chromiumcodereview.appspot.com/10827380/diff/1/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://chromiumcodereview.appspot.com/10827380/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:49:
// be modified through the ContentSettings instance.
On 2012/08/17 12:46:02, Mikhail Naganov (Chromium) wrote:
> On 2012/08/16 18:52:45, joth wrote:
> > completely off topic, but a thought: would it make sense to have this guy
> > implement 2 interfaces, ContentSettingsReader and ContentSettingWriter, and
> then
> > we can structure code to only pass around the interfaces that make sense for
> the
> > main part? e.g. chrome code could exclusively use ContentSettingsReader
> > interface.
> 
> A good idea. This way, we can get rid of assertions in setters that check for
> the current mode.

true, although we'd need to see it right through and make the concrete class
private? that way the only way to get the writer interface would be via some
getWriter() and that would be the one-place we'd still assert it's not a
read-only settings instance.

[mnaganov (inactive), 2012-08-20 08:46:41]

Satish, it seems I need your LGTM. This is what the script says:

Missing LGTM from an OWNER for files in these directories:
    content/public/android

[Satish, 2012-08-20 10:16:57]

https://chromiumcodereview.appspot.com/10827380/diff/10001/base/android/javat...
File base/android/javatests/src/org/chromium/base/test/TestFileUtil.java
(right):

https://chromiumcodereview.appspot.com/10827380/diff/10001/base/android/javat...
base/android/javatests/src/org/chromium/base/test/TestFileUtil.java:33:
"</body></html>").getBytes());
this is going to get the encoded string in the platform's default charset. Is
that what we want? I see the method below  readUtf8File() and wonder if you
wanted to write a utf8 encoded file or unicode etc.

https://chromiumcodereview.appspot.com/10827380/diff/10001/content/browser/an...
File content/browser/android/content_settings.cc (right):

https://chromiumcodereview.appspot.com/10827380/diff/10001/content/browser/an...
content/browser/android/content_settings.cc:80: GetFieldID(env, clazz,
"mAllowContentAccess", "Z");
Is this for 'content uri' access? If so, could the variable names in java and
c++ reflect that instead of just 'content' ?

[mnaganov (inactive), 2012-08-20 13:32:55]

https://chromiumcodereview.appspot.com/10827380/diff/10001/base/android/javat...
File base/android/javatests/src/org/chromium/base/test/TestFileUtil.java
(right):

https://chromiumcodereview.appspot.com/10827380/diff/10001/base/android/javat...
base/android/javatests/src/org/chromium/base/test/TestFileUtil.java:33:
"</body></html>").getBytes());
On 2012/08/20 10:16:57, Satish wrote:
> this is going to get the encoded string in the platform's default charset. Is
> that what we want? I see the method below  readUtf8File() and wonder if you
> wanted to write a utf8 encoded file or unicode etc.

So far, tests were using Latin-1, but I see your point. Changed to write in
UTF-8.

https://chromiumcodereview.appspot.com/10827380/diff/10001/content/browser/an...
File content/browser/android/content_settings.cc (right):

https://chromiumcodereview.appspot.com/10827380/diff/10001/content/browser/an...
content/browser/android/content_settings.cc:80: GetFieldID(env, clazz,
"mAllowContentAccess", "Z");
On 2012/08/20 10:16:57, Satish wrote:
> Is this for 'content uri' access? If so, could the variable names in java and
> c++ reflect that instead of just 'content' ?

Done.

[Satish, 2012-08-20 13:37:41]

lgtm

https://chromiumcodereview.appspot.com/10827380/diff/12003/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://chromiumcodereview.appspot.com/10827380/diff/12003/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:85:
private boolean mAllowFileAccess = true;
it isn't clear from the code below if this is for file:// urls or just accessing
files elsewhere in contentview. If it is the former, suggest renaming this to
use FileUrl like you did with ContentUrl

[mnaganov (inactive), 2012-08-20 14:23:05]

https://chromiumcodereview.appspot.com/10827380/diff/12003/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://chromiumcodereview.appspot.com/10827380/diff/12003/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:85:
private boolean mAllowFileAccess = true;
On 2012/08/20 13:37:41, Satish wrote:
> it isn't clear from the code below if this is for file:// urls or just
accessing
> files elsewhere in contentview. If it is the former, suggest renaming this to
> use FileUrl like you did with ContentUrl

Sure. I'm sorry I didn't think about this myself. Fixed.