UI for auto-detect encoding setting

UI for auto-detect encoding setting

Add checkbox for auto-detect encoding setting to "language" settings screen
(formerly named "translate") and impelment other UI changes.

BUG=512032
Committed: https://crrev.com/16cbeabb7cb284311275c7eda5bbcc2fce4f4261
Cr-Commit-Position: refs/heads/master@{#340610}

[Theresa, 2015-07-22 18:20:53]

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

[Theresa, 2015-07-22 18:20:54]

ptal

The UI is done, but I'm not sure the preference change is being handled
correctly on the native side in the current patchset.

The only place in our code base that I can find currently manipulating an
auto-detect encoding preference is in chrome/browser/ui/browser.cc. It's using
prefs::kWebKitUsesUniversalDetector as the pref name, so I have the UI checkbox
ultimately setting that preference. Does that seem correct to you as well?

Also, do we need logic similar to Browser::ToggleEncodingAutoDetect (see link
below) in a class used by android? Or will native-side implementation be handled
separately? I think the critical piece of this function is calling
WebContents::ResetOverrideEncoding().

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[newt (away), 2015-07-22 20:46:26]

On 2015/07/22 18:20:54, Theresa Wellington wrote:
> ptal
> 
> The UI is done, but I'm not sure the preference change is being handled
> correctly on the native side in the current patchset.

The UI code looks good. Just some nits. Thanks!

> 
> The only place in our code base that I can find currently manipulating an
> auto-detect encoding preference is in chrome/browser/ui/browser.cc. It's using
> prefs::kWebKitUsesUniversalDetector as the pref name, so I have the UI
checkbox
> ultimately setting that preference. Does that seem correct to you as well?

This seems plausible, but definitely verify that everything is working
correctly.
You can use the pages in chrome/test/data/encoding_tests/auto_detect to manually
verify that auto-detect encoding is working. This would also be a good thing to
write a test for. For a starting point, see SiteSettingsPreferencesTest.java.

> Also, do we need logic similar to Browser::ToggleEncodingAutoDetect (see link
> below) in a class used by android? Or will native-side implementation be
handled
> separately? I think the critical piece of this function is calling
> WebContents::ResetOverrideEncoding().
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

I think it's OK not to reload the current page, since the current page isn't
visible when the user is in Settings. Somewhat analogously, on desktop none of
the background pages are reloaded -- only the current page.

However, we should record the "AutoDetectChange" UMA stat, as in
Browser::ToggleEncodingAutoDetect.

[newt (away), 2015-07-22 20:46:33]

https://codereview.chromium.org/1248163002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/LanguagePreferences.java
(right):

https://codereview.chromium.org/1248163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/LanguagePreferences.java:84:
help.setShowAsAction(MenuItem.SHOW_AS_ACTION_ALWAYS);
This should probably be SHOW_AS_ACTION_IF_ROOM, or just leave it out altogether.
We want to avoid cluttering the action bar too much.

https://codereview.chromium.org/1248163002/diff/20001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1248163002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:655: <!-- Google langauge
preferences -->
"Language preferences" (typo, and remove "Google)

https://codereview.chromium.org/1248163002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:663: Allow Chrome to
offer to translate pages using Google Translate
How about removing "Allow Chrome to"? "Offer to translate pages using Google
Translate" seems like a clearer and simpler description.

https://codereview.chromium.org/1248163002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:671: <message
name="IDS_AUTO_DETECT_ENCODING" desc="Title for the auto-detect encoding
preference. ">
"Title for checkbox to enable or disable auto-detection of the character
encoding in web pages."

https://codereview.chromium.org/1248163002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:672: Auto-detect encoding
On desktop, there's no hyphen in auto-detect; it's just "Auto detect". We should
be consistent with them unless there's a good reason to deviate (or maybe
desktop should be changed?). You might want to run these string changes past
rolfe@ and see what she thinks.

[Theresa, 2015-07-22 21:58:50]

On 2015/07/22 20:46:26, newt wrote:
> On 2015/07/22 18:20:54, Theresa Wellington wrote:
> This seems plausible, but definitely verify that everything is working
> correctly.
> You can use the pages in chrome/test/data/encoding_tests/auto_detect to
manually
> verify that auto-detect encoding is working. This would also be a good thing
to
> write a test for. For a starting point, see SiteSettingsPreferencesTest.java.

Manual checking done, it works! I will write an automated test for this as well.

> I think it's OK not to reload the current page, since the current page isn't
> visible when the user is in Settings. Somewhat analogously, on desktop none of
> the background pages are reloaded -- only the current page.
> 
> However, we should record the "AutoDetectChange" UMA stat, as in
> Browser::ToggleEncodingAutoDetect.

Done.

[Theresa, 2015-07-22 22:14:06]

Still need to add test(s)

https://codereview.chromium.org/1248163002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/LanguagePreferences.java
(right):

https://codereview.chromium.org/1248163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/LanguagePreferences.java:84:
help.setShowAsAction(MenuItem.SHOW_AS_ACTION_ALWAYS);
On 2015/07/22 20:46:33, newt wrote:
> This should probably be SHOW_AS_ACTION_IF_ROOM, or just leave it out
altogether.
> We want to avoid cluttering the action bar too much.

Done.

https://codereview.chromium.org/1248163002/diff/20001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1248163002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:655: <!-- Google langauge
preferences -->
On 2015/07/22 20:46:33, newt wrote:
> "Language preferences" (typo, and remove "Google)

Done.

https://codereview.chromium.org/1248163002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:663: Allow Chrome to
offer to translate pages using Google Translate
On 2015/07/22 20:46:33, newt wrote:
> How about removing "Allow Chrome to"? "Offer to translate pages using Google
> Translate" seems like a clearer and simpler description.

Done.

https://codereview.chromium.org/1248163002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:671: <message
name="IDS_AUTO_DETECT_ENCODING" desc="Title for the auto-detect encoding
preference. ">
On 2015/07/22 20:46:33, newt wrote:
> "Title for checkbox to enable or disable auto-detection of the character
> encoding in web pages."

Done.

https://codereview.chromium.org/1248163002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:672: Auto-detect encoding
On 2015/07/22 20:46:33, newt wrote:
> On desktop, there's no hyphen in auto-detect; it's just "Auto detect". We
should
> be consistent with them unless there's a good reason to deviate (or maybe
> desktop should be changed?). You might want to run these string changes past
> rolfe@ and see what she thinks.

Done. Discussed with rolfe@, dropped the hyphen

[newt (away), 2015-07-23 01:46:48]

Looks good. Just waiting for a test :)

[Theresa, 2015-07-24 15:16:31]

ptal - added a (very basic) test

[newt (away), 2015-07-24 19:04:32]

Some small comments inline.

Also, I think we should test the guts of auto-encoding on Android too, at least
as a spot check. Here's an outline for a simple test:

    disable auto-encoding
    load Big5_with_no_encoding_specified.html
    check web_contents->GetEncoding() is the default value
    enable auto-encoding
    reload the page
    check web_contents->GetEncoding() is "Big5"

https://codereview.chromium.org/1248163002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java
(right):

https://codereview.chromium.org/1248163002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:231:
Bundle fragmentArgs = new Bundle();
unused

https://codereview.chromium.org/1248163002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:245:
checkbox.getOnPreferenceChangeListener().onPreferenceChange(checkbox, enabled);
Or maybe:

    if (checkbox.isChecked() != enabled) {
      PreferencesTest.performClick(languagePreferences, checkbox);
    }

That'll run the same code path that happens when a preference is clicked (e.g.
if the preference is disabled, it won't be clicked; or if the preference click
is handled via a different mechanism than a PreferenceChangeListener this will
still work). Calling the PreferenceChangeListener bypasses more of this
mechanism, and thus tests less of the system.

https://codereview.chromium.org/1248163002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:429:
* @throws Exception
nit: no need add "@throws" javadoc for tests. No one cares, since no one calls
these methods. (In this case, the test doesn't even throw Exception, so you
could just remove "throws Exception" below too)

[Theresa, 2015-07-24 21:21:30]

now testing encoding :)

https://codereview.chromium.org/1248163002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java
(right):

https://codereview.chromium.org/1248163002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:231:
Bundle fragmentArgs = new Bundle();
On 2015/07/24 19:04:32, newt wrote:
> unused

Done.

https://codereview.chromium.org/1248163002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:245:
checkbox.getOnPreferenceChangeListener().onPreferenceChange(checkbox, enabled);
On 2015/07/24 19:04:32, newt wrote:
> Or maybe:
> 
>     if (checkbox.isChecked() != enabled) {
>       PreferencesTest.performClick(languagePreferences, checkbox);
>     }
> 
> That'll run the same code path that happens when a preference is clicked (e.g.
> if the preference is disabled, it won't be clicked; or if the preference click
> is handled via a different mechanism than a PreferenceChangeListener this will
> still work). Calling the PreferenceChangeListener bypasses more of this
> mechanism, and thus tests less of the system.

Done.

https://codereview.chromium.org/1248163002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:429:
* @throws Exception
On 2015/07/24 19:04:32, newt wrote:
> nit: no need add "@throws" javadoc for tests. No one cares, since no one calls
> these methods. (In this case, the test doesn't even throw Exception, so you
> could just remove "throws Exception" below too)

Done.

[newt (away), 2015-07-24 21:28:55]

Awesome. Thanks! :)

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

https://codereview.chromium.org/1248163002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:435:
String testUrl =
This is not a URL :P

    String testUrl = TestHttpServerClient.getUrl(
           
"chrome/test/data/encoding_tests/auto_detect/Big5_with_no_encoding_specified.html");

https://codereview.chromium.org/1248163002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:440:
getInstrumentation().waitForIdleSync();
loadUrl() already calls waitForIdleSync(). Was the test failing/flaky before you
added this method? Avoid waitForIdleSync() as much as possible. It's a flaky
workaround for flaky behavior. If it appears to be needed, look for something
else that can be waited upon.

https://codereview.chromium.org/1248163002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/1248163002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:496:
web_contents_->GetEncoding());
indentation

https://codereview.chromium.org/1248163002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/1248163002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:290:
String getEncoding();
you'll need @VisibleForTesting on this (or perhaps on the implementation in
WebContentsImpl?) to prevent proguard from removing this method and causing
tests to fail in Release mode.

[Theresa, 2015-07-24 22:33:25]

twellington@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Theresa, 2015-07-24 22:33:25]

+tedchoc for content/browser/web_contents/web_contents_android.* and
content/public/android/*

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

https://codereview.chromium.org/1248163002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:435:
String testUrl =
On 2015/07/24 21:28:54, newt wrote:
> This is not a URL :P
> 
>     String testUrl = TestHttpServerClient.getUrl(
>            
>
"chrome/test/data/encoding_tests/auto_detect/Big5_with_no_encoding_specified.html");

Done.

https://codereview.chromium.org/1248163002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:440:
getInstrumentation().waitForIdleSync();
On 2015/07/24 21:28:54, newt wrote:
> loadUrl() already calls waitForIdleSync(). Was the test failing/flaky before
you
> added this method? Avoid waitForIdleSync() as much as possible. It's a flaky
> workaround for flaky behavior. If it appears to be needed, look for something
> else that can be waited upon.

Done.

https://codereview.chromium.org/1248163002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/1248163002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:496:
web_contents_->GetEncoding());
On 2015/07/24 21:28:54, newt wrote:
> indentation

Done.

https://codereview.chromium.org/1248163002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/1248163002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:290:
String getEncoding();
On 2015/07/24 21:28:54, newt wrote:
> you'll need @VisibleForTesting on this (or perhaps on the implementation in
> WebContentsImpl?) to prevent proguard from removing this method and causing
> tests to fail in Release mode.

Done.

[newt (away), 2015-07-24 22:36:36]

lgtm after one last comment

https://codereview.chromium.org/1248163002/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java
(right):

https://codereview.chromium.org/1248163002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:450:
getInstrumentation().waitForIdleSync();
Seems like you need to wait for the page load to finish here, rather than
waitForIdleSync(). Perhaps ChromeTabUtils.waitForTabPageLoaded() is what you
want.

[Theresa, 2015-07-24 22:43:29]

https://codereview.chromium.org/1248163002/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java
(right):

https://codereview.chromium.org/1248163002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:450:
getInstrumentation().waitForIdleSync();
On 2015/07/24 22:36:36, newt wrote:
> Seems like you need to wait for the page load to finish here, rather than
> waitForIdleSync(). Perhaps ChromeTabUtils.waitForTabPageLoaded() is what you
> want.

Done.

[Ted C, 2015-07-27 22:46:13]

lgtm

[Theresa, 2015-07-27 22:48:18]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2015-07-27 22:48:18]

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/1248163002/#ps120001
(title: "Replace waitForIdleSync with ChromeTabUtils.waitForTabPageLoaded")

[commit-bot: I haz the power, 2015-07-27 22:49:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1248163002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1248163002/120001

[commit-bot: I haz the power, 2015-07-27 23:59:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-28 00:00:28]

Patchset 7 (id:??) landed as
https://crrev.com/16cbeabb7cb284311275c7eda5bbcc2fce4f4261
Cr-Commit-Position: refs/heads/master@{#340610}

[Finnur, 2015-09-08 11:04:17]

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

[Finnur, 2015-09-08 11:04:17]

https://codereview.chromium.org/1248163002/diff/120001/chrome/android/java/re...
File chrome/android/java/res/xml/site_settings_preferences.xml (right):

https://codereview.chromium.org/1248163002/diff/120001/chrome/android/java/re...
chrome/android/java/res/xml/site_settings_preferences.xml:13: <!-- Language -->
nit: Indentation.

https://codereview.chromium.org/1248163002/diff/120001/chrome/android/java/re...
chrome/android/java/res/xml/site_settings_preferences.xml:18:
android:icon="@drawable/settings_language" />
Hmmm... :/ Just discovered this today when looking at the TOT chromium build on
Android.

This needs to be moved as 'Language' is not the most privacy sensitive item on
this list (see below). I suggest you move it to where Google Translate was, or
negotiate with Rebecca Rolfe where this should be placed.

I'd also like to see a comment at the top (line 6) stating something like:

<!-- The list below All Sites is ordered by privacy-sensitive items first. If
     you are not sure where to add your item, add it to the bottom. -->

[Finnur, 2015-09-08 11:06:28]

https://codereview.chromium.org/1248163002/diff/120001/chrome/android/java/re...
File chrome/android/java/res/xml/site_settings_preferences.xml (right):

https://codereview.chromium.org/1248163002/diff/120001/chrome/android/java/re...
chrome/android/java/res/xml/site_settings_preferences.xml:18:
android:icon="@drawable/settings_language" />
Hmm... 'Bottom' meaning above 'Storage'.

[Theresa, 2015-09-11 00:26:46]

On 2015/09/08 11:06:28, Finnur wrote:
>
https://codereview.chromium.org/1248163002/diff/120001/chrome/android/java/re...
> File chrome/android/java/res/xml/site_settings_preferences.xml (right):
> 
>
https://codereview.chromium.org/1248163002/diff/120001/chrome/android/java/re...
> chrome/android/java/res/xml/site_settings_preferences.xml:18:
> android:icon="@drawable/settings_language" />
> Hmm... 'Bottom' meaning above 'Storage'.

rolfe@ is fine with moving it back to where 'translate' was. I'll prepare a CL
moving the preference and adding a comment.