Implement Site Settings \ Images category.

Implement Site Settings \ Images category.

BUG=425698
Committed: https://crrev.com/6c047d636359cd9136acf509839ac9c2d3fcaf2e
Cr-Commit-Position: refs/heads/master@{#325231}

[Finnur, 2015-04-08 15:29:21]

finnur@chromium.org changed reviewers:
+ newt@chromium.org

[Finnur, 2015-04-08 15:29:21]

As per mocks...

[newt (away), 2015-04-09 23:43:20]

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
(right):

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:673:
public boolean imagesEnabled() {
"areImagesEnabled()" and "areImagesManaged()". also let's use the same name for
the native method as for the Java method. (I know you're following the pattern
in this class, but that pattern should be changed)

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:685:
* Sets the preferences on whether to enable/disable images.
Sets whether webpages are allowed to load images.

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:749:
* Sets whether Images are allowed to show on the given website/domain.
s/Images/images

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:770:
* get all the currently saved Images exceptions.
s/get/Returns

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/AddExceptionPreference.java
(right):

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/AddExceptionPreference.java:59:
Context context, String key, int contentSettingsType, SiteAddedCallback
callback) {
Instead of passing in the contentSettingsType, how about just passing in the
message text?

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentPreferences.java
(right):

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentPreferences.java:107:
if (LOCATION_KEY.equals(p.getKey())) {
s/p.getKey()/prefName

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferenceBridge.java
(right):

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferenceBridge.java:166:
public static List<ImagesExceptionInfo> getImagesExceptionInfo() {
wouldn't it be handy if getImagesExceptionInfo(), getJavaScriptExceptionInfo(),
and getPopupExceptionInfo() could all be collapsed into a single a method that
took a ContentSettingType parameter?

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ContentPreferencesTest.java
(right):

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ContentPreferencesTest.java:167:
final WebsitePreferences websitePreferences =
lines 167-170 should happen on the UI thread, i.e. move them inside run()

https://codereview.chromium.org/1068223002/diff/80001/chrome/browser/android/...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/1068223002/diff/80001/chrome/browser/android/...
chrome/browser/android/preferences/pref_service_bridge.cc:477:
CONTENT_SETTINGS_TYPE_IMAGES, NULL) == CONTENT_SETTING_ALLOW;
use nullptr

https://codereview.chromium.org/1068223002/diff/80001/chrome/browser/android/...
chrome/browser/android/preferences/pref_service_bridge.cc:599: static void
SetImagesAllowed(JNIEnv* env, jobject obj, jstring pattern,
The proliferation of these almost-identical methods is... smelly.

ImagesExceptionInfo, JavaScriptExceptionInfo, and PopupExceptionInfo are almost
identical. SetImagesAllowed(), SetJavaScriptAllowed() and SetPopupException()
are almost identical. Same for GetImagesException(), GetJavaScriptException(),
and GetPopupExceptions().

These should all be merged. For the *ExceptionInfo classes, there should be an
extra member variable specifying the content setting type that the exception
applies to (if that's even needed; maybe it's not). For the methods in this
file, they could take an extra parameter that specifies the content setting type
to act on.

I'd like to see similar changes for all the identical Get*Enabled(),
Get*Managed(), and Set*Enabled() methods, but that's a larger change that should
be in a separate CL.

[Finnur, 2015-04-10 17:03:26]

Tackled the cleanup (and other feedback). Haven't had a chance to run end-to-end
testing on the three exception categories, but that's next on my list.

In the meantime, see if this fits the bill.

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
(right):

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:673:
public boolean imagesEnabled() {
On 2015/04/09 23:43:20, newt wrote:
> "areImagesEnabled()" and "areImagesManaged()". also let's use the same name
for
> the native method as for the Java method. (I know you're following the pattern
> in this class, but that pattern should be changed)

Done.

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:685:
* Sets the preferences on whether to enable/disable images.
On 2015/04/09 23:43:20, newt wrote:
> Sets whether webpages are allowed to load images.

Done.

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:749:
* Sets whether Images are allowed to show on the given website/domain.
On 2015/04/09 23:43:20, newt wrote:
> s/Images/images

Done.

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:770:
* get all the currently saved Images exceptions.
On 2015/04/09 23:43:20, newt wrote:
> s/get/Returns

Done.

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/AddExceptionPreference.java
(right):

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/AddExceptionPreference.java:59:
Context context, String key, int contentSettingsType, SiteAddedCallback
callback) {
That's a good idea.

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentPreferences.java
(right):

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentPreferences.java:107:
if (LOCATION_KEY.equals(p.getKey())) {
On 2015/04/09 23:43:20, newt wrote:
> s/p.getKey()/prefName

Done.

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferenceBridge.java
(right):

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferenceBridge.java:166:
public static List<ImagesExceptionInfo> getImagesExceptionInfo() {
Indeed.

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ContentPreferencesTest.java
(right):

https://codereview.chromium.org/1068223002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ContentPreferencesTest.java:167:
final WebsitePreferences websitePreferences =
On 2015/04/09 23:43:20, newt wrote:
> lines 167-170 should happen on the UI thread, i.e. move them inside run()

Done.

https://codereview.chromium.org/1068223002/diff/80001/chrome/browser/android/...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/1068223002/diff/80001/chrome/browser/android/...
chrome/browser/android/preferences/pref_service_bridge.cc:477:
CONTENT_SETTINGS_TYPE_IMAGES, NULL) == CONTENT_SETTING_ALLOW;
On 2015/04/09 23:43:20, newt wrote:
> use nullptr

Done.

https://codereview.chromium.org/1068223002/diff/80001/chrome/browser/android/...
chrome/browser/android/preferences/pref_service_bridge.cc:599: static void
SetImagesAllowed(JNIEnv* env, jobject obj, jstring pattern,
Yeah, I agree. This is ripe for a cleanup and I tackled it in this CL.

[newt (away), 2015-04-10 18:46:50]

Thanks, Finnur. This is great cleanup!

One out-of-line comment: I suggested renaming "ContentSettingExceptionInfo" to
"ContentSettingException". Could you also look for an eradicate all uses of
"info" in related method and variable names and replace them with "exception"?

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
(right):

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:220:
* @return Whether a particular content setting type is enabled.
"Returns whether". don't use @return by itself, and don't use @return if it's
entirely redundant.

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:236:
* Set a default value for content setting type.
Sets

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:240:
public void setContentSettingDefault(int contentSettingsType, boolean enabled) {
I think "setContentSettingEnabled()" would be a clearer name

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:245:
* Return all the currently saved exceptions for a given content settings type.
Returns

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:247:
* @return List of all the exceptions and their settings.
this "@return" is mostly redundant, and I'd probably omit it

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:732:
* Sets the preferences on whether to enable/disable images.
Sets whether webpages are allowed to load images.

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingExceptionInfo.java
(right):

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingExceptionInfo.java:12:
* Images exception information for a given origin.
Update comments not to mention "images"

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingExceptionInfo.java:14:
public class ContentSettingExceptionInfo implements Serializable {
I'd remove "Info" from the name. ContentSettingException is cleaner.

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:396:
public void setImagesInfo(ContentSettingExceptionInfo info) {
I'd call this setImagesException(), similarly for JS and Popups below.

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferenceBridge.java
(right):

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferenceBridge.java:166:
public static List<ContentSettingExceptionInfo> getContentSettingsExceptionInfo(
"getContentSettingsExceptions()"

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java
(right):

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:404:
}
newline

https://codereview.chromium.org/1068223002/diff/100001/chrome/browser/android...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/1068223002/diff/100001/chrome/browser/android...
chrome/browser/android/preferences/pref_service_bridge.cc:165:
DCHECK(content_settings_type == CONTENT_SETTINGS_TYPE_JAVASCRIPT ||
This is a good check to add. Thanks :0

https://codereview.chromium.org/1068223002/diff/100001/chrome/browser/android...
chrome/browser/android/preferences/pref_service_bridge.cc:180: static void
SetContentSettingDefault(JNIEnv* env, jobject obj,
"SetContentSettingEnabled()"

[Finnur, 2015-04-13 12:59:33]

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
(right):

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:220:
* @return Whether a particular content setting type is enabled.
On 2015/04/10 18:46:49, newt wrote:
> "Returns whether". don't use @return by itself, and don't use @return if it's
> entirely redundant.

Done.

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:236:
* Set a default value for content setting type.
On 2015/04/10 18:46:49, newt wrote:
> Sets

Done.

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:240:
public void setContentSettingDefault(int contentSettingsType, boolean enabled) {
On 2015/04/10 18:46:49, newt wrote:
> I think "setContentSettingEnabled()" would be a clearer name

Done.

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:245:
* Return all the currently saved exceptions for a given content settings type.
On 2015/04/10 18:46:49, newt wrote:
> Returns

Done.

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:247:
* @return List of all the exceptions and their settings.
On 2015/04/10 18:46:49, newt wrote:
> this "@return" is mostly redundant, and I'd probably omit it

Done.

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:732:
* Sets the preferences on whether to enable/disable images.
On 2015/04/10 18:46:49, newt wrote:
> Sets whether webpages are allowed to load images.

Done.

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingExceptionInfo.java
(right):

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingExceptionInfo.java:12:
* Images exception information for a given origin.
On 2015/04/10 18:46:49, newt wrote:
> Update comments not to mention "images"

Done.

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingExceptionInfo.java:14:
public class ContentSettingExceptionInfo implements Serializable {
On 2015/04/10 18:46:49, newt wrote:
> I'd remove "Info" from the name. ContentSettingException is cleaner.

I agree. However, I renamed the class and findbug now complains.

NM_CLASS_NOT_EXCEPTION: Class is not derived from an Exception, even though it
is named as such
  In class
org.chromium.chrome.browser.preferences.website.ContentSettingException
  At ContentSettingException.java:[lines 21-66]

That's probably why it was named as it was. What is the right way forward? Add a
suppression?

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:396:
public void setImagesInfo(ContentSettingExceptionInfo info) {
On 2015/04/10 18:46:49, newt wrote:
> I'd call this setImagesException(), similarly for JS and Popups below.

Done.

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferenceBridge.java
(right):

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferenceBridge.java:166:
public static List<ContentSettingExceptionInfo> getContentSettingsExceptionInfo(
On 2015/04/10 18:46:49, newt wrote:
> "getContentSettingsExceptions()"

Done.

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java
(right):

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:404:
}
On 2015/04/10 18:46:49, newt wrote:
> newline

Done.

https://codereview.chromium.org/1068223002/diff/100001/chrome/browser/android...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/1068223002/diff/100001/chrome/browser/android...
chrome/browser/android/preferences/pref_service_bridge.cc:165:
DCHECK(content_settings_type == CONTENT_SETTINGS_TYPE_JAVASCRIPT ||
On 2015/04/10 18:46:49, newt wrote:
> This is a good check to add. Thanks :0

Acknowledged.

https://codereview.chromium.org/1068223002/diff/100001/chrome/browser/android...
chrome/browser/android/preferences/pref_service_bridge.cc:180: static void
SetContentSettingDefault(JNIEnv* env, jobject obj,
On 2015/04/10 18:46:49, newt wrote:
> "SetContentSettingEnabled()"

Done.

[Finnur, 2015-04-13 13:14:28]

Oh, by the way, I ran an end-to-end test on the three categories affected by
this cleanup and they seem to work fine. (just closing the loop on my earlier
comment)

[newt (away), 2015-04-13 22:49:07]

This all looks good to me now. Thanks!

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingExceptionInfo.java
(right):

https://codereview.chromium.org/1068223002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingExceptionInfo.java:14:
public class ContentSettingExceptionInfo implements Serializable {
On 2015/04/13 12:59:32, Finnur wrote:
> On 2015/04/10 18:46:49, newt wrote:
> > I'd remove "Info" from the name. ContentSettingException is cleaner.
> 
> I agree. However, I renamed the class and findbug now complains.
> 
> NM_CLASS_NOT_EXCEPTION: Class is not derived from an Exception, even though it
> is named as such
>   In class
> org.chromium.chrome.browser.preferences.website.ContentSettingException
>   At ContentSettingException.java:[lines 21-66]
> 
> That's probably why it was named as it was. What is the right way forward? Add
a
> suppression?

You can use an annotation to suppress the warning.
@SuppressFBWarnings("NM_CLASS_NOT_EXCEPTION")

https://codereview.chromium.org/1068223002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingException.java
(right):

https://codereview.chromium.org/1068223002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingException.java:20:
public ContentSettingException(
Please add javadoc. In particular, mPattern, mSetting, and mSource are
non-obvious.

[Finnur, 2015-04-14 11:36:22]

Done. PTAL.

[newt (away), 2015-04-14 16:41:57]

lgtm

[Finnur, 2015-04-15 08:03:07]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2015-04-15 08:04:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1068223002/140001

[commit-bot: I haz the power, 2015-04-15 08:13:01]

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

[commit-bot: I haz the power, 2015-04-15 08:13:02]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Finnur, 2015-04-15 11:17:33]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2015-04-15 11:17:34]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1068223002/#ps160001
(title: "Sync and fix merge conflict")

[commit-bot: I haz the power, 2015-04-15 11:17:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1068223002/160001

[commit-bot: I haz the power, 2015-04-15 12:41:56]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-04-15 12:43:01]

Patchset 9 (id:??) landed as
https://crrev.com/6c047d636359cd9136acf509839ac9c2d3fcaf2e
Cr-Commit-Position: refs/heads/master@{#325231}